Last Comment Bug 647951 - window.top.XULBrowserWindow is null
: window.top.XULBrowserWindow is null
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: x86 Windows 2000
: -- normal (vote)
: Firefox 14
Assigned To: Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
:
: Marco Bonardo [::mak]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-06 00:34 PDT by John P Baker
Modified: 2012-03-29 08:50 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (669 bytes, patch)
2012-02-17 20:20 PST, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
mak77: feedback+
Details | Diff | Splinter Review
proposed patch v2 (990 bytes, patch)
2012-03-25 02:50 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
mak77: review+
Details | Diff | Splinter Review
patch v3 (969 bytes, patch)
2012-03-27 19:14 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
mak77: review+
Details | Diff | Splinter Review

Description John P Baker 2011-04-06 00:34:43 PDT
This may be the same as bug 450683.

1. View/Sidebar/History
2. Ctrl/N
3. Close this new window

Error: window.top.XULBrowserWindow is null
Source File: chrome://browser/content/bookmarks/sidebarUtils.js
Line: 100

Mozilla/5.0 (Windows NT 5.0; rv:2.2a1pre) Gecko/20110405 Firefox/4.2a1pre
Comment 1 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-02-17 20:20:37 PST
Created attachment 598487 [details] [diff] [review]
proposed patch
Comment 2 Marco Bonardo [::mak] 2012-02-20 08:09:43 PST
Comment on attachment 598487 [details] [diff] [review]
proposed patch

Review of attachment 598487 [details] [diff] [review]:
-----------------------------------------------------------------

before reviewing I think we should figure out when this may happen, even if just to add a comment.

XULBrowserWindow is nullified in BrowserShutdown, so this call must happen later, though I'm not sure why it happens at all since the new window doesn't have a sidebar. It's worth getting the call stack and figuring it out.
Comment 3 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-02-21 04:48:57 PST
(In reply to Marco Bonardo [:mak] from comment #2)
> XULBrowserWindow is nullified in BrowserShutdown, so this call must happen
> later, though I'm not sure why it happens at all since the new window
> doesn't have a sidebar. It's worth getting the call stack and figuring it
> out.

At the STR of this comment 0, Step 2 opens the browser window with opening bookmarks/history sidebar.
So bookmarks/history sidebar are opened when close the browser window which is opened on step 2.
This bug caused by that XULBrowserWindow is nullified before calling SidebarUtils.setMouseoverURL().

Stack trace on the Error:
SU_setMouseoverURL("")@chrome://browser/content/bookmarks/sidebarUtils.js:105
onunload([object Event])@chrome://browser/content/history/history-panel.xul:1
Comment 4 Marco Bonardo [::mak] 2012-02-21 05:04:33 PST
Ah, right, the new window has a sidebar.
So basically the problem is that onunload of the browser window is fired before onunload of the sidebar window.
Comment 5 Marco Bonardo [::mak] 2012-02-21 05:14:06 PST
So after looking briefly at the code, I think we may try to enforce correct ordering of the events here, rather than patch the sidebar code, since other things may break also in add-ons.

What we should do imo, is to add code at the beginning of BrowserShutdown that hides the sidebar if it's visible. There is already similar code for DOM fullscreen that I assume works:

if (!document.getElementById("sidebar-box").hidden)
toggleSidebar();

could you please check if doing so solves the problem?
Above the code there should be a brief comment explaining that it hides the sidebar to ensure its unload event is fired before we proceed with shutting down the browser window.
Comment 6 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-02-21 07:12:38 PST
(In reply to Marco Bonardo [:mak] from comment #5)
> So after looking briefly at the code, I think we may try to enforce correct
> ordering of the events here, rather than patch the sidebar code, since other
> things may break also in add-ons.
> 
> What we should do imo, is to add code at the beginning of BrowserShutdown
> that hides the sidebar if it's visible. There is already similar code for
> DOM fullscreen that I assume works:
> 
> if (!document.getElementById("sidebar-box").hidden)
> toggleSidebar();
> 
> could you please check if doing so solves the problem?
> Above the code there should be a brief comment explaining that it hides the
> sidebar to ensure its unload event is fired before we proceed with shutting
> down the browser window.

I checked the behavior with insert your codes into the beginning of BrowserShutdown.
But this did not solve the problem...
Comment 7 Marco Bonardo [::mak] 2012-02-21 07:20:57 PST
So the sidebar document is not unloaded when closing the sidebar, sounds like bug 728426, something is keeping it alive.
Comment 8 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-02-21 07:29:43 PST
(In reply to Marco Bonardo [:mak] from comment #7)
> So the sidebar document is not unloaded when closing the sidebar, sounds
> like bug 728426, something is keeping it alive.

I checked with debugger, SidebarUtils.setMouseoverURL() is called when closing only the sidebar without closing a browser window.
It seems that unload event are fired.
Comment 9 Marco Bonardo [::mak] 2012-02-21 08:32:33 PST
yes, just that from my poor-man's testing XULBrowserWindow is still destroyed before the unload even hits the sidebar.
This seems to work:
  if (!document.getElementById("sidebar-box").hidden) {
    let sidebar = document.getElementById("sidebar");
    sidebar.contentWindow.document.close();
  }
Though I'd rather ask Gavin what he thinks about the idea of enforcing sidebar unload before starting browser shutdown.
Comment 10 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-02-21 08:45:17 PST
(In reply to Marco Bonardo [:mak] from comment #9)
>   if (!document.getElementById("sidebar-box").hidden) {
>     let sidebar = document.getElementById("sidebar");
>     sidebar.contentWindow.document.close();
>   }

This is not working we hoped:
Error: sidebar.contentWindow.document.close is not a function

> Though I'd rather ask Gavin what he thinks about the idea of enforcing
> sidebar unload before starting browser shutdown.
I agree your suggestion.
Comment 11 Marco Bonardo [::mak] 2012-02-21 08:48:45 PST
Comment on attachment 598487 [details] [diff] [review]
proposed patch

after discussing with Gavin, the added complexity is not worth the case of add-ons unload being broken by wrong ordering, so just bailing out here will be fine.

But please, add a comment above the if explaining that when the window is closed the sidebar unload event may happen after the browser window's unload event.
Also just use window.top.XULBrowserWindow, no need for an alias imo.
Comment 12 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-02-21 09:06:12 PST
(In reply to Marco Bonardo [:mak] from comment #11)
> But please, add a comment above the if explaining that when the window is
> closed the sidebar unload event may happen after the browser window's unload
> event.

I check the timing of unload event are fired when closing a browser window with opening a sidebar. the following sequence:

1. Fire the unload event of browser window. (call BrowserShutdown)
2. Fire the unload event of sidebar window. (call SidebarUtils.setMouseoverURL)
Comment 13 Marco Bonardo [::mak] 2012-02-21 09:14:52 PST
yes, that's what I mean
Comment 14 Dão Gottwald [:dao] 2012-02-21 09:45:31 PST
Comment on attachment 598487 [details] [diff] [review]
proposed patch

You can write top.XULBrowserWindow instead of window.top.XULBrowserWindow.
Comment 15 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-03-25 02:50:40 PDT
Created attachment 609095 [details] [diff] [review]
proposed patch v2

*Add the comment describing the sequence of unload event if closing the browser window with sidebar window.
*Use top.XULBrowserWindow instead of window.top.XULBrowserWindow.
Comment 16 Marco Bonardo [::mak] 2012-03-27 16:24:23 PDT
Comment on attachment 609095 [details] [diff] [review]
proposed patch v2

Review of attachment 609095 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you, this looks good, just a minor nit on the comment.

::: browser/components/places/content/sidebarUtils.js
@@ +137,5 @@
>  
>    setMouseoverURL: function SU_setMouseoverURL(aURL) {
> +    // The sidebar unload event happens after the browser window's unload event
> +    // if closing the browser window with sidebar window.
> +    // In this case, top.XULBrowserWindow will be null when calling this method.

I'd say:
// When the browser window is closed with an open sidebar, the sidebar
// unload event happens after the browser's one.  In this case
// top.XULBrowserWindow has been nullified already.
Comment 17 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-03-27 19:14:28 PDT
Created attachment 609978 [details] [diff] [review]
patch v3

Fix a comment
Comment 19 Marco Bonardo [::mak] 2012-03-29 08:50:47 PDT
https://hg.mozilla.org/mozilla-central/rev/a313f75c607c

Note You need to log in before you can comment on or make changes to this bug.