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

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.)

Hit something similar while porting Weave, so fixing this for reuse. No tests because bah tests.

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.

Does this hard-block the sync UI port?

(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.

(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.

Updated, less sucky (will actually track for the last open window instead of possibly getting stuck with a closed one), and has a test

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)

(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...

v0.2 -> v0.4? sure why not. Addresses review comments.

Bug 593687 is changing code in the same place & in a different way, so making that block this.

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.

Is there a good reason that this blocks betaN? If not, it should be moved over to final+.

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.

(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.

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.

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-.

(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.

Addressed comments & fixed openLinkIn(url, "tab") to handle the no window case. Dão - poking you for review since you looked at this last :)

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.

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?

bit of cleanup per comments

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);

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.

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.

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.

Needed to close the console window when cleaning up - caused orange in one of the fuel tests.

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.

And all green on try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=3d86ae557b2f

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.

Pushed http://hg.mozilla.org/mozilla-central/rev/59beb0363569

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12) Gecko/20100101 Firefox/4.0b12

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.