Page MenuHomePhabricator

[2hrs] Production warning: Undefined index: token in InlineDifferenceEngine
Closed, ResolvedPublicSpike

Description

logstash is complaining:

[Mon Nov 28 23:45:01 2016] [hphp] [106011:7f25d87ff700:62565:000001] [] \nNotice: Undefined index: token in /srv/mediawiki/php-1.29.0-wmf.3/extensions/MobileFrontend/includes/diff/InlineDifferenceEngine.php on line 169

This seems to relate to the patrol link code.

Frequency: 2 per hour, a couple of hours.

Details

Related Gerrit Patches:
mediawiki/extensions/MobileFrontend : masterdiff: $linkInfo is not an array of query parameters
mediawiki/extensions/MobileFrontend : masterToken is not always set in diff

Event Timeline

Restricted Application added a subscriber: Aklapper. Β· View Herald TranscriptNov 29 2016, 12:42 AM
Jdlrobson renamed this task from Production warning: Undefined index: token to Production warning: Undefined index: token in InlineDifferenceEngine.Nov 29 2016, 12:43 AM
phuedx added a subscriber: phuedx.Nov 29 2016, 8:05 AM

@Jdlrobson: Could you add the frequency at which this warning is triggered? It'd be useful for prioritisation.

2 per hour on a couple of hours. Nothing too serious it seems. I'll add it to the description.

Jhernandez updated the task description. (Show Details)Nov 29 2016, 10:10 AM
Jhernandez added a comment.EditedNov 29 2016, 10:13 AM

The offending source fo reference.

InlineDifferenceEngine.php#169
/**
 * Create a getter function for the patrol link in Mobile Diff.
 * FIXME: This shouldn't be needed, but markPatrolledLink is protected in DifferenceEngine
 * @return String
 */
public function getPatrolledLink() {
	$linkInfo = $this->getMarkPatrolledLinkInfo();
	if ( $linkInfo ) {
		$this->getOutput()->addModules( 'mobile.patrol.ajax' );
		$linkInfo = Html::linkButton(
			$this->msg( 'markaspatrolleddiff' )->escaped(),
			[
				'href' => $this->mNewPage->getLocalUrl( [
					'action' => 'markpatrolled',
					'rcid' => $linkInfo['rcid'],
					'token' => $linkInfo['token'],
				] ),
			]
		);
	}
	return $linkInfo;
}
MBinder_WMF renamed this task from Production warning: Undefined index: token in InlineDifferenceEngine to [2hrs] Production warning: Undefined index: token in InlineDifferenceEngine.Nov 30 2016, 5:27 PM
MBinder_WMF triaged this task as Low priority.
MBinder_WMF added a project: Spike.
MBinder_WMF moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.

This was broken by T130946 and I6be2c07824c17b165e068fc4ac36ab192e12bc9d which removed the token from this method which is why it reports as unset.

Jdlrobson raised the priority of this task from Low to Medium.Nov 30 2016, 10:54 PM

I'm bumping priority as I worry that this will drop of our radar and I suspect this prevents patrollers from patrolling on mobile (hence why we see so few events - not everyone is a patroller)

These errors also occur more than T151838 which is high so we may want to make this high as well.

These errors also occur more than T151838 which is high so we may want to make this high as well.

AFAICT T151838: [4 hours] Warning: Invalid state error in MobileFormatter was made high priority as it'd been moved into the sprint (without discussion). You might want to bring this issue to @ovasileva's attention.

@Jdlrobson - normal sounds good. And yes, T151838: [4 hours] Warning: Invalid state error in MobileFormatter was made high since it was already in the sprint.

Change 334416 had a related patch set uploaded (by Jdlrobson):
Token is not always set in diff

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

@Jdlrobson: I'm going to need a little more information in order to reproduce the warning. I can't get the lines in question to execute, i.e. I can't get a patrol link to be generated. I can see that DifferenceEngine#getMarkPatrolledLinkInfo no longer returns a dictionary containing the 'token' key, which is why the warning is being triggered and I can also tell that your change fixes the issue but, if possible, I'd like to see it first hand before I +2.

I seem to be able to replicate this when I visit a diff page that belongs to a user with no groups.
So maybe this might replicate it for you:

  1. Create new account
  2. Edit some pages as that user
  3. Logout
  4. Visit diffs of those edits

Note permissions and warnings in these two examples:


πŸ‘ It was the "Create a new account" step that I'd skipped. The user I was editing as was a sysop and so their edits were auto-patrolled.

Change 334416 merged by jenkins-bot:
Token is not always set in diff

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

phuedx assigned this task to bmansurov.Jan 30 2017, 10:14 AM

I guess this can be signed off once the change is deployed to all of the Wikipedias (Thursday, 2nd February).

Jdlrobson closed this task as Resolved.Feb 3 2017, 10:20 PM

No errors have occurred since 2017-02-02T23:35:28 according to the query above. That said... T157172

Change 341930 had a related patch set uploaded (by Krinkle):
[mediawiki/extensions/MobileFrontend] diff: $linkInfo is not an array of query parameters

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

Change 341930 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend] diff: $linkInfo is not an array of query parameters

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

Jhernandez closed this task as Resolved.Mar 14 2017, 11:48 AM

Let's reopen if there are any related issues

I'm bumping priority as I worry that this will drop of our radar and I suspect this prevents patrollers from patrolling on mobile (hence why we see so few events - not everyone is a patroller)

Have we seen an increase in the number of "patroller events" – I'm not sure exactly what you mean here – since the error was fixed?

Huh? This comment is from November way before we fixed it.

I know it's an old comment. I was just wonder if we've seen an increase in the number of the events that you mentioned. Is there any way to check?

I didn't write this so well. The events I was referring to were notices. I was simply pointing out that not all users are patrollers so this would only impact any logged in user with the patrol right viewing the diff page. I don't believe we track patrolling in any way.

mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:11 PM
Restricted Application changed the subtype of this task from "Production Error" to "Spike". Β· View Herald TranscriptAug 28 2019, 11:11 PM