Bypass update of Viewport metadata when no documentElement exists

RESOLVED FIXED in Firefox 27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: capella, Assigned: capella)

Tracking

unspecified
Firefox 27
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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
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");
Flags: needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugmail.mozilla)
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!
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}]
As is ... fixes error generated in browser.js
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #818150 - Flags: review?(bugmail.mozilla)
Attachment #818150 - Flags: review?(bugmail.mozilla) → review+
Oh, please update the commit message to be more meaningful.
Summary: interesting JavaScript Error noticed in browser.js during restart recovery involving readermode → Bypass update of Viewport metadata when no documentElement exists
https://hg.mozilla.org/mozilla-central/rev/517ad8dc7f51
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.