Bugzilla

Updated

•

8 years ago

Priority: -- → P2

Updated

•

8 years ago

Assignee: nobody → kbroida

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/75066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75066/

Comment 2

•

8 years ago

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.

Comment 3

•

8 years ago

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.

Comment 4

•

8 years ago

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.

Comment hidden (mozreview-request)

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75066/diff/1-2/

Comment 6

•

8 years ago

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.

Marco Bonardo [:mak] (Away Apr 25 - May 5)

Comment 7

•

8 years ago

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

Attachment #8786107 - Flags: review?(dao+bmo) → review-

Andrei Vaida [:avaida]

Comment 8

•

8 years ago

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

status-firefox52: --- → affected

Flags: qe-verify+

Comment 9

•

8 years ago

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

Eric Rahm [:erahm]

Updated

•

8 years ago

Blocks: logspam

Updated

•

8 years ago

Blocks: 1300376

Comment 10

•

8 years ago

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?

Eric Rahm [:erahm]

Comment 11

•

8 years ago

(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

Comment 12

•

8 years ago

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

Comment 13

•

8 years ago

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

Flags: needinfo?(kbroida)

Comment 14

•

8 years ago

Attached image zoom-fails-when-tab-switches-windows.gif — Details

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?

Flags: needinfo?(kbroida)

Comment 15

•

8 years ago

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

Comment 16

•

8 years ago

(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 hidden (mozreview-request)

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75066/diff/2-3/

Comment 18

•

8 years ago

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.

Attachment #8786107 - Flags: review?(dao+bmo)

Comment 19

•

8 years ago

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?

Flags: needinfo?(dao+bmo)

Comment 20

•

8 years ago

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

Flags: needinfo?(dao+bmo)

Comment 21

•

8 years ago

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

Flags: needinfo?(kbroida)

Comment 22

•

8 years ago

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.

Flags: needinfo?(kbroida)

Jared Wein [:jaws] (please needinfo? me)

Comment 23

•

8 years ago

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

Comment hidden (mozreview-request)

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75066/diff/3-4/

Comment hidden (mozreview-request)

Just removing _executeSoon doesn't prevent a "win is null" message from appearing in the browser console when dragging a tab to a new window. Gaurding the notifyObservers function with !window.closed ensures that there isn't an error when dragging and dropping a tab.

Review commit: https://reviewboard.mozilla.org/r/89684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89684/

Updated

•

8 years ago

Attachment #8786107 - Flags: review?(dao+bmo)

Jared Wein [:jaws] (please needinfo? me)

Updated

•

8 years ago

Attachment #8806203 - Flags: review?(dao+bmo)

Jared Wein [:jaws] (please needinfo? me)

Comment 26

•

8 years ago

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

Updated

•

7 years ago

Assignee: kbroida → nobody

Updated

•

7 years ago

No longer blocks: 1300376

Depends on: 1300376