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

RESOLVED FIXED

Status

()

P3
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ttaubert, Assigned: Margaret)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

4 years ago
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)

Updated

4 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 2

4 years ago
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!
(Assignee)

Comment 5

4 years ago
(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.
(Assignee)

Comment 6

4 years ago
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.
(Assignee)

Comment 7

4 years ago
(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
(Assignee)

Comment 8

4 years ago
(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
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.