Closed Bug 925983 Opened 6 years ago Closed 6 years ago

Misc javascript error in aboutReader on restart with multiple tabs in readermode

Categories

(Firefox for Android :: Reader View, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27
Tracking Status
fennec - ---

People

(Reporter: capella, Assigned: capella)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=js])

Attachments

(3 files)

1) Start FF, open several articles in seperate tabs in readermode
2) Force close FF through android
3) Re-start FF with Settings (Tabs == Always Restore) or
      manually by About:Home -> History -> Open all tabs from last time

Observe one error message per re-openned tab:

E/GeckoConsole( 5338): [JavaScript Error: "TypeError: this._article is null" {file: "chrome://browser/content/aboutReader.js" line: 508}]

Currently pointing here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#508
Blocks: readerv2
tracking-fennec: --- → ?
It looks like prior to the above observation, if we have multiple tabs open in Readermode, and FF is halted/swiped closed then restarted, that we receive two "DOMContentLoaded" messages that try to instantiate a new AboutReader() object for the same page...
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3394

However, the first one is received when there is no valid document.body and causes aboutReader.js to complain in interesting ways starting here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#54

(see the logs I'll post detailing events before / after applying my patch)

It looks like a safe assumuption, (and some brief testing bears this out), is that the first object instantiation should simply be bypassed as invalid.

Interestingly, though maybe not entirely relevent, is the case where we have only a single tab in Readermode, and FF is halted/swiped closed then restarted... in that case we still receive two "DOMContentLoaded" messages that try to instantiate a new AboutReader() object for the same page... and as they are both valid requests we create a first then a subsequent second one.

Also, it looks like browser.js - Tab.handleEvent("DOMContentLoaded") passes those multiple "DOMContentLoaded" messages downstream to Java ... something at least to be aware of.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch bug925983 (v0)Splinter Review
Attachment #818307 - Flags: review?(lucasr.at.mozilla)
mfinkle, any clues on why we're getting double DOMContentLoaded events here?
Flags: needinfo?(mark.finkle)
(In reply to Lucas Rocha (:lucasr) from comment #5)
> mfinkle, any clues on why we're getting double DOMContentLoaded events here?

My first thought was an iframe, but we don't seem to use iframes in aboutReader.html, which is good. I remembered that we used to use iframes but stopped doing that.

We also protect the "DOMContentLoaded" listener from executing for non-top-level windows too. That would also stop iframe issues.

I am left wondering if this has something to do with the About page redirection:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/AboutRedirector.js

but I don't have any clues yet as to why it could be the problem.

I notice that aboutReader.html uses ... HTML, while the rest of our about pages use XHTML, but I have no idea how that difference could affect the parsing and/or event firing.
Flags: needinfo?(mark.finkle)
Comment on attachment 818307 [details] [diff] [review]
bug925983 (v0)

Review of attachment 818307 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, please add a comment explaining why the check is needed.

::: mobile/android/chrome/content/browser.js
@@ +3391,5 @@
>          }
>  
> +        if (docURI.startsWith("about:reader")) {
> +          let contentDocument = this.browser.contentDocument;
> +          if (contentDocument.body) {

This probably deserves a thorough comment explaining why this check is needed (especially considering we don't really know yet what is causing it).
Attachment #818307 - Flags: review?(lucasr.at.mozilla) → review+
tracking-fennec: ? → -
https://hg.mozilla.org/mozilla-central/rev/a1e94697a744
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.