Closed Bug 1160162 Opened 9 years ago Closed 8 years ago

JavaScript error: 'TypeError: win.gBrowser is undefined' when entering customization mode

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox40 --- affected

People

(Reporter: florian, Unassigned)

References

Details

(Whiteboard: [fixed by bug 1014185])

Attachments

(1 file)

JavaScript error: resource:///modules/ReaderParent.jsm, line 159: TypeError: win.gBrowser is undefined
This sounds similar to bug 1129984, although that was fixed by bug 1153016.

I'm not familiar with how customization mode works, but if it's expected that there isn't a gBrowser, we should probably just add a null/undefined check in updateReaderButton.
I thought maybe this was because a binding was being changed on the #main-window but looking through the CSS I see nothing that does this. I'm attaching the simple fix but it would be nice to know why.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment on attachment 8711358 [details]
MozReview Request: Bug 1160162 - JavaScript error: 'TypeError: win.gBrowser is undefined' when entering customization mode. r?dao

Where is this function called from when entering customization mode? What's win in that case?
Attachment #8711358 - Flags: review?(dao)
(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 8711358 [details]
> MozReview Request: Bug 1160162 - JavaScript error: 'TypeError: win.gBrowser
> is undefined' when entering customization mode. r?dao
> 
> Where is this function called from when entering customization mode? What's
> win in that case?

This only reproduces when the page that is opened is about:home. The sender of the message is the "pagehide" event listener in tab-content.js. In ReaderParent.jsm, win is a ChromeWindow that the debugger fails to expand in the Variables View.

When the selected tab is about:newtab, win.gBrowser is defined.
Correction, the debugger *is* able to expand `win` in the first case, there is just a noticeable delay.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Correction, the debugger *is* able to expand `win` in the first case

So what is it?
I'm really not sure what type of answer you're looking for. Here's a screenshot of the debugger, http://screencast.com/t/LVHyzQwoYsn
I mean why is this called with a window that doesn't have gBrowser?
(In reply to :Gijs Kruitbosch from bug 1179739 comment #1)
> In an epic 2-years-later comeback to this comment:
> 
> (In reply to Tim Taubert [:ttaubert] from bug 1018913 comment #22)
> > Always me :'(
> 
> Yes, yes, always you. Sorry! :-(
> 
> Because!
> 
> This is still happening if you use the context menu of the navbar repeatedly
> to open (and then you close) customize mode. It seems that the code here:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 584870f1cbc5d060a57e147ce249f736956e2b62/browser/modules/ReaderParent.
> jsm#106-110
> 
> gets a browser which doesn't live in a window which is a browser.xul window.
> 
> This in turn turns out to happen because in the customization mode
> preloader, we create a highly magical <browser> inside some data: uri window
> thing that we can later swap into the tabbrowser when needed, via
> HiddenFrame.jsm. The issue being that we load all these frame scripts into
> said browser, and some of them do kind of assume that they are being used in
> 'tab-content' because the file is called tab-content.js .
> 
> The trivial fix here is to nullcheck win.gBrowser.
> 
> The less trivial fix is to critically examine whether we really need those
> frame scripts here, and why.
> 
> The even less trivial fix is to think about what the distinction between
> 'content.js' and 'tab-content.js' really is, if it makes sense, how, and
> audit everything in it (and the places where we listen to messages sent from
> there) to check whether they cope with special snowflakes such as
> HiddenFrame.jsm.
> 
> 
> Thoughts?

(In reply to Tim Taubert [:ttaubert] from bug 1179739 comment #4)
> I don't think there's a really easy fix here, the basic model of preloading
> & swapping docShells isn't great. I think what we changed with bug 1077652
> is a little better, maybe you should do the same for the customization tab?
> 
> When ensurePreload() is called (given a window) you could insert a
> <xul:browser> just like _createPreloadBrowser() does. When the user enters
> customization mode you would then just pick it up in addTab(). The browser
> doesn't have a linked tab, but it's almost the real thing.

So the suggested thing here is to insert a 'real' <xul:browser> into the stack of <browser>s the tabbrowser keeps. There might be a perf gain for about:customizing in that, too.

That will avoid stuff in tab-content.js making assumptions that the browser in question lives in browser.xul .
(In reply to :Gijs Kruitbosch from comment #12)
> So the suggested thing here is to insert a 'real' <xul:browser> into the
> stack of <browser>s the tabbrowser keeps. There might be a perf gain for
> about:customizing in that, too.

A more radical and I think ultimately better alternative would be bug 1014185. As I indicated in bug 1014185 comment 3, I would go as far as removing the about:customizing dummy page entirely.
(In reply to Dão Gottwald [:dao] from comment #13)
> (In reply to :Gijs Kruitbosch from comment #12)
> > So the suggested thing here is to insert a 'real' <xul:browser> into the
> > stack of <browser>s the tabbrowser keeps. There might be a perf gain for
> > about:customizing in that, too.
> 
> A more radical and I think ultimately better alternative would be bug
> 1014185. As I indicated in bug 1014185 comment 3, I would go as far as
> removing the about:customizing dummy page entirely.

Seems to me that removing the preloading hack (which I would support assuming it doesn't regress performance) and removing about:customizing are two separate things, and the latter is much more complicated.

Doing the latter would require some "interesting" work to make sure things work, as the underlying selected browser would still have loaded about:blank. How would code distinguish that that page isn't a regular about:blank page into which they could load stuff (we reuse about:blank and about:newtab for other things like the prefs in certain circumstances, right?) ? How would you protect against web pages doing stuff to that about:blank document if they have a handle to the window? (So you could end up with e.g.:

1) website does: var w = window.open('<wherever>', '_blank'); //, opening a new tab
2) <wherever> convinces/spoofs the user to open customize mode in that tab
3) w.location.href = "about:blank" // doesn't trigger any/much chrome level listeners because it's the same URI as before
4) // web page can now manipulate the thing that's loaded while the user is customizing
);


I don't think we should tackle that in this bug. Removing the preloading hack could work, but I would expect CART regressions.
(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Dão Gottwald [:dao] from comment #13)
> > A more radical and I think ultimately better alternative would be bug
> > 1014185. As I indicated in bug 1014185 comment 3, I would go as far as
> > removing the about:customizing dummy page entirely.
> 
> Seems to me that removing the preloading hack (which I would support
> assuming it doesn't regress performance) and removing about:customizing are
> two separate things, and the latter is much more complicated.
> 
> Doing the latter would require some "interesting" work to make sure things
> work, as the underlying selected browser would still have loaded
> about:blank. How would code distinguish that that page isn't a regular
> about:blank page into which they could load stuff (we reuse about:blank and
> about:newtab for other things like the prefs in certain circumstances,
> right?) ? How would you protect against web pages doing stuff to that
> about:blank document if they have a handle to the window? (So you could end
> up with e.g.:
> 
> 1) website does: var w = window.open('<wherever>', '_blank'); //, opening a
> new tab
> 2) <wherever> convinces/spoofs the user to open customize mode in that tab
[snip]

This step would open a new tab, like today. No web content would have access to that tab, like today. All we'd need to do is track internally that that particular tab represents customization mode.
Gijs, would you like to take over this bug?
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> Gijs, would you like to take over this bug?

Not really. I don't think the error warrants the degree of work suggested in comment 13 / comment 15, and I'm 90% sure that removing the customization preloading will just regress CART. I don't have cycles right now to deal with all of that.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> > Gijs, would you like to take over this bug?
> 
> Not really. I don't think the error warrants the degree of work suggested in
> comment 13 / comment 15, and I'm 90% sure that removing the customization
> preloading will just regress CART. I don't have cycles right now to deal
> with all of that.

I understand. Would you be OK with granting r+ to the patch I attached to this bug provided that I include a description as to why this can happen?
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8711358 [details]
MozReview Request: Bug 1160162 - JavaScript error: 'TypeError: win.gBrowser is undefined' when entering customization mode. r?dao

r=me with an explanation and keeping this bug open to fix it 'properly'.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8711358 - Flags: review+
Depends on: 1014185
I can't reproduce this on 2/22/2016 Nightly. Marking fixed by bug 1014185.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed by bug 1014185]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: