Closed Bug 565667 Opened 10 years ago Closed 9 years ago

'Tools' > 'Add-ons' only works when browser window is in the foreground

Categories

(Firefox :: General, defect, minor)

All
macOS
defect
Not set
minor

Tracking

()

VERIFIED FIXED
Firefox 4.0b12
Tracking Status
blocking2.0 --- -

People

(Reporter: MattN, Assigned: zpao)

References

Details

(Keywords: regression, Whiteboard: [rewrite][has patch])

Attachments

(1 file, 7 obsolete files)

Build: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100513 Minefield/3.7a5pre

Steps to Reproduce:
1) Help > Check for Updates... (or put any other window/dialog in the foreground)
2) While Software Update window is focused, click 'Add-ons' from the 'Tools' menu

Expected Result:
Add-ons manager opens in a tab of my most recent browser window

Actual Result:
Add-ons manager does not appear.  Error in error console:

Error: aWindow.gBrowser is null
Source File: chrome://browser/content/browser.js
Line: 9747

A different error happens if the 'error console' is in the foreground:

Error: makeURI is not defined
Source File: chrome://browser/content/browser.js
Line: 9762
Duplicate of this bug: 567621
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a6pre) Gecko/20100618 Lightning/1.1a1pre SeaMonkey/2.1a2pre - Build ID: 20100618022104

This bug might be Firefox-only. When I do Tools => Add-on Manager in the SeaMonkey Mail window, it opens the addons manager in a new browser tab if the browser is already open, or in a new browser window if it isn't. OTOH if I do Tools => Add-on Manager in the Error Console opened above a browser window, the browser comes to the foreground with the addons manager in a new tab.

OTOH my "Help => Check for Updates" popup has no menubar so that part of the tescase does not apply to SeaMonkey. (Trying to invoke the addons manager from the menubar of an underlying browser window would of course bring the browser to the foreground even before I clicked the menuitem.)
blocking2.0: --- → final+
Attached patch Patch v0.1 (obsolete) — Splinter Review
Hit something similar while porting Weave, so fixing this for reuse. No tests because bah tests.
Assignee: nobody → paul
Comment on attachment 453581 [details] [diff] [review]
Patch v0.1

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

> function switchToTabHavingURI(aURI, aOpenNew, aCallback) {

> gURLBar seems to be a lazy way to check.

Non-null gBrowser makes more sense, I think. gURLBar can be legitimately null (customize the location bar away); this code didn't actually handle that correctly before, but it should.

>   function switchIfURIInWindow(aWindow) {
>-    if (!("gBrowser" in aWindow))
>+    if (!("gBrowser" in aWindow) || !aWindow.gBrowser)

Remove the "in" check since it's redundant?

>+  if (!(aURI instanceof Ci.nsIURI)) {
>+    if (gURLBar) {
>+      aURI = makeURI(aURI);
>+    } else {
>+      aURI = Services.io.newURI(aURI, null, null);
>+    }
>+  }

No point baking in both code paths - just avoid relying on contentAreaUtils entirely and use newURI directly.

>+  if (aOpenNew && browserWin) {

>+  else if (aOpenNew) {

>+  }

Can simplify this logic by putting the !browserWin check inside the if (aOpenNew) then-block, I think.
(In reply to comment #4)
> (From update of attachment 453581 [details] [diff] [review])
> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> > function switchToTabHavingURI(aURI, aOpenNew, aCallback) {
> 
> > gURLBar seems to be a lazy way to check.
> 
> Non-null gBrowser makes more sense, I think. gURLBar can be legitimately null
> (customize the location bar away); this code didn't actually handle that
> correctly before, but it should.

True. I'll have to change the conditions a bit inside switchIfURIInWindow, but that's fine.

> >   function switchIfURIInWindow(aWindow) {
> >-    if (!("gBrowser" in aWindow))
> >+    if (!("gBrowser" in aWindow) || !aWindow.gBrowser)
> 
> Remove the "in" check since it's redundant?

Ah true. I was thinking checking aWindow.gBrowser would trigger a JS strict warning (but that's only if you access/use it - checking it doesn't...)

> >+  if (!(aURI instanceof Ci.nsIURI)) {
> >+    if (gURLBar) {
> >+      aURI = makeURI(aURI);
> >+    } else {
> >+      aURI = Services.io.newURI(aURI, null, null);
> >+    }
> >+  }
> 
> No point baking in both code paths - just avoid relying on contentAreaUtils
> entirely and use newURI directly.

Fair.

> >+  if (aOpenNew && browserWin) {
> 
> >+  else if (aOpenNew) {
> 
> >+  }
> 
> Can simplify this logic by putting the !browserWin check inside the if
> (aOpenNew) then-block, I think.

Yea sure. Makes it a little more obvious, even if it comes out to the same.
Blocks: 571897
Target Milestone: --- → mozilla1.9.3b1
Target Milestone: mozilla1.9.3b1 → ---
Does this hard-block the sync UI port?
blocking2.0: final+ → betaN+
(In reply to comment #6)
> Does this hard-block the sync UI port?

No
This behavior occurs when no window is open too (nor browser windows, nor check for updates or downloads).
Just a quick update, I have an updated patch & started writing some tests, but this is lower priority for me until other blocker work is done. Also, I'd say we could drop the blocking betaN+ to blocking final since it's not terrible (obviously, sooner is better, but I'm not going to stress about this)
It's possible it could slip past beta, but I'd rather this made a beta release since I don't think we can have an automated test for it.
Duplicate of this bug: 590460
(In reply to comment #10)
> It's possible it could slip past beta, but I'd rather this made a beta release
> since I don't think we can have an automated test for it.

Should be doable with a Mozmill test.
Duplicate of this bug: 594112
Attached patch Patch v0.2 (obsolete) — Splinter Review
Updated, less sucky (will actually track for the last open window instead of possibly getting stuck with a closed one), and has a test
Attachment #453581 - Attachment is obsolete: true
Attachment #479969 - Flags: review?(gavin.sharp)
Whiteboard: [rewrite] → [rewrite][has patch][needs review gavin]
Comment on attachment 479969 [details] [diff] [review]
Patch v0.2

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

> function switchToTabHavingURI(aURI, aOpenNew, aCallback) {
>   function switchIfURIInWindow(aWindow) {

>+        let prevTab;

nit: you should leave this right above its initialization, rather than at the top of the block.

But now that I think of it, the handleRevert() call and the closing of the existing tab doesn't belong in this method at all - it's only relevant to the urlbarBindings caller (i.e. the autocomplete case), so it should move there (it can be done if switchToTabHavingURI() returns true). I filed bug 604457 for that.

>   if (!(aURI instanceof Ci.nsIURI))
>-    aURI = makeURI(aURI);
>+    aURI = Services.io.newURI(aURI, null, null);

Don't make this change.

>+    else {
>+      window.openDialog("chrome://browser/content/", "_blank",
>+                        "chrome,all,dialog=no", aURI.spec);

Use window.openDialog(getBrowserURL(), "_blank", "all,dialog=no", aURI.spec);

I filed bug 604261 about us being inconsistent about this all over browser.js.

This doesn't end up calling aCallback, which will break some of the callers. Need to fix that, so r-. Probably need to add a load listener for the chrome window, and then have that get the selectedBrowser and call loadURI and add the pageshow handler, rather than passing the URL into openDialog.

>diff --git a/browser/base/content/test/browser_bug565667.js b/browser/base/content/test/browser_bug565667.js

>+function waitForJSConsole(aCount, aCallback) {

Better would be to just modify toOpenWindowByType/toJavaScriptConsole to return the opened windows.

>+  let numTabs = gBrowser.mTabs.length;

gBrowser.tabs, not mTabs (applies elsewhere in the test)
Attachment #479969 - Flags: review?(gavin.sharp) → review-
Whiteboard: [rewrite][has patch][needs review gavin] → [rewrite][needs new patch]
(In reply to comment #15)
> > function switchToTabHavingURI(aURI, aOpenNew, aCallback) {
> >   function switchIfURIInWindow(aWindow) {
> 
> >+        let prevTab;
> 
> nit: you should leave this right above its initialization, rather than at the
> top of the block.

Truth

> >   if (!(aURI instanceof Ci.nsIURI))
> >-    aURI = makeURI(aURI);
> >+    aURI = Services.io.newURI(aURI, null, null);
> 
> Don't make this change.

That's actually pretty critical... makeURI isn't defined because we don't have the overlay (since we're not in a tabbrowser)

> >+    else {
> >+      window.openDialog("chrome://browser/content/", "_blank",
> >+                        "chrome,all,dialog=no", aURI.spec);
> 
> Use window.openDialog(getBrowserURL(), "_blank", "all,dialog=no", aURI.spec);

Alright

> This doesn't end up calling aCallback, which will break some of the callers.
> Need to fix that, so r-. Probably need to add a load listener for the chrome
> window, and then have that get the selectedBrowser and call loadURI and add the
> pageshow handler, rather than passing the URL into openDialog.

Ah good catch. Will do.

> >+function waitForJSConsole(aCount, aCallback) {
> 
> Better would be to just modify toOpenWindowByType/toJavaScriptConsole to return
> the opened windows.

I agree - will do. I expected it to work like that.

> >+  let numTabs = gBrowser.mTabs.length;
> 
> gBrowser.tabs, not mTabs (applies elsewhere in the test)

Yea not sure why that happened...
Attached patch Patch v0.4 (obsolete) — Splinter Review
v0.2 -> v0.4? sure why not.

Addresses review comments.
Attachment #479969 - Attachment is obsolete: true
Attachment #483658 - Flags: review?(gavin.sharp)
Whiteboard: [rewrite][needs new patch] → [rewrite][needs review gavin]
Bug 593687 is changing code in the same place & in a different way, so making that block this.
Depends on: 593687
Duplicate of this bug: 607331
Whiteboard: [rewrite][needs review gavin] → [rewrite][waiting for bug 593687][will need new patch]
Hmm, is my patch in bug 616919 partially redundant with this work here?
(In reply to comment #20)
> Hmm, is my patch in bug 616919 partially redundant with this work here?

Yes.
Whiteboard: [rewrite][waiting for bug 593687][will need new patch] → [rewrite][waiting for bug 593687][will need new patch][soft blocker]
Whiteboard: [rewrite][waiting for bug 593687][will need new patch][soft blocker] → [rewrite][waiting for bug 593687][will need new patch][softblocker]
Duplicate of this bug: 630636
Duplicate of this bug: 626880
Is there a good reason that this blocks betaN?  If not, it should be moved over to final+.
Whiteboard: [rewrite][waiting for bug 593687][will need new patch][softblocker] → [rewrite][waiting for bug 593687][will need new patch][softblocker][final?]
Attached patch Patch v0.5 (obsolete) — Splinter Review
Built on top of the patch in bug 593687. Included tests pass, though I'm a bit confused why I need the window.focus() to make my test pass and work properly in real life - that shouldn't actually be running when run from a non-browser window.
Attachment #483658 - Attachment is obsolete: true
Attachment #511909 - Flags: review?(gavin.sharp)
(In reply to comment #25)
> Is there a good reason that this blocks betaN?  If not, it should be moved over
> to final+.

I don't think so. The fix is pretty safe. So since we're pretty much at the 9PM PST cutoff, moving to blocking? per http://christian.legnitto.com/blog/2011/02/09/blockers-softblockers-betas-and-you/

This should block final because it makes at least one menu item broken when any non-browser window is focused on OSX.
blocking2.0: betaN+ → ?
Whiteboard: [rewrite][waiting for bug 593687][will need new patch][softblocker][final?] → [rewrite][waiting for bug 593687][final?][has patch][needs review gavin]
Comment on attachment 511909 [details] [diff] [review]
Patch v0.5

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

> function toJavaScriptConsole()
> {
>-  toOpenWindowByType("global:console", "chrome://global/content/console.xul");
>+  return toOpenWindowByType("global:console", "chrome://global/content/console.xul");
> }
> 
> function BrowserDownloadsUI()
> {
>   Cc["@mozilla.org/download-manager-ui;1"].
>   getService(Ci.nsIDownloadManagerUI).show(window);
> }
> 
> function toOpenWindowByType(inType, uri, features)
> {
>   var topWindow = Services.wm.getMostRecentWindow(inType);
> 
>-  if (topWindow)
>+  if (topWindow) {
>     topWindow.focus();
>-  else if (features)
>-    window.open(uri, "_blank", features);
>-  else
>-    window.open(uri, "_blank", "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar");
>+    return topWindow;
>+  }
>+  else {
>+    features = features ||
>+               "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar";
>+    return window.open(uri, "_blank", features);
>+  }
> }

This is unnecessary, you can just call window.open directly in the test.

>+    else if (window.gBrowser || hasOtherWindows)
>+      openUILinkIn(aURI.spec, "tab");
>     else
>-      openUILinkIn(aURI.spec, "tab");
>+      openUILinkIn(aURI.spec, "window");

openUILinkIn should deal with "tab" when there is no browser window.

>+    // If a new tab is opened in this window, make sure the window is focused.
>+    if (window.gBrowser)
>+      window.focus();

"This window" can be a popup here, so it isn't necessarily the window you opened the tab in.
Attachment #511909 - Flags: review?(gavin.sharp) → review-
I'm not convinced this had to hold back the release at all. Someone hanging out and tweaking their add-ons with no browser windows open seems like a pretty narrow situation in an already narrow landscape given it's Mac only.

We should definitely take the fix if it's safe and ready to get on our unbelievably-late Fx4 train. However, I don't think we should keep shipping Firefox 3.slow for this change. Blocking-.
blocking2.0: ? → -
Whiteboard: [rewrite][waiting for bug 593687][final?][has patch][needs review gavin] → [rewrite][has patch]
(In reply to comment #28)
> Comment on attachment 511909 [details] [diff] [review]
> Patch v0.5
> 
> >--- a/browser/base/content/browser.js
> >+++ b/browser/base/content/browser.js
> 
> > function toJavaScriptConsole()
> > {
> >-  toOpenWindowByType("global:console", "chrome://global/content/console.xul");
> >+  return toOpenWindowByType("global:console", "chrome://global/content/console.xul");
> > }
> > 
> > function BrowserDownloadsUI()
> > {
> >   Cc["@mozilla.org/download-manager-ui;1"].
> >   getService(Ci.nsIDownloadManagerUI).show(window);
> > }
> > 
> > function toOpenWindowByType(inType, uri, features)
> > {
> >   var topWindow = Services.wm.getMostRecentWindow(inType);
> > 
> >-  if (topWindow)
> >+  if (topWindow) {
> >     topWindow.focus();
> >-  else if (features)
> >-    window.open(uri, "_blank", features);
> >-  else
> >-    window.open(uri, "_blank", "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar");
> >+    return topWindow;
> >+  }
> >+  else {
> >+    features = features ||
> >+               "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar";
> >+    return window.open(uri, "_blank", features);
> >+  }
> > }
> 
> This is unnecessary, you can just call window.open directly in the test.

True, that could be done in a different bug. I just did that per comment 15 (since I wasn't using window.open directly before). I'll take it out & file a new bug for the "cleanup".

> >+    else if (window.gBrowser || hasOtherWindows)
> >+      openUILinkIn(aURI.spec, "tab");
> >     else
> >-      openUILinkIn(aURI.spec, "tab");
> >+      openUILinkIn(aURI.spec, "window");
> 
> openUILinkIn should deal with "tab" when there is no browser window.

It doesn't (I think it's failing here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#209) but I can fix that - just add a check for w before that.

> >+    // If a new tab is opened in this window, make sure the window is focused.
> >+    if (window.gBrowser)
> >+      window.focus();
> 
> "This window" can be a popup here, so it isn't necessarily the window you
> opened the tab in.

Oh I missed that in openUILinkIn. We should probably still focus the window the tab is opened in though right? But I guess openUILinkIn does focus the tab. I don't know what I did to not have my window focused... I'll take it out.
Attached patch Patch v0.6 (obsolete) — Splinter Review
Addressed comments & fixed openLinkIn(url, "tab") to handle the no window case. Dão - poking you for review since you looked at this last :)
Attachment #511909 - Attachment is obsolete: true
Attachment #512291 - Flags: review?(dao)
Comment on attachment 512291 [details] [diff] [review]
Patch v0.6

>   function switchIfURIInWindow(aWindow) {
>-    if (!("gBrowser" in aWindow))
>+    if (!aWindow.gBrowser)
>       return false;

It looks like that check should happen here:

>   // Prioritise this window.
>   if (switchIfURIInWindow(window))
>     return true;

There shouldn't be another way for switchIfURIInWindow to be called with a non-browser window.
Component: Add-ons Manager → General
Product: Toolkit → Firefox
QA Contact: add-ons.manager → general
Yea, that's true. Updated here.
Comment on attachment 512291 [details] [diff] [review]
Patch v0.6

>+  // consoleWin *should* be focused by toJavaScriptConsole(), but just make sure.

This comment seems wrong... toJavaScriptConsole just called window.open, and you're no longer using it.
Comment on attachment 512291 [details] [diff] [review]
Patch v0.6

>+  // Only need to test on OSX
>+  if (!/Mac/.test(navigator.platform)) {
>+    ok(true, "Test only valid on OSX");
>+    finish();
>+  }

Can you take care of this in the Makefile?
Attached patch Patch v0.7 (obsolete) — Splinter Review
bit of cleanup per comments
Attachment #512291 - Attachment is obsolete: true
Attachment #513401 - Flags: review?(dao)
Attachment #512291 - Flags: review?(dao)
Comment on attachment 513401 [details] [diff] [review]
Patch v0.7

>   // Prioritise this window.
>-  if (switchIfURIInWindow(window))
>+  if (window.gBrowser && switchIfURIInWindow(window))
>     return true;

You could add 'var isBrowserWindow = !!window.gBrowser;' here and reuse it there:

>   // No opened tab has that url.
>   if (aOpenNew) {
>-    if (isTabEmpty(gBrowser.selectedTab))
>+    if (window.gBrowser && isTabEmpty(gBrowser.selectedTab))
>       gBrowser.selectedBrowser.loadURI(aURI.spec);
Attachment #513401 - Flags: review?(dao) → review+
Comment on attachment 513401 [details] [diff] [review]
Patch v0.7

> ifneq (cocoa,$(MOZ_WIDGET_TOOLKIT))
> _BROWSER_FILES += \
> 		browser_bug462289.js \
>+		browser_bug565667.js \
> 		$(NULL)
> else
> _BROWSER_FILES += \
> 		browser_customize.js \
> 		$(NULL)

This seems mixed up.
Attached patch Patch v1.0 (obsolete) — Splinter Review
Ah, I missed the Makefile error because I had already built & didn't clean out the test dir when checking. Also cleaned up a couple comments in the test.

Pushing to try just to make sure it's ok.
Attachment #513401 - Attachment is obsolete: true
Comment on attachment 513520 [details] [diff] [review]
Patch v1.0

(In reply to comment #29)
> We should definitely take the fix if it's safe and ready to get on our
> unbelievably-late Fx4 train. However, I don't think we should keep shipping
> Firefox 3.slow for this change. Blocking-.

Looking for approval now that this is wrapped up and safe.
Attachment #513520 - Flags: approval2.0?
Attached patch Patch v1.1Splinter Review
Needed to close the console window when cleaning up - caused orange in one of the fuel tests.
Attachment #513520 - Attachment is obsolete: true
Attachment #513619 - Flags: approval2.0?
Attachment #513520 - Flags: approval2.0?
Has this passed tests on tryserver?
(In reply to comment #42)
> Has this passed tests on tryserver?

It initially failed due to not closing the console window (that was the only failure). The updated patch closes the console window which I validated manually. I just pushed to try again to confirm it's all ok.
Comment on attachment 513619 [details] [diff] [review]
Patch v1.1

a=me on a low-footprint regression fix with a test, and a green tryserver run.
Attachment #513619 - Flags: approval2.0? → approval2.0+
Pushed http://hg.mozilla.org/mozilla-central/rev/59beb0363569
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12) Gecko/20100101 Firefox/4.0b12
Status: RESOLVED → VERIFIED
Flags: in-litmus-
The issue is reproducible on:
Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0 (XP)
and
Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0 (Windows 7)
(In reply to comment #48)
> The issue is reproducible on:
> Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0 (XP)
> and
> Mozilla/5.0 (Windows NT 6.1; rv:2.0) Gecko/20100101 Firefox/4.0 (Windows 7)

This makes no sense. This bug is OS-X-only.
You need to log in before you can comment on or make changes to this bug.