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

RESOLVED FIXED in Firefox 42

Status

()

Core
Document Navigation
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: Kestrel, Assigned: mconley)

Tracking

Trunk
mozilla42
Points:
---

Firefox Tracking Flags

(e10sm8+, firefox42 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Blocks: 516752
tracking-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
tracking-e10s: ? → m8+
(Assignee)

Updated

2 years ago
Assignee: gkrizsanits → mconley
(Assignee)

Comment 3

2 years ago
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
(Assignee)

Comment 4

2 years ago
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)
(Assignee)

Comment 5

2 years ago
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?
(Assignee)

Comment 6

2 years ago
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)
(Assignee)

Comment 7

2 years ago
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)
(Assignee)

Comment 9

2 years ago
Created 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 - 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!
(Assignee)

Comment 11

2 years ago
(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)
(Assignee)

Comment 12

2 years ago
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. :/
(Assignee)

Comment 13

2 years ago
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.
(Assignee)

Comment 14

2 years ago
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.
(Assignee)

Comment 15

2 years ago
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.
(Assignee)

Comment 16

2 years ago
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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 18

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c820913dff6
Keywords: checkin-needed
(Assignee)

Comment 19

2 years ago
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
(Assignee)

Comment 20

2 years ago
Created attachment 8643256 [details]
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
Attachment #8643256 - Flags: review?(jmathies)
(Assignee)

Updated

2 years ago
Attachment #8643256 - Attachment is obsolete: true
Attachment #8643256 - Flags: review?(jmathies)
(Assignee)

Comment 21

2 years ago
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
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
https://hg.mozilla.org/mozilla-central/rev/7c820913dff6
(Assignee)

Comment 24

10 months ago
bugnotes
Created attachment 8774152 [details]
Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1181475.html
You need to log in before you can comment on or make changes to this bug.