Page MenuHomePhabricator

Cannot read property 'stored' of undefined
Closed, ResolvedPublic

Description

https://logstash.wikimedia.org/app/kibana#/doc/logstash-*/logstash-2019.09.30/parsoid?id=AW2DwSUFCfBFUvHRUZBs&_g=h@1251ff0

https://logstash.wikimedia.org/app/kibana#/doc/logstash-*/logstash-2019.09.30/parsoid?id=AW2DxiB7ZKA7RpirdFhD&_g=h@44136fa

Wonder if this is related to https://github.com/wikimedia/parsoid/commit/6c4f8804c98ba8c63fbdb6591df584bed2c71c31

TypeError: Cannot read property 'stored' of undefined
    at Function.getNodeData (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/utils/DOMDataUtils.js:61:29)
    at Function.getDataParsoid (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/utils/DOMDataUtils.js:72:19)
    at Function.usesURLLinkSyntax (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/utils/WTUtils.js:84:22)
    at ConversionTraverser.aHandler (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/language/LanguageConverter.js:231:16)
    at Object.ConversionTraverser.addHandler [as action] (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/language/LanguageConverter.js:174:42)
    at ConversionTraverser.DOMTraverser.callHandlers (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/utils/DOMTraverser.js:57:25)
    at ConversionTraverser.DOMTraverser.traverse (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/utils/DOMTraverser.js:119:27)
    at ConversionTraverser.DOMTraverser.traverse (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/utils/DOMTraverser.js:130:10)
    at ConversionTraverser.DOMTraverser.traverse (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/utils/DOMTraverser.js:130:10)
    at ConversionTraverser.DOMTraverser.traverse (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/utils/DOMTraverser.js:130:10)
    at ConversionTraverser.DOMTraverser.traverse (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/utils/DOMTraverser.js:130:10)
    at ConversionTraverser.DOMTraverser.traverse (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/utils/DOMTraverser.js:130:10)
    at ConversionTraverser.DOMTraverser.traverse (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/utils/DOMTraverser.js:130:10)
    at ConversionTraverser.DOMTraverser.traverse (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/utils/DOMTraverser.js:130:10)
    at ConversionTraverser.DOMTraverser.traverse (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/utils/DOMTraverser.js:130:10)
    at ConversionTraverser.DOMTraverser.traverse (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/utils/DOMTraverser.js:130:10)
    at Function.baseToVariant (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/language/LanguageConverter.js:456:5)
    at Function.maybeConvert (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/language/LanguageConverter.js:404:8)
    at _languageConversion (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/parse.js:107:20)
    at /srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/src/lib/parse.js:213:11
    at next (native)
    at tryCatchNext (/srv/deployment/parsoid/deploy-cache/revs/77630c59f081ecef658fea29861a6c2835b7add9/node_modules/prfun/lib/index.js:783:28)

Details

Related Gerrit Patches:
mediawiki/services/parsoid : masterStop using DOMDataUtils in LC pass
mediawiki/services/parsoid : masterRevert "Log an error if HTML has already been variant converted"
mediawiki/services/parsoid : masterLog an error if HTML has already been variant converted

Event Timeline

Arlolra created this task.Sep 30 2019, 8:30 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 30 2019, 8:31 PM

There are instances of this error before Sep 19th in kibana, but since Sep 24, these greatly increased which is odd given that our previous deploy was Sep 19th.
But, anyway, there are a lot of these errors in kibana since Sep 24th ... so, should at least be easy to track down.

All the failures seem to be for zhwiki requests by presumably the iOS app based on the user agent string there (not pasting that email id here). They all say 'Main_Page' as the title which indicates a post request. And, they all seem to have stopped today after the mobileapps deploy at 3:00-3:06 pm CT. But, let us wait and see if they reappear. But, not sure we have enough information to debug this yet. But, independent of what in mobileapps caused this, this does look like something broken in Parsoid.

Scratch that. The errors have come back. But, they are all zhwiki and the iOS app user agent string and Main_Page.

Yup, still happening at a rate of approx 100 errors/hour.

Arlolra claimed this task.Wed, Nov 13, 10:59 PM

The RESTBase logs are a little more useful here in that they surface page names,

res.headers.content-location	https://zh.wikipedia.org/api/rest_v1/page/html/%E4%B8%AD%E5%8D%8E%E4%BA%BA%E6%B0%91%E5%85%B1%E5%92%8C%E5%9B%BD%E9%93%81%E8%B7%AF%E9%9A%A7%E9%81%93%E5%88%97%E8%A1%A8/56865865

https://logstash.wikimedia.org/app/kibana#/doc/logstash-*/logstash-2019.11.14/restbase?id=AW5nh5_rh3Uj6x1z8F-0&_g=h@44136fa

Given that,

curl --header "Accept-Language: zh-cn" https://zh.wikipedia.org/api/rest_v1/page/html/%E4%B8%AD%E5%8D%8E%E4%BA%BA%E6%B0%91%E5%85%B1%E5%92%8C%E5%9B%BD%E9%93%81%E8%B7%AF%E9%9A%A7%E9%81%93%E5%88%97%E8%A1%A8/56865865

crashes pretty consistently.

Ok, I can somewhat reproduce this locally now.

The problem stems from the conflux of,
https://github.com/wikimedia/parsoid/commit/554dfcf35dd30d1cb91bb619bb22acde5c2fa47d
and,
https://github.com/wikimedia/parsoid/commit/7df5d973a0b83a4682d6b66274c55ddcb0a18894

When 554dfcf landed, checking data-parsoid (the call to WTUtils.usesURLLinkSyntax(node)) merely left behind a .dataobject on a node that would be dropped when serialized. After 7df5d97, it leaks a data-object-id attribute. In other words, using DOMDataUtils in what are supposed to be pure transformation on the output is fraught.

This might have been caught sooner if the post-processing pass done while parsing was pushed to after cleanup, where the redlinks pass happens,
https://github.com/wikimedia/parsoid/blob/master/lib/wt2html/DOMPostProcessor.js#L307-L312

Indeed, doing so causes some parserTests to fail.

However, the thing that still remains the mystery is that the leak happens while doing a transform. So, that implies transforming already transformed content? Is it possible that RESTBase is doubling up somehow?

Change 550899 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] [WIP] Stop using DOMDataUtils in LC pass

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

Arlolra triaged this task as Medium priority.Thu, Nov 14, 6:19 PM
mobrovac added a comment.EditedFri, Nov 15, 10:18 AM

I am not quite certain I understand your question, but the only scenario I can think of where RESTBase is definitely sending already transformed content is when editors switch from VE to WT and then back:

  1. the editor starts editing with VE; RB stashes the content
  2. the editor switches to WT-editing; the content gets transformed to WT and that gets stashed
  3. the editor switches back to VE, edits some more and saves; RB picks up the transforms from (1) and sends them to Parsoid

Would that explain this? Even if it does, I'm having a hard time explaining the number of occurrences of this error: 100 editors per hour constantly switching between VE and WT sounds unlikely to me.

I am not quite certain I understand your question

Sorry, let me try to clarify but we can speak on IRC.

We're only talking about language conversion here, so the /transform/pagebundle/to/pagebundle/ endpoint.

The question is if content that has already been converted to one variant is then asked to be converted to another.

cscott added a subscriber: cscott.Mon, Nov 18, 4:32 PM

@mobrovac From today's Parsing-Team discussion, we're thinking of adding some logs for the http-equiv meta tag to see if the HTML we're receiving is already populated with Accept-Language.

Change 551623 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Log a warning if HTML has already been variant converted

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

Change 551623 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Log an error if HTML has already been variant converted

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

After deploying the patch in T234266#5673264 there're a ton of logs showing that RESTBase is sending HTML that had already been variant converted to the pb2pb endpoint. That seems like a bug.

For example,
https://logstash.wikimedia.org/app/kibana#/doc/logstash-*/logstash-2019.11.18/parsoid?id=AW6AhhUhKWrIH1QRqeCO&_g=h@1251ff0

mobrovac claimed this task.Tue, Nov 19, 4:07 PM

After deploying the patch in T234266#5673264 there're a ton of logs showing that RESTBase is sending HTML that had already been variant converted to the pb2pb endpoint. That seems like a bug.
For example,
https://logstash.wikimedia.org/app/kibana#/doc/logstash-*/logstash-2019.11.18/parsoid?id=AW6AhhUhKWrIH1QRqeCO&_g=h@1251ff0

Hm, well, to be exact the logs are not as useful per se for two reasons:

  1. RESTBase always adds the Vary: Accept-Language header to the response whenever conversions are possible (regardless of whether the exact request is actually a language conversion)
  2. Parsoid seems to do the same for http-equiv meta tag, as all of HTMLs in RB's storage for domains that supports language variations have them

That said, it does seem like the problem is on the RESTBase side. I have been able to reliably reproduce the error when requesting variants for page/html/{title}/{revision}. Weirdly, asking conversions for /page/html/{title} does not produce the error. I need to investigate further.

Hm, well, to be exact the logs are not as useful per se for two reasons:

Ok, that does seem awfully silly of me. I guess I misconstrued what Scott was saying. Should have verified, sorry.

Change 551863 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Revert "Log an error if HTML has already been variant converted"

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

Change 551863 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Revert "Log an error if HTML has already been variant converted"

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

So, if I'm understanding correctly, RESTBase is always going to add the header on (say) zhwiki, but the usual sequence is:

wt2html, w/ Accept-Language: zh (ie, no conversion is done by Parsoid)
html2html, w/ Accept-Language: zh-hans (ie conversion)

So @Arlolra was initially assuming the Accept-Language indicated language conversion, but you actually needed to look at the contents of the header to determine whether Parsoid was actually going to trigger the language conversion code?

I imagine the code that was reverted could be made to work by checking for a /\-/ in the language?

I've established that it's definitely RESTBase's fault: for some reason, when the revision is provided, it asks for the conversion twice, but the second time it provides the content returned to it by Parsoid for the first conversion request. I'm still investigating why and how that happens. To be continued.

Ok found the bug. The problem was that the filter routine that executed the actual conversion was loaded twice. PR #1219 fixes this.

Change 550899 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Stop using DOMDataUtils in LC pass

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

Mentioned in SAL (#wikimedia-operations) [2019-11-26T07:29:34Z] <mobrovac@deploy1001> Started deploy [restbase/deploy@378f504]: Do not use duplicate filter definitions T234266

Mentioned in SAL (#wikimedia-operations) [2019-11-26T07:43:58Z] <mobrovac@deploy1001> Finished deploy [restbase/deploy@378f504]: Do not use duplicate filter definitions T234266 (duration: 14m 24s)

mobrovac closed this task as Resolved.Tue, Nov 26, 9:08 AM
mobrovac removed a project: Patch-For-Review.

As of the RESTBase deploy, these errors do not appear any more. Resolving.