Closed
Bug 1263760
Opened 8 years ago
Closed 8 years ago
[e10s] TabSwitchDone event will not dispatch if a modal dialog shows during transition
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 48
People
(Reporter: timdream, Assigned: timdream)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
Discovered when working on the patch of bug 1243729. Visually the tab is switched and the dialog is shown, but the test will stuck in e10s because TabSwitchDown is never received by event listener in BrowserTestUtils.switchTab. This bug prevents `skip-if=e10s` to be removed from the to-be-landed test in bug 1243729.
Comment 1•8 years ago
|
||
Which test is this? I looked through the patches and I couldn't find one that both switches tabs and messes with logins. As it is, it's not really clear what the steps are to reproduce this bug...
Flags: needinfo?(timdream)
Assignee | ||
Comment 2•8 years ago
|
||
Sorry about that. Please apply all the patches. In the 3rd patch (marked as part2.patch) I added a new file which tests the "Select username" dialog from password manager (it is actually a generic nsIPromptService2 select dialog). The way to reproduce is simply run the test file in e10s mode: ./mach mochitest toolkit/components/passwordmgr/test/browser/browser_username_select_dialog.js
Flags: needinfo?(timdream)
Updated•8 years ago
|
Blocks: e10s-tests
Assignee | ||
Comment 3•8 years ago
|
||
My investigation shows that `switcher.finish` is not called, so TabSwitchDone is not dispatched. When `finish` is called, it shows the following stack: _getSwitcher/switcher.finish@chrome://browser/content/tabbrowser.xml:3353:33 _getSwitcher/switcher.postActions@chrome://browser/content/tabbrowser.xml:3533:17 _getSwitcher/switcher.handleEvent@chrome://browser/content/tabbrowser.xml:3687:15 EventListener.handleEvent*_getSwitcher/switcher.init@chrome://browser/content/tabbrowser.xml:3328:15 Line 3329 points to `window.addEventListener("MozLayerTreeCleared", this);`
Comment 4•8 years ago
|
||
Hm. Note that the MozLayerTreeCleared and MozLayerTreeReady events are only fired if the frameloader for the browser has had requestNotifyLayerTreeCleared and requestNotifyLayerTreeReady called on it. That occurs in the switcher here: https://hg.mozilla.org/mozilla-central/file/10f66b316457/browser/base/content/tabbrowser.xml#l3315 and https://hg.mozilla.org/mozilla-central/file/10f66b316457/browser/base/content/tabbrowser.xml#l3319 So you might want to make sure that's happening on your frameloader before you expect the events fired.
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #4) > Hm. Note that the MozLayerTreeCleared and MozLayerTreeReady events are only > fired if the frameloader for the browser has had > requestNotifyLayerTreeCleared and requestNotifyLayerTreeReady called on it. > That occurs in the switcher here: > > https://hg.mozilla.org/mozilla-central/file/10f66b316457/browser/base/ > content/tabbrowser.xml#l3315 > and > https://hg.mozilla.org/mozilla-central/file/10f66b316457/browser/base/ > content/tabbrowser.xml#l3319 > > So you might want to make sure that's happening on your frameloader before > you expect the events fired. Yes, I can see that no tabs are being switch to STATE_UNLOADING, so `requestNotifyLayerTreeCleared` was not called. I am now trying to understand why.
Assignee | ||
Comment 6•8 years ago
|
||
My current theory is that the tabs rely on `onUnloadTimeout` invoked by setTimeout to switch to that state (`this.setTabState(tab, this.STATE_UNLOADING)`), however setTimeout is blocked by the select dialog.
Comment 7•8 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #6) > My current theory is that the tabs rely on `onUnloadTimeout` invoked by > setTimeout to switch to that state (`this.setTabState(tab, > this.STATE_UNLOADING)`), however setTimeout is blocked by the select dialog. This is somewhat plausible - I vaguely recall there being an issue with setTimeout and nested event loops. IIRC nsITimer is not affected, so you could potentially test it by using that instead of setTimeout and seeing if the problem goes away?
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #7) > (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment > #6) > > My current theory is that the tabs rely on `onUnloadTimeout` invoked by > > setTimeout to switch to that state (`this.setTabState(tab, > > this.STATE_UNLOADING)`), however setTimeout is blocked by the select dialog. > > This is somewhat plausible - I vaguely recall there being an issue with > setTimeout and nested event loops. IIRC nsITimer is not affected, so you > could potentially test it by using that instead of setTimeout and seeing if > the problem goes away? Yeah that works locally. I can prepare a patch for it. However, it is very troubling that the prompter can blocks the setTimeout of the entire world. Should we be fixing the deeper issue instead, or I should just submit a tabbrowser.xml patch for review?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Comment 9•8 years ago
|
||
I think it's probably worth fixing this here. I'll reply in-depth on the other bug you just filed.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/48019/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/48019/
Attachment #8743758 -
Flags: review?(gijskruitbosch+bugs)
Updated•8 years ago
|
Attachment #8743758 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 11•8 years ago
|
||
Comment on attachment 8743758 [details] MozReview Request: Bug 1263760 - Replace setTimeout with nsITimer in tabbrowser switcher, r=gijs https://reviewboard.mozilla.org/r/48019/#review44809 r=me with the below fixed and a green try run. ::: browser/base/content/tabbrowser.xml:3304 (Diff revision 1) > + var timer = Components.classes["@mozilla.org/timer;1"] > + .createInstance(Components.interfaces.nsITimer); > + timer.initWithCallback( > + event, timeout, Components.interfaces.nsITimer.TYPE_ONE_SHOT); Please uses `Cc` and `Ci` to make this less verbose. ::: browser/base/content/tabbrowser.xml:3684 (Diff revision 1) > > this.preActions(); > > - clearTimeout(this.unloadTimer); > - this.unloadTimer = setTimeout(() => this.onUnloadTimeout(), this.UNLOAD_DELAY); > + if (this.unloadTimer) { > + this.clearTimer(this.unloadTimer); > + this.unloadTimer = null; This doesn't seem useful as we're setting it to a non-null value straight away.
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8743758 [details] MozReview Request: Bug 1263760 - Replace setTimeout with nsITimer in tabbrowser switcher, r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48019/diff/1-2/
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=217d329bec11
Assignee | ||
Comment 14•8 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #13) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=217d329bec11 Locally, the tests runs correctly but there are two failed assertions. this.assert(this.tabbrowser._switcher); this.assert(this.tabbrowser._switcher === this); I am cancelling Try and will look into this tomorrow.
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8743758 [details] MozReview Request: Bug 1263760 - Replace setTimeout with nsITimer in tabbrowser switcher, r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48019/diff/2-3/
Assignee | ||
Comment 16•8 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #15) > Comment on attachment 8743758 [details] > MozReview Request: Bug 1263760 - Replace setTimeout with nsITimer, r=gijs > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/48019/diff/2-3/ I've missed out one clearTimeout(). https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a96914c998b
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8743758 [details] MozReview Request: Bug 1263760 - Replace setTimeout with nsITimer in tabbrowser switcher, r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48019/diff/3-4/
Attachment #8743758 -
Attachment description: MozReview Request: Bug 1263760 - Replace setTimeout with nsITimer, r=gijs → MozReview Request: Bug 1263760 - Replace setTimeout with nsITimer in tabbrowser switcher, r=gijs
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #16) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a96914c998b
Keywords: checkin-needed
Comment 19•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/42c3256b9538
Keywords: checkin-needed
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/42c3256b9538
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in
before you can comment on or make changes to this bug.
Description
•