Closed Bug 1310768 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Document Navigation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: freesamael, Assigned: freesamael)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

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
Priority: -- → P2
Attached patch v0 (obsolete) — Splinter Review
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.
Assignee: nobody → sawang
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
Attached patch v1 (obsolete) — Splinter Review
MozReview-Commit-ID: HULmGalL7Jp
Attachment #8809310 - Attachment is obsolete: true
Duplicate of this bug: 1310769
Attached patch v2 (obsolete) — Splinter Review
MozReview-Commit-ID: HULmGalL7Jp
Attachment #8811689 - Attachment is obsolete: true
Attachment #8809310 - Attachment description: wip.patch → v0
Attachment #8811689 - Attachment description: Load about:blank on partial history deactive and restore entry on active → v1
Attachment #8812733 - Attachment description: Load about:blank on partial history deactive and restore entry on active → v2
Attachment #8812733 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
MozReview-Commit-ID: LPcaQkQ900G
Duplicate of this bug: 1276552
Comment on attachment 8813106 [details] [diff] [review]
v3

Hi Ehsan,

Could you help to review the change to suspend background inactive partial historys' docshells?
Attachment #8813106 - Flags: review?(ehsan)
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!
Flags: needinfo?(sawang)
Flags: needinfo?(sawang)
Summary: In GroupedSHistory, swapped out docshell / tabParent is still active and running → Make the active document in inactive docshells of GroupedSHistory being inactive as normal session history entries, and update History API for GroupedHistory
(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.
> 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.
(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 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/
Attachment #8813106 - Flags: review?(ehsan) → review-
Attachment #8813106 - Attachment description: CreataAboutBlankContentViewer to stop inactive tabs. Update History API → v3
Attachment #8813106 - Attachment is obsolete: true
Attached patch v4 (obsolete) — Splinter Review
MozReview-Commit-ID: LPcaQkQ900G
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?
Attachment #8814837 - Flags: review?(ehsan)
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.
Attachment #8814837 - Flags: review?(ehsan) → review+
(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!
(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().
Attachment #8814837 - Attachment description: Use CreataAboutBlankContentViewer to stop inactive tabs. Update History API → v4
Attachment #8814837 - Attachment is obsolete: true
Attachment #8815197 - Attachment description: Use CreataAboutBlankContentViewer to stop inactive tabs. Update History API → Use CreataAboutBlankContentViewer to stop inactive tabs. Update History API. r=ehsan
Attachment #8815197 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcb1d52e5a82
Use CreataAboutBlankContentViewer to stop inactive tabs. Update History API. a=ehsan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fcb1d52e5a82
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Duplicate of this bug: 1320795
You need to log in before you can comment on or make changes to this bug.