window.top.XULBrowserWindow is null

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: John P Baker, Assigned: tetsuharu)

Tracking

Trunk
Firefox 14
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
Created attachment 598487 [details] [diff] [review]
proposed patch
Attachment #598487 - Flags: review?(mak77)
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.

Updated

6 years ago
Assignee: nobody → saneyuki.s.snyk
Status: NEW → ASSIGNED
(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
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.
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.
(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...
So the sidebar document is not unloaded when closing the sidebar, sounds like bug 728426, something is keeping it alive.
(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.
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.
(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 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.
Attachment #598487 - Flags: review?(mak77) → feedback+
(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)
yes, that's what I mean
Comment on attachment 598487 [details] [diff] [review]
proposed patch

You can write top.XULBrowserWindow instead of window.top.XULBrowserWindow.
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.
Attachment #598487 - Attachment is obsolete: true
Attachment #609095 - Flags: review?(mak77)
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.
Attachment #609095 - Flags: review?(mak77) → review+
Created attachment 609978 [details] [diff] [review]
patch v3

Fix a comment
Attachment #609095 - Attachment is obsolete: true
Attachment #609978 - Flags: review?(mak77)

Updated

5 years ago
Attachment #609978 - Flags: review?(mak77) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/a313f75c607c
Keywords: checkin-needed
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/a313f75c607c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.