Closed Bug 1263760 Opened 5 years ago Closed 5 years ago

[e10s] TabSwitchDone event will not dispatch if a modal dialog shows during transition

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
e10s + ---
firefox48 --- fixed

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.
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)
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)
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);`
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.
(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.
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.
(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?
(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: nobody → timdream
Status: NEW → ASSIGNED
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)
Attachment #8743758 - Flags: review?(gijskruitbosch+bugs) → review+
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.
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/
(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.
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/
(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
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
(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
https://hg.mozilla.org/mozilla-central/rev/42c3256b9538
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.