User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 Build ID: 20160824030337 Steps to reproduce: 1. Start Nightly 2. Open a New Window 3. Move a tab from the other window 4. Check error in Browser Console Actual results: TypeError: win is null URLBarZoom.jsm:24:7 Regression range: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=8725f14625e0c87776f9acf66e6712ceca301000&tochange=ef0801fdc7abe018a8b026c00e8bdebcc690001c Expected results: Moving a tab from other window without error

This patch is probably too simplistic a way to handle this, but this patch gets the chrome window from the focus manager's focusedWindow. Is there another way I should access the chrome window after a tab has been dragged to a different window? Let me know if my terminology is confusing, talking about these things feels a little like who's on first. I've been looking at examples of how other browser modules handle this, but I haven't found one that seems to cover the same scenario.

Why does the attempt to get the window fail in the first place? Is it because of _executeSoon being used here? https://dxr.mozilla.org/mozilla-central/rev/26e22af660e543ebb69930f082188b69ec756185/browser/base/content/browser-fullZoom.js#521 If so I think we should probably just remove that.

Hm yeah, I'm really not sure why it's not able to get the window. My hypothesis was that the window aSubject references no longer exists when the user drags a tab from an existing window into another one. When I remove the _executeSoon function and just have: ``` _notifyOnLocationChange: function FullZoom__notifyOnLocationChange(browser) { Services.obs.notifyObservers(browser, "browser-fullZoom:location-change", ""); }, ``` I still get the TypeError that win is null when moving a tab from one window to another.

I've found a better solution than just using the focused window in this most recent patch, though I'm still not 100% sure why the "browser-fullZoom:location-change" observer doesn't fire once the tab has been moved to a new window and has a new browser the notifyObservers function should pass along. It seems like FullZoom.onLocationChange() should be called in XULBrowserWindow.onLocationChange() in browser.js, but I wasn't sure if that would cause more issues that it solves, so I just called the more specific URLBarZoom.updateZoomButton() instead.

Comment on attachment 8786107 [details] Bug 1297970 - Prevent updateZoomButton from being triggered when a tab's window has closed after the tab was dragged to a new window (In reply to Katie Broida [:ktbee] from comment #6) > I've found a better solution than just using the focused window in this most > recent patch, though I'm still not 100% sure why the > "browser-fullZoom:location-change" observer doesn't fire once the tab has > been moved to a new window and has a new browser the notifyObservers > function should pass along. It seems like FullZoom.onLocationChange() should > be called in XULBrowserWindow.onLocationChange() in browser.js, but I wasn't > sure if that would cause more issues that it solves, so I just called the > more specific URLBarZoom.updateZoomButton() instead. Pretty sure will cause URLBarZoom.updateZoomButton to run too often for ordinary location changes, since it's already invoked via FullZoom.onLocationChange in TabsProgressListener.onLocationChange. I still don't fully understand this bug :(

Aurora 51.0a2 (2016-09-27) and Nightly 52.0a1 (2016-09-27) are also affected.

This shows up a fair amount during testing, I happened to be looking into a test failure and it was definitely misleading: > 245 JavaScript error: resource://app/modules/URLBarZoom.jsm, line 24: TypeError: win is null This warning [1] shows up in the following test suites: > 65 - desktop-test-linux64/debug-mochitest-browser-chrome-4 bc4 > 32 - desktop-test-linux64/debug-mochitest-browser-chrome-e10s-6 bc6 > 27 - desktop-test-linux64/debug-mochitest-jetpack JP > 24 - desktop-test-linux64/debug-mochitest-browser-chrome-7 bc7 > 22 - desktop-test-linux64/debug-mochitest-browser-chrome-2 bc2 > 13 - desktop-test-linux64/debug-mochitest-browser-chrome-3 bc3 > 7 - desktop-test-linux64/debug-mochitest-e10s-8 8 > 7 - desktop-test-linux64/debug-mochitest-browser-chrome-e10s-7 bc7 > 6 - desktop-test-linux64/debug-mochitest-browser-chrome-e10s-1 bc1 > 5 - desktop-test-linux64/debug-mochitest-1 1 > 5 - desktop-test-linux64/debug-mochitest-e10s-3 3 > 4 - desktop-test-linux64/debug-mochitest-browser-chrome-e10s-3 bc3 > 4 - desktop-test-linux64/debug-mochitest-browser-chrome-e10s-4 bc4 > 4 - desktop-test-linux64/debug-mochitest-e10s-1 1 > 3 - desktop-test-linux64/debug-mochitest-browser-chrome-e10s-5 bc5 > 3 - desktop-test-linux64/debug-mochitest-3 3 > 3 - desktop-test-linux64/debug-mochitest-browser-chrome-6 bc6 > 3 - desktop-test-linux64/debug-mochitest-browser-chrome-5 bc5 > 2 - desktop-test-linux64/debug-crashtest-e10s C > 2 - desktop-test-linux64/debug-mochitest-clipboard cl > 1 - desktop-test-linux64/debug-mochitest-e10s-5 5 > 1 - desktop-test-linux64/debug-mochitest-e10s-9 9 > 1 - desktop-test-linux64/debug-mochitest-devtools-chrome-2 dt2 > 1 - desktop-test-linux64/debug-mochitest-e10s-10 10 It shows up in 105 tests. A few of the most prevalent: > 27 - jetpack-package/addon-sdk/source/test/test-simple-prefs.js.testUnloadOfDynamicPrefGeneration > 16 - browser/components/sessionstore/test/browser_354894_perwindowpb.js > 16 - [e10s] browser/components/sessionstore/test/browser_354894_perwindowpb.js > 9 - browser/components/extensions/test/browser/browser_ext_tabs_onUpdated.js > 6 - browser/components/sessionstore/test/browser_590563.js > 6 - browser/components/privatebrowsing/test/browser/browser_privatebrowsing_windowtitle.js > 6 - browser/components/sessionstore/test/browser_restore_cookies_noOriginAttributes.js > 5 - browser/components/sessionstore/test/browser_701377.js > 4 - browser/base/content/test/general/browser_bug462673.js > 4 - browser/base/content/test/general/browser_newWindowDrop.js [1] http://searchfox.org/mozilla-central/rev/3e03a4064eb585d96f28023785a5c242969878a6/browser/modules/URLBarZoom.jsm#24

I can't reproduce on either Windows or OSX. :-\ I can reproduce bug 1300376, but no errors appear in the browser console for URLBarZoom.jsm, neither when using mouse dragging nor when using the tab context menu to move a tab to a new window. Do we know under what exact circumstances this happens?

(In reply to :Gijs Kruitbosch from comment #10) > I can't reproduce on either Windows or OSX. :-\ > > I can reproduce bug 1300376, but no errors appear in the browser console for > URLBarZoom.jsm, neither when using mouse dragging nor when using the tab > context menu to move a tab to a new window. > > Do we know under what exact circumstances this happens? Repros on linux64 debug with the following for me: > ./mach mochitest --disable-e10s browser/components/privatebrowsing/test/browser/browser_privatebrowsing_windowtitle.js

(In reply to Eric Rahm [:erahm] from comment #11) > Repros on linux64 debug with the following for me: > > > ./mach mochitest --disable-e10s browser/components/privatebrowsing/test/browser/browser_privatebrowsing_windowtitle.js I can reproduce this on OS X opt, indeed. This problem goes away for me when I remove the executeSoon referenced in comment #3. AIUI when we move tabs we swap docshells between 2 browsers. It would make sense that this triggers onLocationChange events, but if the window containing the original browser has closed by the time we run the executeSoon, because there are no tabs left in that window, then it also makes sense that the defaultView property is null. If we think the executeSoon is unimportant we can get rid of it. I don't know full zoom code well enough to decide either way. If we don't want to change behaviour like that, a simple fix that seems to work in my local testing is to guard the notifyObservers call (inside the executeSoon'd callback in _notifyOnLocationChange) with !window.closed . I would assume other consumers also don't care about dead windows.

(In reply to :Gijs Kruitbosch from comment #12) > (In reply to Eric Rahm [:erahm] from comment #11) > > Repros on linux64 debug with the following for me: > > > > > ./mach mochitest --disable-e10s browser/components/privatebrowsing/test/browser/browser_privatebrowsing_windowtitle.js > > I can reproduce this on OS X opt, indeed. > > This problem goes away for me when I remove the executeSoon referenced in > comment #3. > > AIUI when we move tabs we swap docshells between 2 browsers. It would make > sense that this triggers onLocationChange events, but if the window > containing the original browser has closed by the time we run the > executeSoon, because there are no tabs left in that window, then it also > makes sense that the defaultView property is null. > > If we think the executeSoon is unimportant we can get rid of it. I don't > know full zoom code well enough to decide either way. If we don't want to > change behaviour like that, a simple fix that seems to work in my local > testing is to guard the notifyObservers call (inside the executeSoon'd > callback in _notifyOnLocationChange) with !window.closed . I would assume > other consumers also don't care about dead windows. Katie, do you have time to look at either of these solutions?

Thank you for these suggestions, Gijs! Removing executeSoon or guarding it with !window.closed does prevent the error from appearing, but because onLocationChange isn't triggered when the tab is moved to a new window (and the old one is closed), the zoom indicator shows the wrong zoom level until the zoom is changed or reset and triggers updateZoomButton function again. Just to make sure we're on the same page, this is what I think you mean in terms of guarding executeSoon: ``` _notifyOnLocationChange: function FullZoom__notifyOnLocationChange(browser) { if(!window.closed) { this._executeSoon(function () { Services.obs.notifyObservers(browser, "browser-fullZoom:location-change", ""); }); } }, ``` I've attached a gif of the incorrect zoom behavior with the above code in place. Here at the STR as well: 1) Open two browser windows. Browser 1 should have just one tab. 2) In browser 1, navigate to a URL and change the zoom level so that the zoom indicator appears. 3) In browser 2, change the zoom level to be different from browser 1. 4) Drag the single tab in browser 1 to browser 2's window. This should close browser 1's window. 5) Observe that the tab from browser 1's zoom indicator zoom level now matches browser 2's zoom level, but not the tab's actual zoom level. You can verify this by checking the Customize panel's zoom buttons' zoom level. Is there another way we should trigger onLocationChange or even updateZoomButton directly in these cases?

(In reply to Katie Broida [:ktbee] from comment #14) > Created attachment 8802199 [details] > zoom-fails-when-tab-switches-windows.gif > > Thank you for these suggestions, Gijs! Removing executeSoon or guarding it > with !window.closed does prevent the error from appearing, but because > onLocationChange isn't triggered when the tab is moved to a new window (and > the old one is closed), the zoom indicator shows the wrong zoom level until > the zoom is changed or reset and triggers updateZoomButton function again. > > Just to make sure we're on the same page, this is what I think you mean in > terms of guarding executeSoon: > > ``` > _notifyOnLocationChange: function FullZoom__notifyOnLocationChange(browser) { > if(!window.closed) { > this._executeSoon(function () { > Services.obs.notifyObservers(browser, > "browser-fullZoom:location-change", ""); > }); > } > }, > ``` I meant putting the check inside the function we're passing to executeSoon. I'm a little puzzled that adding the check on the outside would fix anything. I think it would probably be best to have the check on the inside because having it on the outside would leave open the case where the window is not closed when we call _executeSoon but has closed when the function we pass gets called. > I've attached a gif of the incorrect zoom behavior with the above code in > place. Here at the STR as well: > > 1) Open two browser windows. Browser 1 should have just one tab. > 2) In browser 1, navigate to a URL and change the zoom level so that the > zoom indicator appears. > 3) In browser 2, change the zoom level to be different from browser 1. > 4) Drag the single tab in browser 1 to browser 2's window. This should close > browser 1's window. > 5) Observe that the tab from browser 1's zoom indicator zoom level now > matches browser 2's zoom level, but not the tab's actual zoom level. You can > verify this by checking the Customize panel's zoom buttons' zoom level. > > Is there another way we should trigger onLocationChange or even > updateZoomButton directly in these cases? I think this is effectively the same as bug 1300376. So it already does not behave correctly. I think we can fix one issue at a time. :-)

(In reply to :Gijs Kruitbosch from comment #15) > (In reply to Katie Broida [:ktbee] from comment #14) > I think this is effectively the same as bug 1300376. So it already does not > behave correctly. I think we can fix one issue at a time. :-) Oh! Okay, I didn't realize that the inconsistent zoom behavior could be tackled in a separate bug. In that case, having a guard within _executeSoon does indeed prevent this error message issue. I'll submit that for a review now.

Comment on attachment 8786107 [details] Bug 1297970 - Prevent updateZoomButton from being triggered when a tab's window has closed after the tab was dragged to a new window I think we should try removing _executeSoon before resorting to this option.

Just removing _executeSoon doesn't prevent prevent the "win is null" error message from appearing in the browser console. This is how my code is set up without _executeSoon: _notifyOnLocationChange: function FullZoom__notifyOnLocationChange(browser) { Services.obs.notifyObservers(browser, "browser-fullZoom:location-change", ""); }, Since the error message is still appearing without _executeSoon, should we use the !(window.closed) guard Gijs mentioned?

Gijs said in comment 12 that removing _executeSoon did fix the error for him. Either of you must have done something wrong.

(In reply to Dão Gottwald [:dao] from comment #20) > Gijs said in comment 12 that removing _executeSoon did fix the error for > him. Either of you must have done something wrong. I checked again. On my mac, running: ./mach mochitest --disable-e10s browser/components/privatebrowsing/test/browser/browser_privatebrowsing_windowtitle.js produces the error in the summary of this bug. After applying: diff --git a/browser/base/content/browser-fullZoom.js b/browser/base/content/browser-fullZoom.js --- a/browser/base/content/browser-fullZoom.js +++ b/browser/base/content/browser-fullZoom.js @@ -511,21 +511,19 @@ var FullZoom = { return browser.loadContext; }, /** * Asynchronously broadcasts "browser-fullZoom:location-change" so that * listeners can be notified when the zoom levels on those pages change. * The notification is always asynchronous so that observers are guaranteed a * consistent behavior. */ _notifyOnLocationChange: function FullZoom__notifyOnLocationChange(browser) { - this._executeSoon(function () { - Services.obs.notifyObservers(browser, "browser-fullZoom:location-change", ""); - }); + Services.obs.notifyObservers(browser, "browser-fullZoom:location-change", ""); }, _executeSoon: function FullZoom__executeSoon(callback) { if (!callback) return; Services.tm.mainThread.dispatch(callback, Ci.nsIThread.DISPATCH_NORMAL); }, }; it no longer does. It's possible Katie is testing more thoroughly and there are other situations in which it still produces an error, or that she's seeing something else altogether.

Thanks for the clarification on where the error disappeared from, Gijs. When I apply the same changes you did (just remove _executeSoon), I'm also finding the error is removed from the same test, but I still see this "win is null" error when I build and run Nightly in the browser console. STR: 1) Apply changes Gijs listed in comment 21 2) Build with these changes and run (./mach build faster && ./mach run) 3) Open two separate browser windows (each with a single tab) and the browser console. 4) Drag and drop one tab into the window of another and observe error in browser console. I believe this error I'm seeing is part of what we're trying to prevent, but let me know if I'm mistaken.

(In reply to Katie Broida [:ktbee] from comment #22) > Thanks for the clarification on where the error disappeared from, Gijs. When > I apply the same changes you did (just remove _executeSoon), I'm also > finding the error is removed from the same test, but I still see this "win > is null" error when I build and run Nightly in the browser console. > > STR: > 1) Apply changes Gijs listed in comment 21 > 2) Build with these changes and run (./mach build faster && ./mach run) > 3) Open two separate browser windows (each with a single tab) and the > browser console. > 4) Drag and drop one tab into the window of another and observe error in > browser console. > > I believe this error I'm seeing is part of what we're trying to prevent, but > let me know if I'm mistaken. No, I think you're right. Earlier I wasn't able to reproduce that particular issue (comment #10). If you *can* reproduce, and removing the _executeSoon doesn't fix anything, I'd suggest re-requesting review from dao on the patch you already have, as apparently removing the _executeSoon isn't enough to fix this.

This bug is fixed by the patch in bug 1300376 which just landed.

Too late for 51 and we are about to ship 52 this week. Mark 51 won't fix.

Syncing status flags from bug 1300376.

I've managed to reproduce this issue on an old Nightly build from 2016-08-25 using str form comment 0. This issue is verified fixed on latest Aurora 54.0a2 (2017-03-08) and 53.0b1-build1 (20170307064827) under the following OSes: - Windows 10 x64 - Mac OS X 10.11.6 - Ubuntu 16.04 x64 LTS I can no longer see the "TypeError: win is null URLBarZoom.jsm:24:7" in the Browser Console.