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

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

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

Comment 1

[Mark Capella [:capella]]

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.

Comment 2

[Mark Capella [:capella]]

Attachment: Relevent Logcat with current issues

Comment 3

[Mark Capella [:capella]]

Attachment: bug925983 (v0)

Comment 4

[Mark Capella [:capella]]

Attachment: Relevent Logcat with current issues patched

Comment 5

[Lucas Rocha (:lucasr)]

mfinkle, any clues on why we're getting double DOMContentLoaded events here?

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 7

[Lucas Rocha (:lucasr)]

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).

Comment 8

[Mark Capella [:capella]]

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

Comment 9

[Mark Capella [:capella]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1e94697a744