1181475 - [e10s] Shift + back/forward button opens page in a new window but renders blank

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Kestrel]

Steps to reproduce:

1. Using an e10s window, click through some pages to create a navigation history.
2. Hold down shift and click on the back/forward toolbar button.

Expected result:

The next page in history is opened in a new window.

Actual result:

The new window is opened with the correct url but the page is blank, the cursor still changes over parts of the page indicating the document is there but not being rendered.

Refreshing and navigating does not help, only switching tabs causes the page to render.

Comment 1

[Tracy Walker [:tracy]]

On Mac it doesn't even shows blank url bar and blank page.  

I'll dig up the regression window.

Comment 2

[Tracy Walker [:tracy]]

Tested back to builds of over a year ago (20140608) and this existed then. So thinking it's not a regression.  Certainly not a recent one.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

This seems to be because the docshell on the newly created browser window is inactive when it should be active. This is because with non-e10s, when we swap docshells, adding a docshell X as a child to another active docshell Y makes X be active. We don't get that automatic behaviour with e10s.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

This appears to be a two piece problem:

The first piece is the docshell bit I outlined in Comment 3. I think we can fix this relatively easily by setting the docshell to be active after a remote frameloader swap in TabChild::RecvSwappedWithOtherRemoteLoader.

The second piece is a race that seems to have been introduced in bug 1162871 that we lose far more often in e10s than in non-e10s mode.

Here is the code that opens one page back in a new window (simplified to the relevant bits):


function duplicateTabIn(aTab, where, delta) {
  let newTab = SessionStore.duplicateTab(window, aTab, delta);

  // ...
      gBrowser.hideTab(newTab);
      gBrowser.replaceTabWithWindow(newTab);
  // ...
}

When gBrowser.replaceTabWithWindow is called, we open up a new browser.xul document in a new window, and eventually call swapBrowsersAndCloseOther on the duplicated tab that we created.

The problem is that sometimes the duplicated tab hasn't had its state restored because the async flush didn't finish in time.

I believe this race does occur with non-e10s, but seems far more prevalent with e10s on.

One idea is to add a new method to SessionStore, called something like "promiseDuplicateTab" or something, that returns a Promise that resolves with the new tab nsIDOMNode once the flush has finished and the tab has been restored. Then, we'd have duplicateTabIn do something like:

SessionStore.promiseDuplicateTab(window, aTab, delta, true).then((newTab) => {
  gBrowser.replaceTabWithWindow(newTab);
});

(where the 4th argument to promiseDuplicateTab hides the newly created tab).

Other possibilities? ttaubert, what do you think?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Hey ttaubert - we started pondering this a bit in IRC, and then I think we both got distracted. Do you have any ideas on how best we could open up a new window, and restore the necessary state into its initial tab?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Or perhaps you have an opinion, Mossop? Do you know what the best way would be to inject the state into the new tab? Speaking as a SessionStore noob here.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

2:02 PM <ttaubert> mconley: so I took a look at that bug and it bugs me. if we'd introduce an async API then we'd probably break tests and other stuff that expects it to be sync, at least for the non-window case
2:03 PM <ttaubert> mconley: and the other point is, wtf are we using duplicateTabIn() in that case anyway? we could just open a new tab and restore state into the new tab in the window. that's a lazy hack

Comment 8

[Dave Townsend [:mossop]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #4)
> function duplicateTabIn(aTab, where, delta) {
>   let newTab = SessionStore.duplicateTab(window, aTab, delta);
> 
>   // ...
>       gBrowser.hideTab(newTab);
>       gBrowser.replaceTabWithWindow(newTab);
>   // ...
> }

This code is a little inefficient. Create a new tab in the current window, create a new window, swap tab into the new window. We can make it better and fix this bug at the same time, no SessionStore changes required:

1. Create new window
2. Duplicate tab into new window
3. Close new window's initial tab

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r=Mossop

Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r?Mossop

A race between SessionStore tab duplication and swapBrowsersAndCloseOther was
causing us to usually display a duplicated tab before it was actually ready
to be shown. We were also being pretty inefficient, since SessionStore supports
being able to duplicate a tab into a new window.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8641238 [details]
MozReview Request: Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r=Mossop

https://reviewboard.mozilla.org/r/14451/#review13081

Ship It!

Comment 11

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dave Townsend [:mossop] from comment #10)
> Comment on attachment 8641238 [details]
> MozReview Request: Bug 1181475 - Refactor duplicateTabIn to avoid a race
> between swapping frameloaders and SessionStore. r?Mossop
> 
> https://reviewboard.mozilla.org/r/14451/#review13081
> 
> Ship It!

Hrm - looking at my try push[1], I think I probably need to wait for the window to be "more ready" before I call duplicateTab, otherwise gBrowser won't be defined in the new window.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56bff499d891

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Huh. I thought we'd be going through nsXULWindow::CreateNewContentWindow, which spins a nested event loop until the XUL has finished loading. I guess not. :/

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8641238 [details]
MozReview Request: Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r=Mossop

Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r?Mossop

A race between SessionStore tab duplication and swapBrowsersAndCloseOther was
causing us to usually display a duplicated tab before it was actually ready
to be shown. We were also being pretty inefficient, since SessionStore supports
being able to duplicate a tab into a new window.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8641238 [details]
MozReview Request: Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r=Mossop

Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r?Mossop

A race between SessionStore tab duplication and swapBrowsersAndCloseOther was
causing us to usually display a duplicated tab before it was actually ready
to be shown. We were also being pretty inefficient, since SessionStore supports
being able to duplicate a tab into a new window.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8641238 [details]
MozReview Request: Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r=Mossop

Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r?Mossop

A race between SessionStore tab duplication and swapBrowsersAndCloseOther was
causing us to usually display a duplicated tab before it was actually ready
to be shown. We were also being pretty inefficient, since SessionStore supports
being able to duplicate a tab into a new window.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8641238 [details]
MozReview Request: Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r=Mossop

This one seems to pass on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c27b4c9ee0fd

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 8641238 [details]
MozReview Request: Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r=Mossop

https://reviewboard.mozilla.org/r/14451/#review13305

Ship It!

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c820913dff6

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8641238 [details]
MozReview Request: Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r=Mossop

Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r=Mossop

A race between SessionStore tab duplication and swapBrowsersAndCloseOther was
causing us to usually display a duplicated tab before it was actually ready
to be shown. We were also being pretty inefficient, since SessionStore supports
being able to duplicate a tab into a new window.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1186448 - pdfjschildbootstrap.js should only be executed once per child process. r?jimm

Bug 1186448 - pdfjschildbootstrap.js should only be executed once per child process. r?jimm

Comment 21

[Mike Conley (:mconley) (:⚙️)]

url:        https://hg.mozilla.org/integration/fx-team/rev/65a711af2c0bc55d434783fee16e6061556506dd
changeset:  65a711af2c0bc55d434783fee16e6061556506dd
user:       Mike Conley <[e-mail]>
date:       Thu Jul 30 15:06:13 2015 -0400
description:
Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r=Mossop

A race between SessionStore tab duplication and swapBrowsersAndCloseOther was
causing us to usually display a duplicated tab before it was actually ready
to be shown. We were also being pretty inefficient, since SessionStore supports
being able to duplicate a tab into a new window.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/65a711af2c0b

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7c820913dff6

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1181475.html