1129984 - ReaderParent.jsm "X is null" failures in mochitest runs

Status: RESOLVED FIXED

(Firefox :: General, defect, P3)

Description

[Tim Taubert [:ttaubert] (inactive)]

05:38:40     INFO -  503 INFO TEST-START | browser/base/content/test/general/browser_customize_popupNotification.js
05:38:41     INFO -  JavaScript error: resource://app/modules/ReaderParent.jsm, line 105: TypeError: button is null

05:40:48     INFO -  750 INFO TEST-START | browser/base/content/test/social/browser_social_errorPage.js
05:40:50     INFO -  JavaScript error: resource://app/modules/ReaderParent.jsm, line 96: TypeError: win.gBrowser is null
05:40:50     INFO -  JavaScript error: resource://app/modules/ReaderParent.jsm, line 96: TypeError: win.gBrowser is null
05:40:50     INFO -  JavaScript error: resource://app/modules/ReaderParent.jsm, line 96: TypeError: win.gBrowser is null
05:40:50     INFO -  JavaScript error: resource://app/modules/ReaderParent.jsm, line 96: TypeError: win.gBrowser is null

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Those excerpts are from:

https://treeherder.mozilla.org/logviewer.html#?repo=b2g-inbound&job_id=1294706

Comment 2

[:Margaret Leibovic]

I would think we shouldn't hit this because reader mode is disabled by default right now, but it looks like this call to updateReaderButton doesn't account for that pref:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4368

However, it seems weird that win.gBrowser would be null here, if we're using gBrowser.selectedBrowser to pass in a browser:
http://mxr.mozilla.org/mozilla-central/source/browser/modules/ReaderParent.jsm#96

I could just add a check to make sure win.gBrowser isn't null, but I want to make sure that's not covering up some other problem.

Comment 3

[Justin Dolske [:Dolske]]

P3 on fear that this could be an actual bug breaking the feature for actual users.

Comment 4

[Justin Dolske [:Dolske]]

...but could be P2 if this is happening frequently for real people! It's now enabled so we'll find out soon!

Comment 5

[:Margaret Leibovic]

(In reply to Tim Taubert [:ttaubert] (on PTO, back April 14th) from comment #0)
> 05:38:40     INFO -  503 INFO TEST-START |
> browser/base/content/test/general/browser_customize_popupNotification.js
> 05:38:41     INFO -  JavaScript error:
> resource://app/modules/ReaderParent.jsm, line 105: TypeError: button is null

I don't see this test in the tree anymore, so I couldn't reproduce this issue.

> 05:40:48     INFO -  750 INFO TEST-START |
> browser/base/content/test/social/browser_social_errorPage.js
> 05:40:50     INFO -  JavaScript error:
> resource://app/modules/ReaderParent.jsm, line 96: TypeError: win.gBrowser is
> null
> 05:40:50     INFO -  JavaScript error:
> resource://app/modules/ReaderParent.jsm, line 96: TypeError: win.gBrowser is
> null
> 05:40:50     INFO -  JavaScript error:
> resource://app/modules/ReaderParent.jsm, line 96: TypeError: win.gBrowser is
> null
> 05:40:50     INFO -  JavaScript error:
> resource://app/modules/ReaderParent.jsm, line 96: TypeError: win.gBrowser is
> null

But I did reproduce this issue:

47 INFO Console message: [JavaScript Error: "TypeError: win.gBrowser is null" {file: "resource:///modules/ReaderParent.jsm" line: 137}]

I suspect this has something to do with how the social API works, and that we can't make assumptions about win.gBrowser being there. So I think a null check would be safe here.

Comment 6

[:Margaret Leibovic]

After some discussion on IRC, it seems like the real issue here is that content.js is running in frames that aren't tabs in tabbrowser, but we're making that assumption in content.js. This seems like a problem that might affect more than just about:reader, and I don't know that hiding this error with a null check is the right call.

Comment 7

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #6)
> After some discussion on IRC, it seems like the real issue here is that
> content.js is running in frames that aren't tabs in tabbrowser, but we're
> making that assumption in content.js. This seems like a problem that might
> affect more than just about:reader, and I don't know that hiding this error
> with a null check is the right call.

Mossop filed bug 1153016 about this.

Comment 8

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #7)
> (In reply to :Margaret Leibovic from comment #6)
> > After some discussion on IRC, it seems like the real issue here is that
> > content.js is running in frames that aren't tabs in tabbrowser, but we're
> > making that assumption in content.js. This seems like a problem that might
> > affect more than just about:reader, and I don't know that hiding this error
> > with a null check is the right call.
> 
> Mossop filed bug 1153016 about this.

This was indeed fixed by bug 1153016, so I'm going to close this out.

Good find, ttaubert!