As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact
Last Comment Bug 586360 - Add-ons manager doesn't open if all windows are closed
: Add-ons manager doesn't open if all windows are closed
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: seamonkey2.1b1
Assigned To: Justin Wood (:Callek)
Depends on:
Blocks: 566593
  Show dependency treegraph
Reported: 2010-08-11 11:46 PDT by Stefan [:stefanh]
Modified: 2010-09-11 20:59 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

potential fix (563 bytes, patch)
2010-09-01 22:13 PDT, Justin Wood (:Callek)
no flags Details | Diff | Splinter Review
Real Fix (659 bytes, patch)
2010-09-04 20:26 PDT, Justin Wood (:Callek)
neil: review+
stefanh: feedback+
Details | Diff | Splinter Review

Description User image Stefan [:stefanh] 2010-08-11 11:46:32 PDT

1) Use a Mac, and launch SeaMonkey
2) Close all windows
3) From the menubar, select Tools --> Add-on Manager

Expected results: Probably a new browser window with Add-on manager loaded in it
Actual results: Nothing, and an error message saying:
"Error: aWindow.gBrowser is null
Source File: chrome://communicator/content/utilityOverlay.js
Line: 1339"

(We don't have a browser here)

I'm assigning it to Callek, since he thought he could look at it
Comment 1 User image Stefan [:stefanh] 2010-08-11 11:48:15 PDT
> I'm assigning it to Callek, since he thought he could look at it

Right, I didn't. But I'll CC him now ;-)
Comment 2 User image Justin Wood (:Callek) 2010-09-01 22:10:50 PDT
Ok, here's what is going on:

navigator.js sets gBrowser to null in global scope.

Startup() sets it to a real browser, which is called from onload in navigator.xul

However this doesn't happen in hiddenwindow.xul since its onload calls hiddenWindowStartup.  And surely shouldn't set gBrowser.

So when we test |if (!("gBrowser" in aWindow))| for the early return, we succeed since gBrowser is defined in scope, though its null (never was initialized)

I'll attach a patch in a moment, though since I'm not on mac I can't verify it works there.

Gavin, I presume this is a problem in Firefox (at least for _this_ use) as well right?  If so, feel free to have me piggyback on this bug for Firefox, or if you file a new one, you can assign to me and I'll patch for you.
Comment 3 User image Justin Wood (:Callek) 2010-09-01 22:13:25 PDT
Created attachment 471409 [details] [diff] [review]
potential fix
Comment 4 User image Justin Wood (:Callek) 2010-09-01 23:41:53 PDT
Comment on attachment 471409 [details] [diff] [review]
potential fix

err I typo'd here.... :/ should be 

-    if (!("gBrowser" in aWindow))
+    if (!("gBrowser" in aWindow) || !(aWindow.gBrowser))

but I prefer the first, will update patch tomorrow, neil if you want to review contingent on this update, great.
Comment 5 User image 2010-09-02 03:08:46 PDT
(In reply to comment #4)
> +    if (!("gBrowser" in aWindow) || !(aWindow.gBrowser))
We probably only need the !aWindow.gBrowser test.

There are other ways to do this test; instead of prioritising the current window we can prioritise the most recent browser window (getTopWin()) or we can compare the current window to the the most recent browser window.

For example:

if (window == getTopWin() && switchIfURIInWindow(window))
  return true;

var w = getTopWin();
if (w) {
  // Prioritise this window.
  if (switchIfURIInWindow(w))
    return true;

  let winEnum = Services.wm.getEnumerator("navigator:browser");
  // ...
    if (browserWin.closed || browserWin == w)
  // etc.
Comment 6 User image Stefan [:stefanh] 2010-09-04 16:31:01 PDT
Comment on attachment 471409 [details] [diff] [review]
potential fix

Cancelling request, since Justin told me there's a new patch on the way.
Comment 7 User image Justin Wood (:Callek) 2010-09-04 20:26:53 PDT
Created attachment 472226 [details] [diff] [review]
Real Fix

Ok, this simplifies the test, I did not do any of the larger sweeping changes, as I still think this is the better design.
Comment 8 User image Stefan [:stefanh] 2010-09-05 06:43:52 PDT
Comment on attachment 472226 [details] [diff] [review]
Real Fix

Works fine - thanks.
Comment 9 User image 2010-09-06 16:08:38 PDT
Comment on attachment 472226 [details] [diff] [review]
Real Fix

This will do until I manage to demo why getTopWin() is better.
Comment 10 User image 2010-09-06 16:13:23 PDT
Actually it's easier than I thought:
1. Start SeaMonkey
2. Open about:addons manually
3. Open a new window
4. Open about:addons manually again
5. Open the Error Console
6. Tools - Add-on Manager
Note that the first browser window is raised, rather than the top browser.
Comment 11 User image Justin Wood (:Callek) 2010-09-07 08:09:18 PDT

Neil, if you want to do a different approach here, willing to file the bug with the patch, I'll review it :-)
Comment 12 User image :Gavin Sharp [email:] 2010-09-11 20:59:49 PDT
You can have r+a=me if you want to apply the same patch to Firefox.

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