Closed Bug 1181475 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Document Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
e10s m8+ ---
firefox42 --- fixed

People

(Reporter: ke5trel, Assigned: mconley)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Blocks: e10s
On Mac it doesn't even shows blank url bar and blank page.  

I'll dig up the regression window.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(twalker)
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.
Flags: needinfo?(twalker)
Assignee: nobody → gkrizsanits
Assignee: gkrizsanits → 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.
Status: NEW → ASSIGNED
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?
Flags: needinfo?(ttaubert)
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?
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.
Flags: needinfo?(dtownsend)
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
(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
Flags: needinfo?(dtownsend)
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.
Attachment #8641238 - Flags: review?(dtownsend)
Attachment #8641238 - Flags: review?(dtownsend) → review+
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!
(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
Flags: needinfo?(ttaubert)
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 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 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 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 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
Attachment #8641238 - Flags: review+ → review?(dtownsend)
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!
Attachment #8641238 - Flags: review?(dtownsend) → review+
Keywords: checkin-needed
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.
Attachment #8641238 - Attachment description: MozReview Request: Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r?Mossop → MozReview Request: Bug 1181475 - Refactor duplicateTabIn to avoid a race between swapping frameloaders and SessionStore. r=Mossop
Bug 1186448 - pdfjschildbootstrap.js should only be executed once per child process. r?jimm
Attachment #8643256 - Flags: review?(jmathies)
Attachment #8643256 - Attachment is obsolete: true
Attachment #8643256 - Flags: review?(jmathies)
url:        https://hg.mozilla.org/integration/fx-team/rev/65a711af2c0bc55d434783fee16e6061556506dd
changeset:  65a711af2c0bc55d434783fee16e6061556506dd
user:       Mike Conley <mconley@mozilla.com>
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.
https://hg.mozilla.org/mozilla-central/rev/65a711af2c0b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.