1085045 - Move SwapFrameLoader to nsIFrameLoader

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Comment 1

[Roshan Vidyashankar [:rvid]]

Attachment: swapframeloaderbits(WIP).patch

Comment 2

[Hsin-Yi Tsai (she/her)[:hsinyi]]

Hi Samael,
Assigning this bug to you, as per the Orlando session, this is the start for the b2g pre-rendering feature. Thank you!

Comment 3

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

Attachment: Implement nsIFrameLoader::adaptSessionHistoryForPrerender

Comment 4

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

Comment on attachment 8739312 [details] [diff] [review]
Implement nsIFrameLoader::adaptSessionHistoryForPrerender

Hi Olli,

Here are the main changes to frameloader / docshell for prerendering.

The original plan of this bug seems to be implementing another |swapFrameLoaders()| function in frameloader for prerendering, while my proposal is to add an |adaptSessionHistoryForPrerender()| function, and reuse current |nsXULElement::SwapFrameLoaders()| at frontend.

In tabbrowser.xml, it will adapt prerendered tab in this way:

> let contentBrowser = this.getBrowserForTab(aCurrentTab);
> let prerenderBrowser = aPrerenderedTab.linkedBrowser;
> prerenderBrowser.adaptSessionHistoryForPrerender(contentBrowser);
> this.swapBrowsersAndCloseOther(aCurrentTab, aPrerenderedTab);

There are many things left undecided or unfinished, such as should we fire 
|onlocationchange|? Should we support non-root docshells? Should the session history be cloned or taken or swapped?

I would like to have your feedback on the proposal and hopefully we can have more discussions here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I assume we're adopting history, not adapting it?

Comment 6

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

(In reply to Boris Zbarsky [:bz] from comment #5)
> I assume we're adopting history, not adapting it?

Haha that's embarrassing. I'll fix that.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Trying to get to this tomorrow.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8739312 [details] [diff] [review]
Implement nsIFrameLoader::adaptSessionHistoryForPrerender

>+NS_IMETHODIMP
>+nsDocShell::AdaptSessionHistoryFrom(nsIDocShell *aOther)
Adopt
and nsIDocShell* aOther

>+{
>+  nsCOMPtr<nsISHEntry> currentEntry;
>+  RefPtr<nsDocShell> otherDocShell = static_cast<nsDocShell*>(aOther);
>+  NS_ENSURE_TRUE(otherDocShell, NS_ERROR_FAILURE);
>+
>+  if (!mSessionHistory != !otherDocShell->mSessionHistory) {
>+    NS_ERROR("The docshells must both be root or belong to the same root dochshell.");
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  // Get current SHEntry.
>+  bool oshe;
>+  nsresult rv = GetCurrentSHEntry(getter_AddRefs(currentEntry), &oshe);
>+  NS_ENSURE_TRUE(currentEntry, NS_ERROR_FAILURE);
>+
>+  if (otherDocShell->mSessionHistory) {
>+    // Create new history.
>+    nsCOMPtr<nsISHistory> newSHistory =
>+      do_CreateInstance(NS_SHISTORY_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Get the private interface.
>+    nsCOMPtr<nsISHistoryInternal> newSHistoryPrivate =
>+      do_QueryInterface(newSHistory);
>+    NS_ENSURE_TRUE(newSHistoryPrivate, NS_ERROR_FAILURE);
>+
>+    // Copy all entries from otherDocShell.
>+    nsCOMPtr<nsISimpleEnumerator> iter;
>+    otherDocShell->mSessionHistory->GetSHistoryEnumerator(getter_AddRefs(iter));
>+    bool hasMoreElements;
>+    rv = iter->HasMoreElements(&hasMoreElements);
>+    while (NS_SUCCEEDED(rv) && hasMoreElements) {
>+      nsCOMPtr<nsISupports> item;
>+      rv = iter->GetNext(getter_AddRefs(item));
>+      NS_ENSURE_SUCCESS(rv, rv);
>+
>+      nsCOMPtr<nsISHEntry> entry(do_QueryInterface(item));
>+      if (entry) {
>+        newSHistoryPrivate->AddEntry(entry, true);
>+      }
>+
>+      rv = iter->HasMoreElements(&hasMoreElements);
>+    }
>+
>+    // Append currentEntry to new session history.
>+    rv = newSHistoryPrivate->AddEntry(currentEntry, true);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Apply the new history.
>+    SetSessionHistory(newSHistory);

So why couldn't we take the session history object from aOther and append whatever we have in
session history?. In other words, why do we need to create any new session history object?

What happens to the bfcached documents? Remember that nsISHEntries may keep, via nsSHEntryShared, docshells, contentviewers etc. alive, and those
are from the original docshell.





>+nsFrameLoader::AdaptSessionHistoryForPrerender(nsIFrameLoader* aOtherFrameLoader)
>+{
...
>+  mIsPrerendered = false;
>+  if (IsRemoteFrame()) {
>+    PBrowserParent *otherRemoteBrowser = otherFrameLoader->GetRemoteBrowser();
>+    if (!mRemoteBrowser || !otherRemoteBrowser) {
>+      NS_WARNING("Missing remote browser.");
>+      return NS_ERROR_FAILURE;
>+    }
>+
>+    Unused << mRemoteBrowser->SendAdaptSessionHistoryFrom(otherRemoteBrowser);
What if remote browsers are using different child processes. I guess we need to somehow limit this to the case where same child process is being used, at least for now.
But please add some check.

>+    Unused << mRemoteBrowser->SendSetIsPrerendered(false);
in order to reduce risk for anything unexpected to happen, would it make sense to have just one message from parent to child telling that
"please adopt session history from this other tabchild and then make us a normal, non-prerendered tabchild"


>@@ -2420,6 +2462,10 @@ nsFrameLoader::TryRemoteBrowser()
>     return false;
>   }
>
>+  if (mIsPrerendered) {
>+    Unused << mRemoteBrowser->SendSetIsPrerendered(true);
>+  }
>+
Could we perhaps pass the flag when mRemoteBrowser is being created.
So, either ContentParent::CreateBrowserOrApp could take another parameter, or perhaps MutableTabContext could 
have a flag for prerendering.

>+  void adaptSessionHistoryForPrerender(in nsIFrameLoader aOtherFrameLoader);
adopt

>+++ b/dom/ipc/PBrowser.ipdl
>@@ -534,6 +534,10 @@ child:
>
>     async SizeModeChanged(nsSizeMode sizeMode);
>
>+    async SetIsPrerendered(bool prerendered);
so I think we could live without this explicit message if the state was transferred when PBrowser is created and also ...


>+
>+    async AdaptSessionHistoryFrom(PBrowser browser);
...AdaptSessionHistoryFrom could be renamed to something else which tells that prerendered PBrowser becomes active.
Perhaps async MakePrerenderedBrowserActive(PBrowser previousActiveBrowser)
or some such

Needs some work sure, but I think I like the setup how this would enable prerendering.

Comment 9

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

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8739312 [details] [diff] [review]
> So why couldn't we take the session history object from aOther and append
> whatever we have in
> session history?. In other words, why do we need to create any new session
> history object?

I was hesitate about whether I should reset aOther's mSessionHistory / mLSHE and mOSHE to nullptr. I'm not sure if the function name is clear enough to indicate that the other docshell will essentially being at an invalid status so I was trying to see if I can clone the history.

Do you think it would be better if I change the function in nsIDocShell to
>  /**
>   * Make the prerendered docshell being active. The caller can optionally pass
>   * an original docshell which triggered the prerendering to adopt session 
>   * history from.
>   *
>   * @param aOrigin - The original docshell to take history from. Can be null.
>   *                  Note that since it takes mSessionHistory directly, 
>   *                  |aOrigin| will be invalid after this call, and should be 
>   *                  destroyed without other operations.
>   */
>  void makePrerenderDocShellActive(in nsIDocShell aOrigin);
And just take the mSessionHistory object directly? Much like |MakePrerenderedBrowserActive| you mentioned.

> What happens to the bfcached documents? Remember that nsISHEntries may keep,
> via nsSHEntryShared, docshells, contentviewers etc. alive, and those
> are from the original docshell.

I admit I'm not quite familiar with the details of bfcaches, but my test case shows when calling history.back(), it generates another |load| event instead of just |pageshow|, so I guess all bfcached docs are gone and I'll need to figure out how to preserve it on adopting history.

I'll also try to address other issues you mentioned, and I agree |MakePrerenderedBrowserActive| looks better. I have 2 more questions and hopefully to have your suggestions:

1. Should we support adopting history from non-root docshells? I was trying to do this mainly for testing purpose. I noticed when running XUL mochitest-chrome test, the XUL document is in a content shell which causes <xul:browser> being subframes of it. But I don't see clear use cases otherwise.

2. Should I add a |onHistoryChange| function to |nsIWebProgressListener|? I need a way to update canGoBack / canGoForward cache and |onLocationChange| doesn't look quite right to me.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

it is not quite clear to me what kind of behavior we want for bfcache here.
It becomes super error prone, I think, if we move bfached session history entries as such to a new session history, since everything in the cached documents assume they are still in the old docshells.
One option is to evict bfcache and then adoption would deal with only non-bfcache SHEntries.
Another option is to keep the old docshell there, and if we go back in history to a page which was loaded in that docshell, we swap frameloader's docshell again (I guess that would mean that nsFrameLoader would keep a list of docshells and/or remotebrowsers alive)

Comment 11

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

Ah... maybe I should do it in an inverse way - take the document and history entry from prerendered docshell, re-attach to the original docshell, and update URI / fire onlocationchange as what LoadURI does. I'll try if that's feasible.

Comment 12

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

(In reply to Samael Wang [:freesamael][:sawang] from comment #11)
> Ah... maybe I should do it in an inverse way - take the document and history
> entry from prerendered docshell, re-attach to the original docshell, and
> update URI / fire onlocationchange as what LoadURI does. I'll try if that's
> feasible.

Turns out this approach is basically driving me back to Viven's prototype in bug 1190805, which swaps content viewers and inner windows. This approach might be potentially more buggy and difficult to maintain due to the complexity of docshell...

Comment 13

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

Another issue is that if the page which triggers the prerendering has an opener, the opener can not access the window after docshell swapped. 

For exmaple, if I use |var w = window.open('http://prerender-test.appspot.com');|, type an URL in the webpage, press prerender, and then click the link. After the page swapped, w no longer points to the window, and operations like |w.location| returns null.

Comment 14

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

I'm splitting the history issue into 3 bugs:
Bug 1276553 covers necessary changes to nsISHistory and related objects;
Bug 1276551 covers the implementation of cross root-docshell session history based on bug 1276553.
Bug 1276552 covers History API to reflect the changes above.

Comment 15

[Hsin-Yi Tsai (she/her)[:hsinyi]]

(In reply to Samael Wang [:freesamael][:sawang] from comment #14)
> I'm splitting the history issue into 3 bugs:
> Bug 1276553 covers necessary changes to nsISHistory and related objects;
> Bug 1276551 covers the implementation of cross root-docshell session history
> based on bug 1276553.
> Bug 1276552 covers History API to reflect the changes above.

Samael will revisit this after the above three are done.

Comment 16

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

With current progress I believe this bug is no longer applicable.