Page MenuHomePhabricator

1.27-alpha / TitleMoveComplete-closure has invalid call signature
Closed, ResolvedPublic

Description

I just pulled in a fresh 1.27alpha (ab1e69b) and suddenly all move actions fail with:

[7468e0b6] /mw-core/index.php?title=%E7%89%B9%E5%88%A5:%E7%A7%BB%E5%8B%95&action=submit MWException from line 220 of ...mw-core\includes\Hooks.php: Detected bug in an extension! Hook hook-TitleMoveComplete-closure has invalid call signature; Parameter 3 to SMW\MediaWiki\Hooks\HookRegistry::SMW\MediaWiki\Hooks\{closure}() expected to be a reference, value given

Backtrace:

#0 ...mw-core\includes\MovePage.php(376): Hooks::run(string, array)
#1 [internal function]: MovePage->{closure}()
#2 ...mw-core\includes\db\Database.php(3310): call_user_func(Closure)
#3 ...mw-core\includes\db\Database.php(3562): DatabaseBase->runOnTransactionIdleCallbacks()
#4 ...mw-core\includes\db\loadbalancer\LoadBalancer.php(1055): DatabaseBase->commit(string, string)
#5 [internal function]: LoadBalancer->commitMasterChanges()
#6 ...mw-core\includes\db\loadbalancer\LBFactory.php(194): call_user_func_array(array, array)
#7 [internal function]: LBFactory->{closure}(LoadBalancer, string, array)
#8 ...mw-core\includes\db\loadbalancer\LBFactorySimple.php(152): call_user_func_array(Closure, array)
#9 ...mw-core\includes\db\loadbalancer\LBFactory.php(195): LBFactorySimple->forEachLB(Closure, array)
#10 ...mw-core\includes\db\loadbalancer\LBFactory.php(212): LBFactory->forEachLBCallMethod(string)
#11 ...mw-core\includes\MediaWiki.php(501): LBFactory->commitMasterChanges()
#12 ...mw-core\includes\MediaWiki.php(677): MediaWiki->doPreOutputCommit()
#13 ...mw-core\includes\MediaWiki.php(474): MediaWiki->main()
#14 ...mw-core\index.php(41): MediaWiki->run()
#15 {main}

MediaWiki 1.27alpha (ab1e69b)
PHP 5.6.8 (apache2handler)
MySQL 5.6.24

The claimed 3 parameter is a by-reference and this hook definition hasn't seen any change since (I can't remember ...) [0]. It surely works with MW 1.25/1.24/1.23.

$this->handlers['TitleMoveComplete'] = function ( &$oldTitle, &$newTitle, &$user, $oldId, $newId ) {

	$titleMoveComplete = new TitleMoveComplete(
		$oldTitle,
		$newTitle,
		$user,
		$oldId,
		$newId
	);

	return $titleMoveComplete->process();
};

For the sake of amusement I just cleared the hook handler and left it empty such as:

$this->handlers['TitleMoveComplete'] = function ( &$oldTitle, &$newTitle, &$user, $oldId, $newId ) {
	return true;
};

and it still complains with the same exception.

[0] https://github.com/SemanticMediaWiki/SemanticMediaWiki/blob/master/src/MediaWiki/Hooks/HookRegistry.php#L236-L247

Event Timeline

mwjames created this task.Nov 15 2015, 10:21 AM
mwjames updated the task description. (Show Details)
mwjames raised the priority of this task from to Needs Triage.
mwjames added a subscriber: mwjames.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptNov 15 2015, 10:21 AM

Could you pull the latest master? It was changed in https://gerrit.wikimedia.org/r/#/c/244044 and then partially reverted in https://gerrit.wikimedia.org/r/#/c/249189/

Could you pull the latest master?

I think I mentioned that 1.27alpha (ab1e69b) is used which according to

includes the suspected revert and it doesn't fix it.

It was changed in https://gerrit.wikimedia.org/r/#/c/244044

Moving before the cited change allows again to carry out move actions, so this is certainly a regression.

After a month of inactivity on this task, I just made a new attempt (1.27alpha (d545912)) after the following comment "The removal of the custom error handler for Hooks::run() is in 1.27.0-wmf.8 which should be active on all wikis now. I tested a page move on wikitech and could not recreate the error:" by bd808 [0] which most likely has the same cause.

I tried to move a page and it reports with "Parameter 3 to SMW\MediaWiki\Hooks\{closure}() expected to be a reference, value given in ...\includes\Hooks.php on line 195".

The closure is defined as function ( &$oldTitle, &$newTitle, &$user, $oldId, $newId ) and has been since the usage of MW 1.19.

[0] https://phabricator.wikimedia.org/T120218

Nikerabbit added a subscriber: Nikerabbit.

This might be a bug in HHVM. I modified the code as follows:

		$params = [
			&$this->oldTitle,
			&$this->newTitle,
			&$user,
			$pageid,
			$redirid,
			$reason,
			$nullRevision
		];
		
		var_dump( 'LOLXX', $params );
		$dbw->onTransactionIdle( function () use ( $params, $dbw ) {
			// Keep each single hook handler atomic
			$dbw->setFlag( DBO_TRX ); // flag is automatically reset by DB layer
			var_dump( 'LOLYY, $params );
			Hooks::run( 'TitleMoveComplete', $params );
		} );

At LOLXX $params[2] is reference, but at LOLYY it is only plain value. If I immediately invoke the anonymous function, it is also a reference.

I don't know how I would convince any knowledgeable person to look into this. I believe if I were to report this to HHVM upstream, they would ask minimal test case, but I can't provide anything but MediaWiki as-is. I am running HHVM 3.12.0 btw. It would be nice if someone could test this with PHP.

ori added a subscriber: ori.Mar 8 2016, 1:06 AM

I believe if I were to report this to HHVM upstream, they would ask minimal test case, but I can't provide anything but MediaWiki as-is. I am running HHVM 3.12.0 btw. It would be nice if someone could test this with PHP.

I think I have a repro case:

<?php
function expectsReference( &$reference ) { echo "ok\n"; }
call_user_func_array( 'expectsReference', [ [] ] );

Outputs:

Zend:

$ php5 closure.php
ok

HHVM 3.12.1:

$ hhvm closure.php
[Tue Mar  8 01:04:48 2016] [hphp] [2782:7f388c1dc100:0:000001] []
Warning: Parameter 1 to expectsReference() expected to be a reference, value given in /home/ori/closure.php on line 3
ok

Why was this closed? I encountered this issue on PHP 5.6 as reported (not HHVM) and T115781 only makes references to HHVM not PHP.

Nemo_bis reopened this task as Open.Apr 8 2016, 1:39 PM
Nemo_bis added a subscriber: Nemo_bis.

Probably just an oversight

rakkaus:#mediawiki-i18n- (1 lines skipped) [Tue May 10 12:06:20 2016] [hphp] [18882:7fd06c3ff700:22433:000002] [] \nWarning: Parameter
          3 to SMW\\MediaWiki\\Hooks\\HookRegistry::__invoke() expected to be a reference, value given in
          /srv/mediawiki/tags/2016-05-06_11:17:18/includes/Hooks.php on line 195
Danny_B moved this task from Backlog to Defect on the HHVM board.May 29 2016, 11:24 PM

I, too, don't think this is an HHVM issue - I got a report of it happening with MW 1.27.0 and PHP 5.6.24, no HHVM in use.

The problem also occurs with MW 1.27.1 - I just tried it out myself. (And PHP 5.6.25, and no HHVM.)

Subfader added a subscriber: Subfader.EditedOct 2 2016, 1:31 PM

How can MW 1.27.1 be stable with a broken hook?

PHP 5.5.9-1ubuntu4.14 (cli)

This comment was removed by Subfader.
Legoktm edited projects, added MW-1.27-release; removed HHVM.Jun 27 2017, 8:22 PM
Legoktm added a subscriber: Legoktm.

How can MW 1.27.1 be stable with a broken hook?

To answer the obvious, AFAIS this was never triaged as a 1.27 release blocker, I've added the proper tag now. (Also removing HHVM since it's not an HHVM bug apparently).

Change 361776 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@master] [WIP] Testing TitleMoveComplete hook with &

https://gerrit.wikimedia.org/r/361776

Tgr added a subscriber: Tgr.Jun 28 2017, 7:54 AM

Seems to be caused by the inherited parameter going out of scope. Here is a minimal reproduction:

global $hook;
$hook = function ( &$reference ) { echo "ok\n"; }

$wrap = function() {
    $user = [];
    $params = [ &$user ];
    $f = function () use ( $params ) {
        global $hook;
        call_user_func_array( $hook, $params );
    };
    echo "inner\n"; call_user_func( $f ); // works
    return $f;
};

$f = $wrap();
echo "outer\n"; call_user_func( $f ); // fails

Defining $f as function () use ( &$params ) does not help either.
(Tested on 5.6.99-hhvm.)

Tgr added a comment.EditedJun 28 2017, 8:01 AM

The simplest workaround I could find is to unpack/repack the parameter array:

global $hook;
$hook = function ( &$reference ) { echo "ok\n"; }

$wrap = function() {
    $user = [];
    $params = [ &$user ];
    $f = function () use ( $params ) {
        global $hook;
        list( $user ) = $params;
        call_user_func_array( $hook, [ &$user ] );
    };
    echo "inner\n"; call_user_func( $f ); // works
    return $f;
};

$f = $wrap();
echo "outer\n"; call_user_func( $f ); // works

It will break the reference but with the callback moved to post-commit it wouldn't do anything anyway...

Seems to be caused by the inherited parameter going out of scope. Here is a minimal reproduction:
<snip>
(Tested on 5.6.99-hhvm.)

Based on https://3v4l.org/XbNo3 it appears this works in PHP 7, but is broken in PHP 5 and HHVM.

Anomie added a subscriber: Anomie.EditedOct 16 2017, 6:07 PM

Here's a more minimal reproduction: https://3v4l.org/dlubp

Apparently PHP5 and HHVM convert a reference into a non-reference when no other reference to the value exists (i.e. the refcount goes to 1), much like how an existing non-reference is turned into a reference when you take a reference of it.

Another, simpler, workaround would be to just keep more than the one reference to $user live as long as $params is, e.g. with use ( $params, &$user ). Or you could do $tmp = &$params[2]; to turn it back into a reference just before the Hooks::run() call.

Krinkle moved this task from Backlog to Core on the MW-1.31-release board.Apr 18 2018, 6:08 PM

(Un)fortunately this issue will now only affect HHVM, since PHP 7 handles this correctly, and we no longer support <=5.6.

Change 438078 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@master] Ensure $user is passed by reference in TitleMoveComplete hook on HHVM

https://gerrit.wikimedia.org/r/438078

Change 438078 merged by jenkins-bot:
[mediawiki/core@master] Ensure $user is passed by reference in TitleMoveComplete hook on HHVM

https://gerrit.wikimedia.org/r/438078

Change 438249 had a related patch set uploaded (by Krinkle; owner: Legoktm):
[mediawiki/core@REL1_31] Ensure $user is passed by reference in TitleMoveComplete hook on HHVM

https://gerrit.wikimedia.org/r/438249

Change 438251 had a related patch set uploaded (by Krinkle; owner: Legoktm):
[mediawiki/core@REL1_30] Ensure $user is passed by reference in TitleMoveComplete hook on HHVM

https://gerrit.wikimedia.org/r/438251

Change 438254 had a related patch set (by Krinkle) published:
[mediawiki/core@REL1_27] Ensure $user is passed by reference in TitleMoveComplete hook on HHVM

https://gerrit.wikimedia.org/r/438254

Krinkle moved this task from Core to Done on the MW-1.31-release board.Jun 8 2018, 2:59 PM

Change 438249 merged by jenkins-bot:
[mediawiki/core@REL1_31] Ensure $user is passed by reference in TitleMoveComplete hook on HHVM

https://gerrit.wikimedia.org/r/438249

Krinkle moved this task from Backlog to Done on the MW-1.30-release board.Jun 8 2018, 3:02 PM

Change 438251 merged by jenkins-bot:
[mediawiki/core@REL1_30] Ensure $user is passed by reference in TitleMoveComplete hook on HHVM

https://gerrit.wikimedia.org/r/438251

Kghbln closed this task as Resolved.Jun 8 2018, 9:13 PM
Kghbln assigned this task to Legoktm.

I guess this is done now.

Change 438254 merged by jenkins-bot:
[mediawiki/core@REL1_27] Ensure $user is passed by reference in TitleMoveComplete hook on HHVM

https://gerrit.wikimedia.org/r/438254

Krinkle moved this task from Backlog to Done on the MW-1.27-release board.Jun 10 2018, 12:23 AM

Change 361776 abandoned by Legoktm:
[WIP] Testing TitleMoveComplete hook with &

https://gerrit.wikimedia.org/r/361776