Page MenuHomePhabricator

[4 hours] Warning: Invalid state error in MobileFormatter
Closed, ResolvedPublic

Description

Since this bug appeared while implementing the lead paragraph moving there is concern the number of errors may escalate upon rolling this out everywhere.

logstash is complaining:

Warning: Invalid State Error in /srv/mediawiki/php-1.29.0-wmf.3/extensions/MobileFrontend/includes/MobileFormatter.php on line 667
Warning: Invalid State Error in /srv/mediawiki/php-1.29.0-wmf.3/extensions/MobileFrontend/includes/MobileFormatter.php on line 673

This seems to relate to code in MobileFormatter::makeHeadingsEditable where we call setAttribute

Stack trace:

0 [internal function]: MWExceptionHandler::handleError(integer, string, string, integer, array, array)
#1 /srv/mediawiki/php-1.29.0-wmf.18/extensions/MobileFrontend/includes/MobileFormatter.php(672): DOMElement->setAttribute(string, string)
#2 /srv/mediawiki/php-1.29.0-wmf.18/extensions/MobileFrontend/includes/MobileFormatter.php(178): MobileFormatter->makeHeadingsEditable(array)
#3 /srv/mediawiki/php-1.29.0-wmf.18/extensions/MobileFrontend/includes/MobileFrontend.body.php(72): MobileFormatter->filterContent(boolean, boolean, boolean, boolean)
#4 /srv/mediawiki/php-1.29.0-wmf.18/extensions/MobileFrontend/includes/MobileFrontend.hooks.php(197): ExtMobileFrontend::DOMParse(OutputPage, string)
#5 /srv/mediawiki/php-1.29.0-wmf.18/includes/Hooks.php(186): MobileFrontendHooks::onOutputPageBeforeHTML(OutputPage, string)
#6 /srv/mediawiki/php-1.29.0-wmf.18/includes/OutputPage.php(1874): Hooks::run(string, array)
#7 /srv/mediawiki/php-1.29.0-wmf.18/includes/OutputPage.php(1892): OutputPage->addParserOutputText(ParserOutput)
#8 /srv/mediawiki/php-1.29.0-wmf.18/includes/page/Article.php(537): OutputPage->addParserOutput(ParserOutput)
#9 /srv/mediawiki/php-1.29.0-wmf.18/includes/actions/ViewAction.php(71): Article->view()
#10 /srv/mediawiki/php-1.29.0-wmf.18/includes/MediaWiki.php(497): ViewAction->show()
#11 /srv/mediawiki/php-1.29.0-wmf.18/includes/MediaWiki.php(291): MediaWiki->performAction(Article, Title)
#12 /srv/mediawiki/php-1.29.0-wmf.18/includes/MediaWiki.php(860): MediaWiki->performRequest()
#13 /srv/mediawiki/php-1.29.0-wmf.18/includes/MediaWiki.php(521): MediaWiki->main()
#14 /srv/mediawiki/php-1.29.0-wmf.18/index.php(43): MediaWiki->run()
#15 /srv/mediawiki/w/index.php(3): include(string)
#16 {main}

Frequency: ~20 per day

(Expected) Outcomes

  • We log the title of the article that's being parsed in an easily found error message.
  • e.g. prefix it with T151838.
  • The nature of the error is well understood and the error itself is fixed.

Example Articles

https://en.wikipedia.org/wiki/Schopenhauer
https://en.wikipedia.org/wiki/Bahri_dynasty
https://es.wikipedia.org/wiki/Altos_Hornos_de_M%C3%A9xico

Replication instructions

  1. Ensure lazy loaded references is enabled in beta mode:
$wgMFLazyLoadReferences = array(
  'useMobileViewApi' => true,
  'base' => false,
  'beta' => true,
);
$wgMFRemovableClasses = [
  'base' => [ '.navbox', '.nomobile' ],
  'beta' => [ '.navbox', '.nomobile' ],
  'HTML' => [],
];
  1. Check out the magic patch to load content directly from enwiki.
  2. Navigate to /wiki/Arthur_Schopenhauer?mobileaction=beta on your development wiki to see the error.

QA

This task can't be QA'd directly and can skip the Needs QA column.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. ยท View Herald TranscriptNov 29 2016, 12:47 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.

Frequency: ~20 per day

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

Apparently a problem setting or getting the class attribute on a dom node. Source:

MobileFormatter.php#667
/**
 * Marks the headings as editable by adding the <code>in-block</code>
 * class to each of them, if it hasn't already been added.
 *
 * FIXME: <code>in-block</code> isn't semantic in that it isn't
 * obviously connected to being editable.
 *
 * @param [DOMElement] $headings
 */
protected function makeHeadingsEditable( array $headings ) {
	foreach ( $headings as $heading ) {
		$class = $heading->getAttribute( 'class' );

		if ( strpos( $class, 'in-block' ) === false ) {
			$heading->setAttribute(
				'class',
				ltrim( $class . ' in-block' )
			);
		}
	}
}
pmiazga triaged this task as High priority.Nov 30 2016, 5:20 PM
pmiazga set the point value for this task to 2.

Invalid State Error is not documented in PHP docs. I checked in DOM extension source code and it looks like PHP engine cannot find pointer to given node element (https://github.com/php/php-src/blob/master/ext/dom/attr.c#L102)

There are two possible reasons:

  • element got moved/removed
  • HTML is broken and DOMDocument cannot fetch headings data
bmansurov renamed this task from Warning: Invalid state error in MobileFormatter to 4 hours: Warning: Invalid state error in MobileFormatter.Dec 14 2016, 4:17 PM
bmansurov updated the task description. (Show Details)
bmansurov removed the point value for this task.
ovasileva lowered the priority of this task from High to Normal.Dec 14 2016, 4:18 PM
ovasileva moved this task from Upcoming to Triaged but Future on the Readers-Web-Backlog board.
demon added a subscriber: demon.

(Still happening as of wmf.12/wmf.13)

@ovasileva we may want to bring this in to the next sprint.

Jhernandez raised the priority of this task from Normal to High.Feb 22 2017, 11:28 AM

I worked on this task couple months ago and I couldn't find what's wrong. Looks like only couple articles cause this issue - error occurs when parsing the article HTML. IMHO the easiest way to fix it is adding page URL in our error log. With the URL we will be able to verify the article HTML and fix or solution or the article (it's possible that the article HTML is malformed/broken)

Adding extra debugging sounds like a good starting point!

pmiazga removed pmiazga as the assignee of this task.Mar 15 2017, 5:16 PM
pmiazga added a subscriber: pmiazga.
phuedx updated the task description. (Show Details)Mar 21 2017, 4:34 PM
phuedx updated the task description. (Show Details)
phuedx updated the task description. (Show Details)
phuedx updated the task description. (Show Details)Mar 22 2017, 5:55 AM

I'm not sure when. but these errors started being converted to exceptions before being logged. The errors are now being logged with the wiki and the title that was being viewed when the error occurred (woo!). I've added example links to the wiki pages to the task description.

@phuedx - I would love to see how can I add a title to error log as similar case might happen in different feature.

Jdlrobson updated the task description. (Show Details)Apr 6 2017, 6:10 PM
Jdlrobson updated the task description. (Show Details)
pmiazga removed a subscriber: pmiazga.Apr 13 2017, 9:26 PM
Jdlrobson renamed this task from 4 hours: Warning: Invalid state error in MobileFormatter to [4 hours] Warning: Invalid state error in MobileFormatter.Apr 19 2017, 5:49 PM
Jdlrobson moved this task from Needs Analysis to To Do on the Reading-Web-Sprint-96 board.

Can we at very least suppress this error from the logs when we encounter it?

pmiazga added a comment.EditedApr 19 2017, 8:44 PM

Not really, in PHP7 all errors became exceptions so in PHP7 it would be possible but in PHP5/(and probably HHVM) it's not possible. We could change the error_reporting() to suppress all errors in given block of code but this is bad practice and we shouldn't do that.

Jdlrobson updated the task description. (Show Details)Apr 24 2017, 9:15 PM
Jdlrobson updated the task description. (Show Details)Apr 24 2017, 9:55 PM
Jdlrobson moved this task from To Do to Doing on the Reading-Web-Sprint-96 board.

After debugging this with @pmiazga we found a fix! Woop!!

Change 350088 had a related patch set uploaded (by Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Fix invalid state error

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

Change 350088 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix invalid state error

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

Jdlrobson removed a project: Patch-For-Review.

@pmiazga can we add some tests in a follow-up?

I'll think about it but tbh nothing comes to my mind

phuedx claimed this task.Apr 25 2017, 6:08 AM

I'll sign this off today.

(Also, nice work! ๐Ÿ’ฅ๐Ÿ’ฅ๐Ÿ’ฅ )

phuedx updated the task description. (Show Details)Apr 25 2017, 9:01 AM
phuedx closed this task as Resolved.EditedApr 25 2017, 9:57 AM

๐Ÿ‘

I made sure the replication instructions worked/were repeatable and then verified that the fix made the warning go away.