1310768 - Make the active document in inactive docshells of GroupedSHistory being inactive as normal session history entries, and update History API for GroupedHistory

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Samael Wang [:freesamael] (away for now)]

Another follow up for bug 1276553 - the documents of swapped out tabs / docshells should be frozen as in bfcache.

The solution is yet to discuss. nsGlobalWindow::Freeze() [1] might be helpful. Another possibility is make inactive tabs load about:blank but it's a bit tricky.

[1] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/dom/base/nsGlobalWindow.h#1615

Comment 1

[Samael Wang [:freesamael] (away for now)]

Attachment: v0

It seems CreateAboutBlankContentViewer can do the trick for me, but it feels a bit too simple to be true... anyway I'll try to verify that later.

Comment 2

[Samael Wang [:freesamael] (away for now)]

The test case fails after applying the patch. nsSHistory::EvictAllContentViewers was called in frameloader swapping function after docshell's RestorePresentationEvent has been scheduled, and then viewer test [1] fails.

What makes feel worry about is that in frameloader swapping it's intentional to drop bfcache according to the ancient bug 449780. I'm not sure how bad it was and what it implies to groupedshistory...

[1] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/docshell/base/nsDocShell.cpp#8519

Comment 3

[Samael Wang [:freesamael] (away for now)]

Attachment: v1

MozReview-Commit-ID: HULmGalL7Jp

Comment 5

[Samael Wang [:freesamael] (away for now)]

Attachment: v2

MozReview-Commit-ID: HULmGalL7Jp

Comment 6

[Samael Wang [:freesamael] (away for now)]

Attachment: v3

MozReview-Commit-ID: LPcaQkQ900G

Comment 8

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8813106 [details] [diff] [review]
v3

Hi Ehsan,

Could you help to review the change to suspend background inactive partial historys' docshells?

Comment 9

[(no longer active)]

Hi Samael,

I'm a bit lost about what this patch is trying to do.  One problem that it is solving is the issue you described in the beginning of comment 2, but it seems like the patch is doing some extra work on top of that too, and I'm not sure what part of it fixes which issue.  Can you please provide a short description of what problems the patch solves and how?

Thanks!

Comment 10

[Samael Wang [:freesamael] (away for now)]

(In reply to :Ehsan Akhgari from comment #9)
> Hi Samael,
> 
> I'm a bit lost about what this patch is trying to do.  One problem that it
> is solving is the issue you described in the beginning of comment 2, but it
> seems like the patch is doing some extra work on top of that too, and I'm
> not sure what part of it fixes which issue.  Can you please provide a short
> description of what problems the patch solves and how?
> 
> Thanks!

Updated the bug description for that. 

When navigating between different PartialSHistory tabs, the document in a swapped out PartialSHistory is still active, so I'm trying to use CreateAboutBlankContentViewer() to check permitUnload, fire correct pagehide and/or unload events on the document, transfer the ownership of the content viewer, ..., etc. When swapped in, I just restore it by using nsSHistory::InitiateLoad(). I also updated History API in this bug because I found they can share the same test case.

Talking about comment 2 (which I mean bug 449780), I honestly haven't figure it out yet. I had short discussion with bz. He told me the layout data structure could potentially holding incorrect TabChild / nsIWidget pointers after swapping. It's not covered in this bug.

Comment 11

[Samael Wang [:freesamael] (away for now)]

> Talking about comment 2 (which I mean bug 449780), I honestly haven't figure
> it out yet. I had short discussion with bz. He told me the layout data
> structure could potentially holding incorrect TabChild / nsIWidget pointers
> after swapping. It's not covered in this bug.

Oh and the thing is, unlike non-e10s in which the view manager's root widget (nsWindow) could change after swapping, my understanding is that in e10s the view manager should stick to the same PuppetWidget after swapping even if the actual window of the tab has changed, so I don't think it's applicable in e10s. But I could be wrong.

Comment 12

[(no longer active)]

(In reply to Samael Wang [:freesamael][:sawang] from comment #10)
> (In reply to :Ehsan Akhgari from comment #9)
> > Hi Samael,
> > 
> > I'm a bit lost about what this patch is trying to do.  One problem that it
> > is solving is the issue you described in the beginning of comment 2, but it
> > seems like the patch is doing some extra work on top of that too, and I'm
> > not sure what part of it fixes which issue.  Can you please provide a short
> > description of what problems the patch solves and how?
> > 
> > Thanks!
> 
> Updated the bug description for that. 
> 
> When navigating between different PartialSHistory tabs, the document in a
> swapped out PartialSHistory is still active, so I'm trying to use
> CreateAboutBlankContentViewer() to check permitUnload, fire correct pagehide
> and/or unload events on the document, transfer the ownership of the content
> viewer, ..., etc. When swapped in, I just restore it by using
> nsSHistory::InitiateLoad(). I also updated History API in this bug because I
> found they can share the same test case.

Thanks!

> Talking about comment 2 (which I mean bug 449780), I honestly haven't figure
> it out yet. I had short discussion with bz. He told me the layout data
> structure could potentially holding incorrect TabChild / nsIWidget pointers
> after swapping. It's not covered in this bug.

(In reply to Samael Wang [:freesamael][:sawang] from comment #11)
> > Talking about comment 2 (which I mean bug 449780), I honestly haven't figure
> > it out yet. I had short discussion with bz. He told me the layout data
> > structure could potentially holding incorrect TabChild / nsIWidget pointers
> > after swapping. It's not covered in this bug.
> 
> Oh and the thing is, unlike non-e10s in which the view manager's root widget
> (nsWindow) could change after swapping, my understanding is that in e10s the
> view manager should stick to the same PuppetWidget after swapping even if
> the actual window of the tab has changed, so I don't think it's applicable
> in e10s. But I could be wrong.

Hmm, I'm not sure what the right answer is either, but should be easy to test.

Comment 13

[(no longer active)]

Comment on attachment 8813106 [details] [diff] [review]
v3

Review of attachment 8813106 [details] [diff] [review]:
-----------------------------------------------------------------

Looks very good!  Minusing because of the FirePageShowEvent issue.

::: docshell/base/nsDocShell.cpp
@@ +11604,2 @@
>      NS_ASSERTION(NS_IsAboutBlank(mCurrentURI),
>                   "no SHEntry for a non-transient viewer?");

Are you hitting this assertion where mCurrentURI is not valid?  I think it's worth filing a follow-up bug for this, and including the number of the bug in the comment here.

::: docshell/shistory/nsSHistory.cpp
@@ +1932,5 @@
> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> +  if (!doc) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  doc->OnPageShow(true, nullptr);

Shouldn't you use nsContentUtils::FirePageShowEvent instead?

::: dom/base/test/chrome/window_groupedSHistory.xul
@@ +33,4 @@
>    function run() {
>      SpecialPowers.pushPrefEnv(
>        {'set' : [[ 'browser.groupedhistory.enabled', true ]]})
> +    // Since we're towarding not to use GroupedSHistory in non-10s or

Nit: s/towarding not to use/not going to use/

Comment 14

[Samael Wang [:freesamael] (away for now)]

Attachment: v4

MozReview-Commit-ID: LPcaQkQ900G

Comment 15

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8814837 [details] [diff] [review]
v4

(In reply to :Ehsan Akhgari from comment #13)
> Comment on attachment 8813106 [details] [diff] [review]
> v3
> 
> Review of attachment 8813106 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks very good!  Minusing because of the FirePageShowEvent issue.
> 
> ::: docshell/base/nsDocShell.cpp
> @@ +11604,2 @@
> >      NS_ASSERTION(NS_IsAboutBlank(mCurrentURI),
> >                   "no SHEntry for a non-transient viewer?");
> 
> Are you hitting this assertion where mCurrentURI is not valid?  I think it's
> worth filing a follow-up bug for this, and including the number of the bug
> in the comment here.
Found a related bug 1301399 so I commented there and update the patch accordingly.

 
> ::: docshell/shistory/nsSHistory.cpp
> @@ +1932,5 @@
> > +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> > +  if (!doc) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +  doc->OnPageShow(true, nullptr);
> 
> Shouldn't you use nsContentUtils::FirePageShowEvent instead?
After second thought I think I should remove this part. The document is mostly like IsShowing at this point, and forcing fire another pageshow may be confusing for web developers, and I really don't know if that will fit HTML specs. For actual prerendering use case web content should relay on page visibility API instead so I update the test case to do similar thing -- waiting for visibility becomes visible.

Could you help to review this updated patch?

Comment 16

[(no longer active)]

Comment on attachment 8814837 [details] [diff] [review]
v4

Review of attachment 8814837 [details] [diff] [review]:
-----------------------------------------------------------------

Note: there is a typo in the commit message (Creata).

::: docshell/base/nsDocShell.cpp
@@ +11599,5 @@
>    if (shAvailable && mCurrentURI && !mOSHE && aLoadType != LOAD_ERROR_PAGE) {
> +      // XXX mCurrentURI can be changed from any caller regardless what actual
> +      // loaded document is, so testing mCurrentURI isn't really a reliable way.
> +      // Session restore is one example which changes current URI in order to
> +      // show address before loading. See bug 1301399.

Nit: please fix the indentation here.

Comment 17

[(no longer active)]

(In reply to Samael Wang [:freesamael][:sawang] from comment #15)
> > ::: docshell/shistory/nsSHistory.cpp
> > @@ +1932,5 @@
> > > +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> > > +  if (!doc) {
> > > +    return NS_ERROR_FAILURE;
> > > +  }
> > > +  doc->OnPageShow(true, nullptr);
> > 
> > Shouldn't you use nsContentUtils::FirePageShowEvent instead?
> After second thought I think I should remove this part. The document is
> mostly like IsShowing at this point, and forcing fire another pageshow may
> be confusing for web developers, and I really don't know if that will fit
> HTML specs. For actual prerendering use case web content should relay on
> page visibility API instead so I update the test case to do similar thing --
> waiting for visibility becomes visible.

I r+ed the patch because I think it can land before we settle on this part.  But we do need to dispatch a pageshow event when we navigate back/forward to a page in the session history.  Is there some other code that guarantees those events are dispatched here?  Note that FirePageShowEvent takes a aFireIfShowing argument which you can set to false to avoid firing the pageshow event twice.

Can you please file a follow-up bug to add a test to ensure that the pageshow and pagehide events do fire when necessary, and fix the code to make it so if it turns out that we don't fire these events as we should?  Thanks!

Comment 18

[Samael Wang [:freesamael] (away for now)]

(In reply to :Ehsan Akhgari from comment #17)
> But we do need to dispatch a pageshow event when we navigate back/forward to
> a page in the session history.  Is there some other code that guarantees
> those events are dispatched here?  Note that FirePageShowEvent takes a
> aFireIfShowing argument which you can set to false to avoid firing the
> pageshow event twice.

Yes we do. The pageshow mentioned in comment 15 is about what should be done when a non-partial session history firstly joins a grouped session history and becomes partial (i.e. a prerendered tab is just being adopted now).

For a normal GroupedSHistory navigation, like GoForward / GoBackward and GotoIndex, we use existing code to generate corresponding events. CreateAboutBlankContentViewer() will deliver pagehide, and nsSHistory::InitiateLoad() will cause a pageshow later.

>    // Expecting pagehide from previous page, and a pageshow from next page.
>    //
>    // "Pagehide" is sent before calling OnRequestCrossBrowserNavigation(),
>    // so it should be handled before swapping. "Pageshow" on the other hand
>    // should be handled after swapping cause frameloader is synchronously
>    // swapping on main thread after calling PartialSHistory::OnActive().
>    //
>    // Therefore both messages should be delivered to browser.messageManager.
>    promises.push(BrowserTestUtils.waitForMessage(browser.messageManager,
>      'test:content-pagehide', msg => msg.data && (msg.data.title == prevTitle)));
>    promises.push(BrowserTestUtils.waitForMessage(browser.messageManager,
>      'test:content-pageshow', msg => msg.data && (msg.data.title == nextTitle)));

Here's how we test it in wrapHistoryNavFn(), it waits for explicit "test:content-pagehide" / "test:content-pageshow" messages and verify the title to ensure it comes from expected document. 

The messages are from custom events generated by the web content when the web content gets pagehide / pageshow, so we guarantee it's not the pagehide / pageshow delivered to chrome handlers directly in TabChild::RecvSwappedWithOtherRemoteLoader().

Comment 19

[Samael Wang [:freesamael] (away for now)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f87e398dceb

Comment 20

[Samael Wang [:freesamael] (away for now)]

Attachment: Use CreataAboutBlankContentViewer to stop inactive tabs. Update History API. r=ehsan

Fix indent.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcb1d52e5a82
Use CreataAboutBlankContentViewer to stop inactive tabs. Update History API. a=ehsan

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fcb1d52e5a82