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
Status: RESOLVED FIXED
:
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)
:
Mentors:
Depends on:
Blocks: 566593
  Show dependency treegraph
 
Reported: 2010-08-11 11:46 PDT by Stefan [:stefanh] (away until May 28)
Modified: 2010-09-11 20:59 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


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

Description Stefan [:stefanh] (away until May 28) 2010-08-11 11:46:32 PDT
STR:

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 Stefan [:stefanh] (away until May 28) 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 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 Justin Wood (:Callek) 2010-09-01 22:13:25 PDT
Created attachment 471409 [details] [diff] [review]
potential fix
Comment 4 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 neil@parkwaycc.co.uk 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)
      continue;
  // etc.
}
Comment 6 Stefan [:stefanh] (away until May 28) 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 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 Stefan [:stefanh] (away until May 28) 2010-09-05 06:43:52 PDT
Comment on attachment 472226 [details] [diff] [review]
Real Fix

Works fine - thanks.
Comment 9 neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 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 Justin Wood (:Callek) 2010-09-07 08:09:18 PDT
http://hg.mozilla.org/comm-central/rev/a12d80667637

Neil, if you want to do a different approach here, willing to file the bug with the patch, I'll review it :-)
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 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.