Page MenuHomePhabricator

wikibase.statement.saved hook is no longer fired after error in tainted refs when adding new statement
Closed, ResolvedPublic

Description

When adding a statement, the following error occurs in the console (with debug mode; out of debug mode the stack trace is less readable):

jQuery.Deferred exception: t is null 
value@https://www.wikidata.org/w/extensions/Wikibase/view/lib/wikibase-tainted-ref/dist/tainted-ref.init.js?a7eca:1:46591
value/<@https://www.wikidata.org/w/extensions/Wikibase/view/lib/wikibase-tainted-ref/dist/tainted-ref.init.js?a7eca:1:46195
fire@https://www.wikidata.org/w/resources/lib/jquery/jquery.js?11c05:3291:31
fireWith@https://www.wikidata.org/w/resources/lib/jquery/jquery.js?11c05:3421:7
fire@https://www.wikidata.org/w/load.php?debug=true&lang=en&modules=dataValues%2Cjquery%2Coojs%2Coojs-ui%2Coojs-ui-core%2Coojs-ui-toolbars%2Coojs-ui-widgets%2Coojs-ui-windows%2Csite%2CvalueFormatters%2CvalueParsers%2Cvue2%2Cwikibase%7CdataValues.DataValue%2CTimeValue%2Cvalues%7Cext.centralNotice.choiceData%2Cdisplay%2CgeoIP%2CimpressionDiet%2CkvStore%2CstartUp%7Cext.centralauth.ForeignApi%7Cext.centralauth.centralautologin.clearcookie%7Cext.citoid.wikibase.init%7Cext.echo.api%2Cinit%7Cext.eventLogging%2CnavigationTiming%2CwikimediaEvents%7Cext.uls.common%2Ccompactlinks%2Ci18n%2Cinit%2Cinter…
 undefined jquery.js:3841:18

This would be bad enough on its own (some bug in tainted refs); however, because tainted refs listens to the wikibase.statement.saved hook, due to T250069 this also prevents that hook from reaching any other listeners, for this event or any subsequent events. This can break gadgets and user scripts which rely on that hook being fired.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 13 2020, 12:24 PM

Wild guess: t is supposed to be the hook argument with the statement ID, which doesn’t exist yet for newly added statements? That might be why so far I haven’t seen this error when editing or removing existing statements.

Nikki added a subscriber: Nikki.Apr 13 2020, 12:27 PM
LucasWerkmeister added a comment.EditedApr 13 2020, 12:32 PM

Yeah, I think that’s more or less it. Hook arguments for a new statement (on test wikidata, logged with mw.hook( 'wikibase.statement.saved' ).add( console.log )):

Q117701
Q117701$d18c20ce-4a85-13a1-7ce8-0ba5fd15c69b
null
Object { _claim: {…}, _references: {…}, _rank: 1 }

Tainted References’ MWHookHandler:

	private addSaveHook( store: Store<Application> ): void {
		this.mwHooks( 'wikibase.statement.saved' ).add(
			( _entityId: string, statementId: string, oldStatement: Statement, newStatement: Statement ) => {
				if ( store.state.statementsTaintedState[ statementId ] ) {
					store.dispatch( STATEMENT_TAINTED_STATE_UNTAINT, statementId );
				} else if ( this.taintedChecker.check( oldStatement, newStatement ) ) {
					store.dispatch( STATEMENT_TAINTED_STATE_TAINT, statementId );
				}
			},
		);
		this.mwHooks( 'wikibase.statement.saved' ).add(
			( _entityId: string, _statementId: string, oldStatement: Statement, newStatement: Statement ) => {
				this.statementTracker.trackChanges( oldStatement, newStatement );
			},
		);
	}

The TaintedChecker:

	public check( oldStatement: Statement, newStatement: Statement ): boolean {
		return !( oldStatement.getClaim().getMainSnak().equals( newStatement.getClaim().getMainSnak() ) ) &&
			oldStatement.getReferences().equals( newStatement.getReferences() ) &&
			!oldStatement.getReferences().isEmpty();
	}

The StatementTracker:

	public trackChanges( oldStatement: Statement, newStatement: Statement ): void {
		const referenceChangeCount = this.refChangeCounter.countOldReferencesRemovedOrChanged(
			oldStatement.getReferences(),
			newStatement.getReferences(),
		);
		const oldRefCount = oldStatement.getReferences().length;
		const newRefCount = newStatement.getReferences().length;
		// ...
	}

Nothing here is prepared for the oldStatement being null.

Random-ish example of a broken user script (found via search): @matej_suchanek’s moveClaim.js is supposed to add the “move claim” button to newly added statements, but currently doesn’t. (The rest of the gadget probably still works, I haven’t tested it.)

Lydia_Pintscher triaged this task as High priority.Apr 19 2020, 1:37 PM

Does this only happen in the tainted reference situation (i.e. changing statement value without changing the reference), or on every statement edit?

It happens only not in the tainted reference situation, because if you’re changing the statement value (with or without changing the reference), then oldStatement won’t be null.

However, due to T250069, once this has happened anywhere on the page (i. e. you’ve added any new statement), the hook will never reach subscribers again until the page is reloaded, for tainted reference-eligible edits as well as other statement edits.

LucasWerkmeister renamed this task from wikibase.statement.saved hook is no longer fired after error in tainted refs to wikibase.statement.saved hook is no longer fired after error in tainted refs when adding new statement.Apr 21 2020, 12:21 PM

Change 591376 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] TR: Handle oldStatement being null in hook handler

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

Lucas_Werkmeister_WMDE claimed this task.EditedApr 21 2020, 4:42 PM

The above change causes TR to ignore this case; @Lydia_Pintscher should it be tracked to Grafana instead?

Edit: briefly discussed elsewhere; result: no need to track this, okay to ignore.

Change 591376 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] TR: Handle oldStatement being null in hook handler

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

LucasWerkmeister closed this task as Resolved.May 15 2020, 4:31 PM

This seems to be fixed; the error in the console is gone, and moveClaim.js again adds the “move claim” button to newly added statemtents.