Closed Bug 1129984 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: General, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: ttaubert, Assigned: Margaret)

References

Details

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
Assignee: nobody → 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.
P3 on fear that this could be an actual bug breaking the feature for actual users.
Priority: -- → P3
...but could be P2 if this is happening frequently for real people! It's now enabled so we'll find out soon!
(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.
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.
(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.
Depends on: 1153016
(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!
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.