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

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: smaug, Assigned: freesamael)

Tracking

(Blocks: 1 bug)

50 Branch
mozilla54
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

Trying to understand all these mystery iframe history stuff...
Assignee: nobody → sawang
Depends on: 1326845
Created attachment 8825385 [details] [diff] [review]
Part 1 v1

MozReview-Commit-ID: 4pBy0vO5yD9
Created attachment 8825386 [details] [diff] [review]
Part 2 v1

MozReview-Commit-ID: 5YfIoH1f4fD
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?
Attachment #8825385 - Flags: review?(bugs)
Attachment #8825386 - Flags: review?(bugs)
No longer depends on: 1326845
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.
Attachment #8825385 - Flags: review?(bugs) → review-
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.
Attachment #8825386 - Flags: review?(bugs) → review+
(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.
Created attachment 8826577 [details] [diff] [review]
Part 1 v2

MozReview-Commit-ID: 4pBy0vO5yD9
Created attachment 8826579 [details] [diff] [review]
Part 2 v2

MozReview-Commit-ID: 5YfIoH1f4fD
Attachment #8825385 - Attachment description: Part 1: Remove dynamic entries on root document unloading → Part 1 v1
Attachment #8825385 - Attachment is obsolete: true
Attachment #8825386 - Attachment description: Part 2: Add test case → Part 2 v1
Attachment #8825386 - Attachment is obsolete: true
Created attachment 8826586 [details] [diff] [review]
Part 1 v3

MozReview-Commit-ID: 4pBy0vO5yD9
Attachment #8826577 - Attachment description: Part 1: Remove dynamic entries on unloading & evicting bfcache → Part 1 v2
Attachment #8826577 - Attachment is obsolete: true
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.
Attachment #8826586 - Flags: review?(bugs)
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.
Attachment #8826579 - Flags: review?(bugs)
+  // 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.
Created attachment 8827833 [details] [diff] [review]
Part 1: Remove dynamic entries on unloading & evicting bfcache

MozReview-Commit-ID: 4pBy0vO5yD9
Created attachment 8827834 [details] [diff] [review]
Part 2: Add test case

MozReview-Commit-ID: 5YfIoH1f4fD
Created attachment 8827835 [details] [diff] [review]
Part 3: Fix broken test case

MozReview-Commit-ID: 9KbKIHbbbmX
Attachment #8826586 - Attachment description: Part 1: Remove dynamic entries on unloading & evicting bfcache → Part 1 v3
Attachment #8826586 - Attachment is obsolete: true
Attachment #8826586 - Flags: review?(bugs)
Attachment #8826579 - Attachment description: Part 2: Add test case → Part 2 v2
Attachment #8826579 - Attachment is obsolete: true
Attachment #8826579 - Flags: review?(bugs)
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.
Attachment #8827833 - Flags: review?(bugs)
Attachment #8827834 - Flags: review?(bugs)
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).
Attachment #8827835 - Flags: review?(bugs)
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.
Attachment #8827833 - Flags: review?(bugs) → review+
Created attachment 8829763 [details] [diff] [review]
Part 1: Remove dynamic entries on unloading & evicting bfcache

rebase and address review ocmment
Created attachment 8829765 [details] [diff] [review]
Part 2: Add test case

rebase
Attachment #8829763 - Flags: review+
Attachment #8829765 - Flags: review+
Attachment #8827833 - Attachment is obsolete: true
Attachment #8827834 - Attachment is obsolete: true
Keywords: checkin-needed

Comment 23

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f3a4d1cafeaa
https://hg.mozilla.org/mozilla-central/rev/e4c8cfffcf49
https://hg.mozilla.org/mozilla-central/rev/b73f534f457f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1326845
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.
Flags: qe-verify-
Bad me, the test relies on Gecko's current broken Promise scheduling. Fixing.
You need to log in before you can comment on or make changes to this bug.