647951 - window.top.XULBrowserWindow is null

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[John P Baker]

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)]

Attachment: proposed patch

Comment 2

[Marco Bonardo [:mak]]

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)]

(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]]

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]]

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)]

(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]]

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)]

(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]]

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)]

(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]]

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)]

(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]]

yes, that's what I mean

Comment 14

[Dão Gottwald [:dao]]

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)]

Attachment: 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]]

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)]

Attachment: patch v3

Fix a comment

Comment 18

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a313f75c607c

Comment 19

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/a313f75c607c