926015 - Bypass update of Viewport metadata when no documentElement exists

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Mark Capella [:capella]]

Open FF
Ensure Settings -> Customize -> Tabs is set to "Always restore"
Navigate to a page such as arstechnica.com
Navigate to any article
Click reader icon to enter reading mode
Wait for page to load then use Android to Force Stop the browser
Restart the browser and watch the log

Observe the following messages:

Error: "TypeError: aWindow.document.documentElement is null" {file: "chrome://browser/content/browser.js" line: 5711}]
Error: "TypeError: body is null" {file: "chrome://browser/content/aboutReader.js" line: 53}]
Error: "TypeError: this._doc.getElementById(...) is null" {file: "chrome://browser/content/aboutReader.js" line: 275}]
Error: "TypeError: this._article is null" {file: "chrome://browser/content/aboutReader.js" line: 508}]

Initially triggered from here in browser.js it seems to spill down into aboutReader.js:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#5736

Comment 1

[Mark Capella [:capella]]

In bug 807447 a call was added here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4021

It seems that when we observe("before-first-paint") while reloading multiple readerMode tabs @ FF restart, we haven't had time to load the tab 2 for example while tab 3 is loading in the foreground ...

So at start of tab 2 we call updateMetadata() which calls getViewportMetadata() and we hit a javascript null error here http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#5740 ... since the error message is correct and aWindow.document.documentElement is null ...

I wonder if we can safely default the RTL decision to something like the following to safely complete, or if there are other nefarious considerations I'm not aware of here... maybe simply avoid the call to updateMetadata() entirely?

let isRTL = (!aWindow.document.documentElement || aWindow.document.documentElement.dir == "rtl");

Comment 2

[Kartikaya Gupta (email:[e-mail])]

It doesn't look like the error in browser.js is "spilling over" into aboutReader.js. Rather, I think all the pieces of code make the same underlying assumption, that they are being invoked at a point where the document DOM has been populated. In updateViewportMetadata function you're welcome to add the check from comment 1 into the isRTL definition but I don't think it'll fix the other errors noted in comment 0.

We could also skip the updateMetadata call entirely, because if the document.documentElement doesn't exist then there's no meta viewport tag either so running updateMetadata is pointless. I'm a little surprised that before-first-paint is triggering at all though, because layout has nothing to paint if there's no documentElement in the DOM.

Comment 3

[Mark Capella [:capella]]

Ok, so I'll plan to skip the call during entirely ...

The other messages that follow in aboutReader do still occur (I tested this quickly) and need to be addressed next ...

I'd already filed a seperate bug Bug 925983 - Misc javascript error in aboutReader on restart with multiple tabs in readermode that looks to cover that :-)

Thanks for the comments!

Comment 4

[Mark Capella [:capella]]

Fyi the following ... patch to follow ... three tabs, 2d and 3d in reader mode

**********************   Quick trace of the event sequences ... BEFORE the patch

- Tab - create(http://www.businessinsider.com/) STARTS   ==========
- Tab - create(http://www.businessinsider.com/) FINISHES ==========
- Tab - create(about:reader?url=http%3A%2F%2Fwww.businessinsider.com%2Fmutual-funds-vs-sp-500-2013-10&tabId=4) STARTS   ==========
- Tab - create(about:reader?url=http%3A%2F%2Fwww.businessinsider.com%2Fmutual-funds-vs-sp-500-2013-10&tabId=4) FINISHES ==========
- Tab - create(about:reader?url=http%3A%2F%2Fwww.businessinsider.com%2Fdebt-ceiling-deal-reid-mcconnell-house-vote-boehner-government-shutdown-2013-10&tabId=5) STARTS   ==========
- Tab - create(about:reader?url=http%3A%2F%2Fwww.businessinsider.com%2Fdebt-ceiling-deal-reid-mcconnell-house-vote-boehner-government-shutdown-2013-10&tabId=5) FINISHES ==========

<---- Where it goes bad ---->
- Tab - observe(before-first-paint) Hears a call
- Tab - observe(before-first-paint) Calling ViewportHandler.updateMetadata(this, true)
- ViewportHandler - updateMetadata(2) Hears a call
- ViewportHandler - updateMetadata() I SHOULD BE BYPASSING ViewportHandler.getViewportMetadata([object XrayWrapper [object Window]])
- getViewportMetadata() Hears a call - aWindow[[object XrayWrapper [object Window]]]
- getViewportMetadata() Here before crash suspect - aWindow.document.documentElement[null]
Error: "TypeError: aWindow.document.documentElement is null" {file: "chrome://browser/content/browser.js" line: 5736}]

- Tab - observe(before-first-paint) Hears a call
- Tab - observe(before-first-paint) Hears a call
Error: "TypeError: body is null" {file: "chrome://browser/content/aboutReader.js" line: 54}]
- ViewportHandler - handleEvent(DOMMetaAdded) Calling ViewportHandler.updateMetadata(tab, false)
- ViewportHandler - updateMetadata(2) Hears a call
- ViewportHandler - updateMetadata() Calling ViewportHandler.getViewportMetadata([object XrayWrapper [object Window]])
- getViewportMetadata() Hears a call - aWindow[[object XrayWrapper [object Window]]]
- getViewportMetadata() Here before crash suspect - aWindow.document.documentElement[[object XrayWrapper [object HTMLHtmlElement]]]
- getViewportMetadata() Here after  crash suspect - aWindow.document.documentElement[[object XrayWrapper [object HTMLHtmlElement]]]

- Tab - observe(before-first-paint) Hears a call
- Tab - observe(before-first-paint) Calling ViewportHandler.updateMetadata(this, true)
- ViewportHandler - updateMetadata(2) Hears a call
- ViewportHandler - updateMetadata() Calling ViewportHandler.getViewportMetadata([object XrayWrapper [object Window]])
- getViewportMetadata() Hears a call - aWindow[[object XrayWrapper [object Window]]]
- getViewportMetadata() Here before crash suspect - aWindow.document.documentElement[[object XrayWrapper [object HTMLHtmlElement]]]
- getViewportMetadata() Here after  crash suspect - aWindow.document.documentElement[[object XrayWrapper [object HTMLHtmlElement]]]

- Tab - observe(before-first-paint) Hears a call
- Tab - observe(before-first-paint) Hears a call

Error: "TypeError: this._doc.getElementById(...) is null" {file: "chrome://browser/content/aboutReader.js" line: 306}]
Error: "TypeError: this._article is null" {file: "chrome://browser/content/aboutReader.js" line: 238}]
Error: "TypeError: this._article is null" {file: "chrome://browser/content/aboutReader.js" line: 550}]


**********************   Quick trace of the event sequences ... AFTER the patch

- Tab - create(http://www.businessinsider.com/) STARTS   ==========
- Tab - create(http://www.businessinsider.com/) FINISHES ==========
- Tab - create(about:reader?url=http%3A%2F%2Fwww.businessinsider.com%2Fmutual-funds-vs-sp-500-2013-10&tabId=4) STARTS   ==========
- Tab - create(about:reader?url=http%3A%2F%2Fwww.businessinsider.com%2Fmutual-funds-vs-sp-500-2013-10&tabId=4) FINISHES ==========
- Tab - create(about:reader?url=http%3A%2F%2Fwww.businessinsider.com%2Fdebt-ceiling-deal-reid-mcconnell-house-vote-boehner-government-shutdown-2013-10&tabId=5) STARTS   ==========
- Tab - create(about:reader?url=http%3A%2F%2Fwww.businessinsider.com%2Fdebt-ceiling-deal-reid-mcconnell-house-vote-boehner-government-shutdown-2013-10&tabId=5) FINISHES ==========

<---- Where it USED TO bad ---->
- Tab - observe(before-first-paint) Hears a call
- Tab - observe(before-first-paint) Calling ViewportHandler.updateMetadata(this, true)
- ViewportHandler - updateMetadata(2) Hears a call
- ViewportHandler - updateMetadata() I AM BYPASSING ViewportHandler.getViewportMetadata([object XrayWrapper [object Window]])

- Tab - observe(before-first-paint) Hears a call
- Tab - observe(before-first-paint) Hears a call
Error: "TypeError: body is null" {file: "chrome://browser/content/aboutReader.js" line: 54}]
- ViewportHandler - handleEvent(DOMMetaAdded) Calling ViewportHandler.updateMetadata(tab, false)
- ViewportHandler - updateMetadata(2) Hears a call
- ViewportHandler - updateMetadata() Calling ViewportHandler.getViewportMetadata([object XrayWrapper [object Window]])
- getViewportMetadata() Hears a call - aWindow[[object XrayWrapper [object Window]]]
- getViewportMetadata() Here before crash suspect - aWindow.document.documentElement[[object XrayWrapper [object HTMLHtmlElement]]]
- getViewportMetadata() Here after  crash suspect - aWindow.document.documentElement[[object XrayWrapper [object HTMLHtmlElement]]]

- Tab - observe(before-first-paint) Hears a call
- Tab - observe(before-first-paint) Calling ViewportHandler.updateMetadata(this, true)
- ViewportHandler - updateMetadata(2) Hears a call
- ViewportHandler - updateMetadata() Calling ViewportHandler.getViewportMetadata([object XrayWrapper [object Window]])
- getViewportMetadata() Hears a call - aWindow[[object XrayWrapper [object Window]]]
- getViewportMetadata() Here before crash suspect - aWindow.document.documentElement[[object XrayWrapper [object HTMLHtmlElement]]]
- getViewportMetadata() Here after  crash suspect - aWindow.document.documentElement[[object XrayWrapper [object HTMLHtmlElement]]]

- Tab - observe(before-first-paint) Hears a call
- Tab - observe(before-first-paint) Hears a call

Error: "TypeError: this._doc.getElementById(...) is null" {file: "chrome://browser/content/aboutReader.js" line: 306}]
Error: "TypeError: this._article is null" {file: "chrome://browser/content/aboutReader.js" line: 238}]
Error: "TypeError: this._article is null" {file: "chrome://browser/content/aboutReader.js" line: 550}]

Comment 5

[Mark Capella [:capella]]

Attachment: bug926015 (v0)

As is ... fixes error generated in browser.js

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Oh, please update the commit message to be more meaningful.

Comment 7

[Mark Capella [:capella]]

Ok on TRY https://tbpl.mozilla.org/?tree=Try&rev=dc71381b98cc

Comment 8

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/fx-team/rev/517ad8dc7f51

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/517ad8dc7f51