If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

nsSHIstory::RemoveDuplicate should not truncate the SHistory in GroupedSHistory mode

NEW
Unassigned

Status

()

Core
DOM
P3
normal
10 months ago
2 months ago

People

(Reporter: mystor, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

10 months ago
Currently in GroupedSHistory mode, nsSHistory::RemoveDuplicate doesn't truncate the SHistory, instead it removes an entry in place, shortening the current set of SHistories. 

We need to fix GroupedSHistory to support that type of history navigation, which is triggered when, for example, an iframe with history entries is dynamically removed from the DOM.
(Reporter)

Comment 1

10 months ago
Samael, I think you would be the one who would know best how to handle this, as you designed the rest of the GroupedSHistory work. I am not sure how we will handle it but we will need to support offsetting future SHistoryEntries when entries are removed in a previous SHistory entry which may be tricky with the current setup.
Flags: needinfo?(sawang)
We have the same idea that we can add another parameter to OnLengthChanged(), and based on your work in bug 1320391 we can distinguish if the change needs truncation.

Looking at GrouepdSHistory, I'm thinking that we don't really need nsIPartialSHistory.globalIndexOffset since we basically count each partial history in HandleSHistoryUpdate() and GotoIndex(), so my rough plan here is to add an offset parameter to nsIPartialSHistory.onActive(), and remove nsIPartialSHistory.globalIndexOffset so we don't cache outdated values.
Assignee: nobody → sawang
Blocks: 1310761
Depends on: 1320391
Flags: needinfo?(sawang)
Created attachment 8822323 [details] [diff] [review]
Part 1: Don't truncate on removing dynamic history entries

Remove dynamic history entries before swapping to prerendered document,
and don't trunecate / remove following partial history in this case.

Make prerendered id being UUID instead of integer. If we prerendered
again after adopting a prerendered document, the integer based id would
be incorrect.

Always check if frameloader is dead before RequestFrameLoaderClose to
prevent it being invoked multiple times.

MozReview-Commit-ID: IiWlkiUe4hq
Created attachment 8822324 [details] [diff] [review]
Part 2: Add iframe navigation test case

MozReview-Commit-ID: K7taUdBvpx4
Comment on attachment 8822323 [details] [diff] [review]
Part 1: Don't truncate on removing dynamic history entries

Hi Olli,

This bug is about not to truncate following partial histories on removing duplicate or dynamic history entries.

The idea is to make this kind of modifications being non-truncate length changes, and make sure removing dynamic history entries happen before swapping to the prerendered document.

In addition I changed prerender id to UUID, because I found different partial histories in the same grouped history can generate duplicate prerender id since they're using different tab-content.js instances, so if it adopts a prerendered document and then prerender again the result would be incorrect.

Would you help to review this patch?
Attachment #8822323 - Flags: review?(bugs)
Comment on attachment 8822324 [details] [diff] [review]
Part 2: Add iframe navigation test case

I was adding test case of iframe navigation in the existing test_groupedSHistory only to find chrome mochitest doesn't load WebBrowserChrome scripts and it's difficult to test prerender adoption, so I later added the test to browser_prerendering.js. 

Both are refactored to be more readable. The tests for this bug are basically test 3 and test 4 in browser_prerendering.js.
Attachment #8822324 - Flags: review?(bugs)

Updated

9 months ago
Depends on: 1326251

Comment 7

9 months ago
Comment on attachment 8822323 [details] [diff] [review]
Part 1: Don't truncate on removing dynamic history entries

>@@ -10643,17 +10646,20 @@ nsDocShell::InternalLoad(nsIURI* aURI,
>       if (prevViewer) {
> #ifdef DEBUG
>         nsCOMPtr<nsIContentViewer> prevPrevViewer;
>         prevViewer->GetPreviousViewer(getter_AddRefs(prevPrevViewer));
>         NS_ASSERTION(!prevPrevViewer, "Should never have viewer chain here");
> #endif
>         nsCOMPtr<nsISHEntry> viewerEntry;
>         prevViewer->GetHistoryEntry(getter_AddRefs(viewerEntry));
>-        if (viewerEntry == aSHEntry) {
>+        bool sameDoc = false;
>+        if (viewerEntry &&
>+            NS_SUCCEEDED(viewerEntry->SharesDocumentWith(aSHEntry, &sameDoc)) &&
>+            sameDoc) {
>           // Make sure this viewer ends up in the right place
>           mContentViewer->SetPreviousViewer(nullptr);
>           prevViewer->Destroy();
>         }
I have no idea what this change is about. Please explain?


>-  if (browserChrome3 && aCheckForPrerender) {
>-    nsCOMPtr<nsIRunnable> ev =
>+  if (browserChrome3 && aCheckForPrerender && mSessionHistory) {
>+    nsCOMPtr<nsIRunnable> beforeSwapRunnable =
>+      NewRunnableMethod(static_cast<nsSHistory*>(mSessionHistory.get()),
>+                        &nsSHistory::RemoveCurrentDynEntries);
>+    nsCOMPtr<nsIRunnable> failureRunnable =
...
>     rv = browserChrome3->ShouldSwitchToPrerenderedDocument(
>-      aURI, mCurrentURI, nullptr, ev, &shouldSwitch);
>+      aURI, mCurrentURI, beforeSwapRunnable, nullptr, failureRunnable, &shouldSwitch);
>     if (NS_SUCCEEDED(rv) && shouldSwitch) {
>       return NS_OK;
>     }
>   }
> 
>   // Whenever a top-level browsing context is navigated, the user agent MUST
>   // lock the orientation of the document to the document's default
>   // orientation. We don't explicitly check for a top-level browsing context
...
>+NS_IMETHODIMP_(void)
>+nsSHistory::RemoveCurrentDynEntries()
>+{
>+  nsCOMPtr<nsISHEntry> currentEntry;
>+  GetEntryAtIndex(mIndex, false, getter_AddRefs(currentEntry));
>+  nsCOMPtr<nsISHContainer> container = do_QueryInterface(currentEntry);
>+  AutoTArray<nsID, 16> toBeRemovedEntries;
>+  if (container) {
>+    GetDynamicChildren(container, toBeRemovedEntries, true);
>+  }
>+  if (toBeRemovedEntries.Length()) {
>+    RemoveEntries(toBeRemovedEntries, mIndex);
>+  }
>+}
Ok, so this brings similar to normal session history handling.
But, now after 6 years of implementing RemoveDynEntries, I think it is actually buggy when dealing with bfcached documents.
The issue is that when going back the dynamically added iframe is still there in docshell tree, but session history doesn't have entry for it. And if
now some new pages are loaded to the iframe, session history for it seems to be all broken. 
Could you possibly investigate this issue (filed Bug 1326251)? Or if not, I'll try to find time


> NS_IMETHODIMP
>-nsSHistory::OnPartialSHistoryActive(int32_t aGlobalLength, int32_t aTargetIndex)
>+nsSHistory::OnPartialSHistoryActive(int32_t aOffset,
>+                                    int32_t aGlobalLength,
>+                                    int32_t aTargetIndex)
> {
>   NS_ENSURE_TRUE(mRootDocShell && mIsPartial, NS_ERROR_UNEXPECTED);
>+  NS_ENSURE_TRUE(aOffset >= 0, NS_ERROR_ILLEGAL_VALUE);
> 
>-  int32_t extraLength = aGlobalLength - mLength - mGlobalIndexOffset;
>+  int32_t extraLength = aGlobalLength - mLength - aOffset;
>   NS_ENSURE_TRUE(extraLength >= 0, NS_ERROR_UNEXPECTED);
> 
>-  if (extraLength != mEntriesInFollowingPartialHistories) {
>-    mEntriesInFollowingPartialHistories = extraLength;
>-  }
>+  mGlobalIndexOffset = aOffset;
>+  mEntriesInFollowingPartialHistories = extraLength;
Could you rename aOffset to aGlobalOffset or so, to make this a bit easier to read.
Same also in .idl


> GroupedSHistory::PurgePrerendering()
> {
>   nsTArray<PrerenderingHistory> histories = Move(mPrerenderingHistories);
>   // Remove the frameloaders which are owned by the prerendering history, and
>   // remove them from mPrerenderingHistories.
>   for (uint32_t i = 0; i < histories.Length(); ++i) {
>     nsCOMPtr<nsIFrameLoader> loader;
>     histories[i].mPartialHistory->GetOwnerFrameLoader(getter_AddRefs(loader));
>-    if (loader) {
>+    if (loader && !loader->GetIsDead()) {
>       loader->RequestFrameLoaderClose();
Why is this change needed? Should loader check in RequestFrameLoaderClose whether it can actually close?

> GroupedSHistory::CloseInactiveFrameLoaderOwners()
> {
>   MOZ_ASSERT(mIndexOfActivePartialHistory >= 0);
>   // Remove inactive frameloaders which are participating in the grouped shistory
>   for (uint32_t i = 0; i < mPartialHistories.Length(); ++i) {
>     if (i != static_cast<uint32_t>(mIndexOfActivePartialHistory)) {
>       nsCOMPtr<nsIFrameLoader> loader;
>       mPartialHistories[i]->GetOwnerFrameLoader(getter_AddRefs(loader));
>-      loader->RequestFrameLoaderClose();
>+      if (loader && !loader->GetIsDead()) {
>+        loader->RequestFrameLoaderClose();
ditto

> {
>   for (uint32_t i = 0; i < mPrerenderingHistories.Length(); ++i) {
>     if (mPrerenderingHistories[i].mId == aId) {
>       nsCOMPtr<nsIPartialSHistory> partialHistory = mPrerenderingHistories[i].mPartialHistory;
>       nsCOMPtr<nsIFrameLoader> fl;
>       partialHistory->GetOwnerFrameLoader(getter_AddRefs(fl));
>-      if (fl) {
>+      if (fl && !fl->GetIsDead()) {
>         fl->RequestFrameLoaderClose();
and here

> PartialSHistory::GetGlobalIndexOffset(uint32_t* aResult)
> {
>   if (!aResult) {
>     return NS_ERROR_INVALID_POINTER;
>   }
> 
>-  // If we have direct reference to nsISHistory, simply pass through.
>-  nsCOMPtr<nsISHistory> shistory(GetSessionHistory());
>-  if (shistory) {
>-    int32_t offset;
>-    nsresult rv = shistory->GetGlobalIndexOffset(&offset);
>-    if (NS_FAILED(rv) || offset < 0) {
>-      *aResult = 0;
>-      return NS_ERROR_FAILURE;
>-    }
>-    *aResult = offset;
>-    return NS_OK;
>-  }
Why can we remove this?


>-TabChild::RecvNotifyPartialSHistoryActive(const uint32_t& aGlobalLength,
>+TabChild::RecvNotifyPartialSHistoryActive(const uint32_t& aOffset,
>+                                          const uint32_t& aGlobalLength,
>                                           const uint32_t& aTargetLocalIndex)
aOffset could be aGlobalOffset also here.



I'd like to see a new patch. And possibly bug 1326251 fixed before this, since it may affect to this one too. But if that is hard, I guess we could take this first.
Attachment #8822323 - Flags: review?(bugs) → review-

Comment 8

9 months ago
Comment on attachment 8822324 [details] [diff] [review]
Part 2: Add iframe navigation test case

This is pretty much impossible to review. Changing coding style to use async-await _and_ also adding some new tests.
Could you please split the new tests into separate patch?

Is there any particular reason to use async-await here?
I mean, fine to use it in new test files and so, but why to convert existing test to use it?
Attachment #8822324 - Flags: review?(bugs) → review-

Comment 9

9 months ago
But feel free to convert the tests to async-await if you think it makes code significantly easier to read. Just do the conversion and adding new tests in separate patches.
Not actively working on this.
Assignee: sawang → nobody

Updated

2 months ago
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.