Page MenuHomePhabricator

PHP Notice: Trying to get property 'textContent' of non-object
Closed, ResolvedPublic

Description

In the production logs today on https://wikivoyage.org/wiki/Gran_Canaria

[{exception_id}] {exception_url} ErrorException from line 257 of /srv/mediawiki/php-1.36.0-wmf.9/extensions/MobileFrontend/includes/Transforms/MoveLeadParagraphTransform.php: PHP Notice: Trying to get property 'textContent' of non-object

#0 /srv/mediawiki/php-1.36.0-wmf.9/extensions/MobileFrontend/includes/Transforms/MoveLeadParagraphTransform.php(257): MWExceptionHandler::handleError(integer, string, string, integer, array)
#1 /srv/mediawiki/php-1.36.0-wmf.9/extensions/MobileFrontend/includes/Transforms/MoveLeadParagraphTransform.php(134): MobileFrontend\Transforms\MoveLeadParagraphTransform->isNonLeadParagraph(DOMXPath, DOMElement)
#2 /srv/mediawiki/php-1.36.0-wmf.9/extensions/MobileFrontend/includes/Transforms/MoveLeadParagraphTransform.php(168): MobileFrontend\Transforms\MoveLeadParagraphTransform->identifyLeadParagraph(DOMXPath, DOMElement)
#3 /srv/mediawiki/php-1.36.0-wmf.9/extensions/MobileFrontend/includes/Transforms/MoveLeadParagraphTransform.php(37): MobileFrontend\Transforms\MoveLeadParagraphTransform->moveFirstParagraphBeforeInfobox(DOMElement, DOMDocument)
#4 /srv/mediawiki/php-1.36.0-wmf.9/extensions/MobileFrontend/includes/Transforms/MakeSectionsTransform.php(146): MobileFrontend\Transforms\MoveLeadParagraphTransform->apply(DOMElement)
#5 /srv/mediawiki/php-1.36.0-wmf.9/extensions/MobileFrontend/includes/Transforms/MakeSectionsTransform.php(260): MobileFrontend\Transforms\MakeSectionsTransform->makeSections(DOMElement, array)
#6 /srv/mediawiki/php-1.36.0-wmf.9/extensions/MobileFrontend/includes/MobileFormatter.php(158): MobileFrontend\Transforms\MakeSectionsTransform->apply(DOMElement)
#7 /srv/mediawiki/php-1.36.0-wmf.9/extensions/MobileFrontend/includes/ExtMobileFrontend.php(103): MobileFormatter->applyTransforms(boolean, boolean)
#8 /srv/mediawiki/php-1.36.0-wmf.9/extensions/MobileFrontend/includes/MobileFrontendHooks.php(286): ExtMobileFrontend::domParse(OutputPage, string, boolean)
#9 /srv/mediawiki/php-1.36.0-wmf.9/includes/HookContainer/HookContainer.php(330): MobileFrontendHooks::onOutputPageBeforeHTML(OutputPage, string)
#10 /srv/mediawiki/php-1.36.0-wmf.9/includes/HookContainer/HookContainer.php(137): MediaWiki\HookContainer\HookContainer->callLegacyHook(string, array, array, array)
#11 /srv/mediawiki/php-1.36.0-wmf.9/includes/HookContainer/HookRunner.php(2730): MediaWiki\HookContainer\HookContainer->run(string, array)
#12 /srv/mediawiki/php-1.36.0-wmf.9/includes/OutputPage.php(2005): MediaWiki\HookContainer\HookRunner->onOutputPageBeforeHTML(OutputPage, string)
#13 /srv/mediawiki/php-1.36.0-wmf.9/includes/OutputPage.php(2017): OutputPage->addParserOutputText(ParserOutput, array)
#14 /srv/mediawiki/php-1.36.0-wmf.9/includes/page/Article.php(742): OutputPage->addParserOutput(ParserOutput, array)
#15 /srv/mediawiki/php-1.36.0-wmf.9/includes/actions/ViewAction.php(74): Article->view()
#16 /srv/mediawiki/php-1.36.0-wmf.9/includes/MediaWiki.php(527): ViewAction->show()
#17 /srv/mediawiki/php-1.36.0-wmf.9/includes/MediaWiki.php(313): MediaWiki->performAction(Article, Title)
#18 /srv/mediawiki/php-1.36.0-wmf.9/includes/MediaWiki.php(940): MediaWiki->performRequest()
#19 /srv/mediawiki/php-1.36.0-wmf.9/includes/MediaWiki.php(543): MediaWiki->main()
#20 /srv/mediawiki/php-1.36.0-wmf.9/index.php(53): MediaWiki->run()
#21 /srv/mediawiki/php-1.36.0-wmf.9/index.php(46): wfIndexMain()
#22 /srv/medi

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptWed, Sep 16, 3:16 PM
brennen triaged this task as Unbreak Now! priority.EditedWed, Sep 16, 3:33 PM
brennen added a project: User-brennen.
brennen added subscribers: LarsWirzenius, brennen.

This looks to me like a probable train blocker; ~35 or so since rollout of wmf.9 and still happening at a low level.

cc: @LarsWirzenius.

Seems reproducible at these URLs, at least with a mobile client. (I'm fuzzy on how MobileFrontend actually works.) I don't see obvious user-facing breakage, but I may not know what to look for.

Ammarpad added a subscriber: Ammarpad.EditedWed, Sep 16, 4:07 PM

DOMNodeList::item returns DOMNode object or null on failure The codepath itself is fragile per the comment there (working on assumption)

if ( $nodeContent ) {
 // assume valid HTML and only one #coordinates element
 // this may not behave correctly if garbage in.
 $coordEl = $coords->item( 0 );
 // Is there content of this node in addition to the coordinates ?
 return $nodeContent === trim( $coordEl->textContent );
}

Investigating...

Change 627877 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/MobileFrontend@master] Check $coords matched some nodes before comparing contents

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

Esanders updated the task description. (Show Details)Wed, Sep 16, 4:18 PM

I verified the above fix locally by pointing my content API at https://wikivoyage.org/wiki/Gran_Canaria. I will write a test case later.

I can review and test

Change 627793 had a related patch set uploaded (by Jdlrobson; owner: Esanders):
[mediawiki/extensions/MobileFrontend@wmf/1.36.0-wmf.9] Check $coords matched some nodes before comparing contents

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

Thanks @Esanders for the testing instructions that made review super easy.
Thanks @Ammarpad for the detective work!

Over to you @brennen > https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/627793

Jdlrobson reassigned this task from Jdlrobson to brennen.Wed, Sep 16, 4:36 PM

Change 627881 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/MobileFrontend@master] Add test case for template styles inside paragraph with content

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

Test case as promised ^. Looks like I didn't account for other content in the <p> wrapping a TemplateStyles <style> tag. Presumably this is an inline template using TemplateStyles.

Change 627793 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@wmf/1.36.0-wmf.9] Check $coords matched some nodes before comparing contents

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

Mentioned in SAL (#wikimedia-operations) [2020-09-16T18:00:09Z] <brennen@deploy1001> Synchronized php-1.36.0-wmf.9/extensions/MobileFrontend: Backport: [[gerrit:627793|Check $coords matched some nodes before comparing contents (T263034)]] (duration: 01m 06s)

Fixed from wmf.9 train perspective.

brennen removed brennen as the assignee of this task.Wed, Sep 16, 6:55 PM
brennen lowered the priority of this task from Unbreak Now! to Medium.
brennen moved this task from Backlog to Logs/Train on the User-brennen board.

Change 627877 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Check $coords matched some nodes before comparing contents

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

Change 627881 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add test case for template styles inside paragraph with content

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

Jdlrobson closed this task as Resolved.Thu, Sep 17, 6:13 PM

No errors in last 12 hrs.