1326251 - Session history in dynamically added iframes break when loading new pages to the iframes after bfcache restore

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

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

.

Comment 1

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

Trying to understand all these mystery iframe history stuff...

Comment 2

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

Attachment: Part 1 v1

MozReview-Commit-ID: 4pBy0vO5yD9

Comment 3

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

Attachment: Part 2 v1

MozReview-Commit-ID: 5YfIoH1f4fD

Comment 4

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

Comment on attachment 8825385 [details] [diff] [review]
Part 1 v1

I found children's nsDocShell::Destory() is too late to operate on session history since the parent already SetDocLoaderParent(nullptr) before. So I eventually chose to remove dynamic entries on root document is going to unload.

Would you take a look on the patch?

Comment 5

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

Comment on attachment 8825385 [details] [diff] [review]
Part 1 v1

>@@ -1695,16 +1695,27 @@ nsDocShell::FirePageHideNotification(boo
>     }
> 
>     n = kids.Length();
>     for (uint32_t i = 0; i < n; ++i) {
>       if (kids[i]) {
>         kids[i]->FirePageHideNotification(aIsUnload);
>       }
>     }
>+
>+    // If the document is unloading and this is the root docshell, remove all
>+    // dynamic frame entries.
>+    if (aIsUnload) {
>+      nsCOMPtr<nsISHistoryInternal> shPrivate =
>+        do_QueryInterface(mSessionHistory);
>+      if (shPrivate) {
>+        shPrivate->RemoveDynEntries();
>+      }
>+    }
Ah, this is quite nice place to do this.
And I guess this gets called for top level docshells too when they are removed from bfcache.
Hmm, but in that case RemoveRynEntries plays with wrong index. ... except that mFiredUnloadEvent is already true, so we don't enter this code path.
So, I guess we just end up leaving dynamic entries to the session history.
Could we possibly move RemoveDynEntries outside mFiredUnloadEvent check?

But how does this approach deal with iframes.
Couldn't we remove dynamic entries also when destroying the docshell for iframe, I mean the dynamic entries underneath that particular iframe.
This should happen also when unloading an iframe I think.

Comment 6

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

Comment on attachment 8825386 [details] [diff] [review]
Part 2 v1

>+        // Load another page in dynamic iframe, canGoForward should be false.
>+        await loadUriInFrame('dynamicFrame', 'frame3.html');
>+        opener.is(webNav.canGoForward, false, 'canGoForward');
>+        opener.is(shistory.index, 5, 'shistory.index');
>+        opener.is(history.length, 6, 'history.legnth');
length in the comment

This could be then extended a bit if the patch changes, but looks ok.

Comment 7

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

(In reply to Olli Pettay [:smaug] from comment #5)
> And I guess this gets called for top level docshells too when they are
> removed from bfcache.
> Hmm, but in that case RemoveRynEntries plays with wrong index. ... except
> that mFiredUnloadEvent is already true, so we don't enter this code path.
> So, I guess we just end up leaving dynamic entries to the session history.
> Could we possibly move RemoveDynEntries outside mFiredUnloadEvent check?

About removing from bfcache part, I think it's non top level docshells being called. The problem is subframe docshells don't have references to root docshells / shistory in this case, since root docshell has called DestroyChildren() soon after CaptureState().

I'm thinking if I should 
1) cache root shistory on CaptureState() or 
2) consider not to set child shell's parent to nullptr in RestoreFromHistory & SetupNewViewer.

Comment 8

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

Attachment: Part 1 v2

MozReview-Commit-ID: 4pBy0vO5yD9

Comment 9

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

Attachment: Part 2 v2

MozReview-Commit-ID: 5YfIoH1f4fD

Comment 10

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

Attachment: Part 1 v3

MozReview-Commit-ID: 4pBy0vO5yD9

Comment 11

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

Comment on attachment 8826586 [details] [diff] [review]
Part 1 v3

I'm taking a different approach for the bfcache evicting case. It's hopefully a better way to avoid the problem mentioned in comment 7.

Comment 12

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

Comment on attachment 8826579 [details] [diff] [review]
Part 2 v2

The test case is extended to reflect the change so I'm hoping to get a 2nd review.

Comment 13

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

+  // The code relies on the assumption that the caller will call
+  // EvictContentViewerForTransaction on all transactions with the same shared
+  // bfcache entry. Otherwise it's still possible that some entries have no
+  // content viewer but the dynamic entries are exist.
+  int32_t index = -1;
+  GetIndexOfEntry(entry, &index);
+  if (index != -1) {
+    nsCOMPtr<nsISHContainer> container(do_QueryInterface(entry));
+    RemoveDynEntries(index, container);
+  }

The comment here should be incorrect. I'll remove that after the review.

Comment 14

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

Attachment: Part 1: Remove dynamic entries on unloading & evicting bfcache

MozReview-Commit-ID: 4pBy0vO5yD9

Comment 15

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

Attachment: Part 2: Add test case

MozReview-Commit-ID: 5YfIoH1f4fD

Comment 16

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

Attachment: Part 3: Fix broken test case

MozReview-Commit-ID: 9KbKIHbbbmX

Comment 17

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

Comment on attachment 8827833 [details] [diff] [review]
Part 1: Remove dynamic entries on unloading & evicting bfcache

The patch tries to remove dynamic subframe entries on FirePageHideNotification & EvictContentViewerForTransaction, and drop all frame entries on reloading.

Comment 18

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

Comment on attachment 8827835 [details] [diff] [review]
Part 3: Fix broken test case

It seems this test was trying to change a nested iframe src, and then reload the outer iframe. It expects the nested iframe would load changed src.

Since I'm making docshell dropping all subframe history entries on reloading it's no longer applicable. The reasonable fix I can think of is to reload only the nested iframe instead.

The reason I want to drop frame entries is because I'm noticing Edge / Chrome / Safari all drop frame entries on reloading with location.reload() or history.go(0).

Comment 19

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

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

Comment 20

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

Comment on attachment 8827833 [details] [diff] [review]
Part 1: Remove dynamic entries on unloading & evicting bfcache

>+  // Internal implementation of nsIDocShell::FirePageHideNotification.
>+  // If aSkipCheckingDynEntries is true, it will not try to remove dynamic
>+  // subframe entries. This is to avoid redundant RemoveDynEntries calls in all
>+  // children docshells.
>+  void FirePageHideNotificationInternal(bool aIsUnload,
>+                                        bool aSkipCheckingDynEntries = false);
I would prefer if the latter param didn't have default value.

>    /**
>+    * Remove dynamic entries found at given index.
>+    *
>+    * @param aIndex
>+    *        Index to remove dynamic entries from. It will be passed to
>+    *        RemoveEntries as aStartIndex.
>+    * @param aContainer (optional)
>+    *        The container to start looking for dynamic entries. Only the
>+    *        dynamic descenders of the container will be removed. If not given,
descendants?

>     // Drop the presentation state before destroying the viewer, so that
>     // document teardown is able to correctly persist the state.
>     ownerEntry->SetContentViewer(nullptr);
>     ownerEntry->SyncPresentationState();
>     viewer->Destroy();
>   }
>+
>+  // When dropping bfcahe, we have to remove associated dynamic entries as well.
bfcache

>+  int32_t index = -1;
>+  GetIndexOfEntry(entry, &index);
>+  if (index != -1) {
>+    nsCOMPtr<nsISHContainer> container(do_QueryInterface(entry));
>+    RemoveDynEntries(index, container);
>+  }

Ah, this is a good catch.



This is all very scary, as all the changes to session history behavior are, but looks reasonable.

Comment 21

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

Attachment: Part 1: Remove dynamic entries on unloading & evicting bfcache

rebase and address review ocmment

Comment 22

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

Attachment: Part 2: Add test case

rebase

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3a4d1cafeaa
Part 1: Remove dynamic entries on unloading & evicting bfcache. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c8cfffcf49
Part 2: Add test case. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/b73f534f457f
Part 3: Fix broken test case. r=smaug

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f3a4d1cafeaa
https://hg.mozilla.org/mozilla-central/rev/e4c8cfffcf49
https://hg.mozilla.org/mozilla-central/rev/b73f534f457f

Comment 26

[Andrei Vaida [:avaida]]

Since this fix has automated coverage, I don't think manual testing would be of much use here.

Samael, if you think Manual QA should instead be looking at this, feel free to flip the qe-verify flag or ni? me directly.

Comment 27

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

Bad me, the test relies on Gecko's current broken Promise scheduling. Fixing.