Add-ons manager doesn't open if all windows are closed

RESOLVED FIXED in seamonkey2.1b1

Status

SeaMonkey
UI Design
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: stefanh, Assigned: Callek)

Tracking

Trunk
seamonkey2.1b1
x86
Mac OS X

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

659 bytes, patch
neil@parkwaycc.co.uk
: review+
stefanh
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
> I'm assigning it to Callek, since he thought he could look at it

Right, I didn't. But I'll CC him now ;-)
(Reporter)

Updated

7 years ago
Assignee: nobody → bugspam.Callek
(Assignee)

Comment 2

7 years ago
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.
(Assignee)

Comment 3

7 years ago
Created attachment 471409 [details] [diff] [review]
potential fix
Attachment #471409 - Flags: review?(neil)
Attachment #471409 - Flags: feedback?
(Assignee)

Updated

7 years ago
Attachment #471409 - Flags: feedback? → feedback?(stefanh)
(Assignee)

Comment 4

7 years ago
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

7 years ago
(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.
}
(Reporter)

Comment 6

7 years ago
Comment on attachment 471409 [details] [diff] [review]
potential fix

Cancelling request, since Justin told me there's a new patch on the way.
Attachment #471409 - Flags: feedback?(stefanh)
(Assignee)

Comment 7

7 years ago
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.
Attachment #471409 - Attachment is obsolete: true
Attachment #472226 - Flags: review?(neil)
Attachment #472226 - Flags: feedback?
Attachment #471409 - Flags: review?(neil)
(Assignee)

Updated

7 years ago
Attachment #472226 - Flags: feedback? → feedback?(stefanh)
(Reporter)

Comment 8

7 years ago
Comment on attachment 472226 [details] [diff] [review]
Real Fix

Works fine - thanks.
Attachment #472226 - Flags: feedback?(stefanh) → feedback+

Comment 9

7 years ago
Comment on attachment 472226 [details] [diff] [review]
Real Fix

This will do until I manage to demo why getTopWin() is better.
Attachment #472226 - Flags: review?(neil) → review+
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.
(Assignee)

Comment 11

7 years ago
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 :-)
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Updated

7 years ago
Target Milestone: --- → seamonkey2.1b1
You can have r+a=me if you want to apply the same patch to Firefox.
You need to log in before you can comment on or make changes to this bug.