1330332 - Don't apply global history updates for prerendered documents until they are activated

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Nika Layzell [:nika] (ooo, ni? for response)]

Ideally this should avoid problems such as links purpling before they are clicked on when <link rel=prerender> is in use.

Comment 1

[Nika Layzell [:nika] (ooo, ni? for response)]

Attachment: Add support for handling global history changes with a HistoryTransaction for prerendering

This ensures that global history is not updated when prerendering a page, and is only updated once that prerendered document becomes visible. This should prevent issues with links purpling and unvisited websites appearing within the places database.

MozReview-Commit-ID: 2j9JwRSGtTC

Comment 2

[Nika Layzell [:nika] (ooo, ni? for response)]

Attachment: Update prerendering tests to validate global history state

MozReview-Commit-ID: BlmyzkURF9t

Comment 3

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

Comment on attachment 8825892 [details] [diff] [review]
Add support for handling global history changes with a HistoryTransaction for prerendering

>+++ b/docshell/base/HistoryTransaction.cpp
Could we please call this file+class something which hints what it is about.
Perhaps PrerenderGlobalHistoryEntry? (I don't quite understand what 'transaction' means in this global history context)
Or PendingGlobalHistoryEntry?

>+HistoryTransaction::ApplyChanges(nsIGlobalHistory2* aHistory)
>+{
>+  nsresult rv;
>+  for (const URIVisit& visit : mVisits) {
>+    bool redirect = (visit.mFlags & IHistory::REDIRECT_TEMPORARY) ||
>+      (visit.mFlags & IHistory::REDIRECT_PERMANENT);
Why is this right? Non-prerender case uses !!aChannelRedirectFlags.
Perhaps worth to separately store redirect flag?

>+private:
>+  struct URIVisit {
{ goes to its own line

>+    nsCOMPtr<nsIURI> mURI;
>+    nsCOMPtr<nsIURI> mLastVisitedURI;
>+    nsCOMPtr<nsIURI> mReferrerURI;
>+    uint32_t mFlags = 0;
>+  };
>+  struct URITitle {
ditto


>       if (mUseGlobalHistory && !UsePrivateBrowsing()) {
>         nsCOMPtr<IHistory> history = services::GetHistoryService();
>-        if (history) {
>+        if (mPrerenderHistoryTransaction) {
>+          mPrerenderHistoryTransaction->SetURITitle(aURI, mTitle);
>+        } else if (history) {
>           history->SetURITitle(aURI, mTitle);
>         } else if (mGlobalHistory) {
>           mGlobalHistory->SetPageTitle(aURI, mTitle);
>         }
>       }
> 
>       SetDocCurrentStateObj(mOSHE);
> 
>@@ -12097,17 +12115,19 @@ nsDocShell::AddState(JS::Handle<JS::Value> aData, const nsAString& aTitle,
>     }
> 
>     AddURIVisit(newURI, oldURI, oldURI, 0);
> 
>     // AddURIVisit doesn't set the title for the new URI in global history,
>     // so do that here.
>     if (mUseGlobalHistory && !UsePrivateBrowsing()) {
>       nsCOMPtr<IHistory> history = services::GetHistoryService();
>-      if (history) {
>+      if (mPrerenderHistoryTransaction) {
>+        mPrerenderHistoryTransaction->SetURITitle(newURI, mTitle);
>+      } else if (history) {
>         history->SetURITitle(newURI, mTitle);
>       } else if (mGlobalHistory) {
>         mGlobalHistory->SetPageTitle(newURI, mTitle);
>       }
So we have similar Set*Title calls in many places. Could you please add some helper method.

Comment 4

[Nika Layzell [:nika] (ooo, ni? for response)]

(In reply to Olli Pettay [:smaug] (review request backlog because of a work week) from comment #3)
> >+++ b/docshell/base/HistoryTransaction.cpp
> Could we please call this file+class something which hints what it is about.
> Perhaps PrerenderGlobalHistoryEntry? (I don't quite understand what
> 'transaction' means in this global history context)
> Or PendingGlobalHistoryEntry?

Changed. I agree the old name was bad.

> >+HistoryTransaction::ApplyChanges(nsIGlobalHistory2* aHistory)
> >+{
> >+  nsresult rv;
> >+  for (const URIVisit& visit : mVisits) {
> >+    bool redirect = (visit.mFlags & IHistory::REDIRECT_TEMPORARY) ||
> >+      (visit.mFlags & IHistory::REDIRECT_PERMANENT);
> Why is this right? Non-prerender case uses !!aChannelRedirectFlags.
> Perhaps worth to separately store redirect flag?

aChannelRedirectFlags is only passed in aChannelRedirectFlags as `0`, or with a result gotten from nsDocShell::ExtractLastVisit, which produces a value previously set with nsDocShell::SaveLastVisit, which is called from nsDocShell::OnRedirectStateChange, which is called fron nsDocLoader::AsyncOnChannelRedirect. asyncOnChannelRedirect is an interface method of nsIChannelEventSink, and the documentation for this method states the following:

     * @param flags
     *        Flags indicating the type of redirect. A bitmask consisting
     *        of flags from above.
     *        One of REDIRECT_TEMPORARY and REDIRECT_PERMANENT will always be
     *        set.

There _are_ other flags available other than REDIRECT_TEMPORARY and REDIRECT_PERMANENT, but only checking for one of those two flags _should_ be sufficient. I can still store the information seperately, but I'll at least add an assertion that the two sources of data match, as in:

    MOZ_ASSERT(((visitURIFlags & IHistory::REDIRECT_TEMPORARY) || (visitURIFlags & IHistory::REDIRECT_PERMANENT)) == !!aChannelRedirectFlags)

Comment 5

[Nika Layzell [:nika] (ooo, ni? for response)]

Attachment: Add support for handling global history changes with PendingGlobalHistoryEntry for prerendering

MozReview-Commit-ID: 2j9JwRSGtTC

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d00ea9cc0fc5
Add support for handling global history changes with PendingGlobalHistoryEntry for prerendering, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/031e8ac3457d
Update prerendering tests to validate global history state, r=smaug

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d00ea9cc0fc5
https://hg.mozilla.org/mozilla-central/rev/031e8ac3457d