Clean up PartialSHistories which are keeping alive bfcache entries which should be evicted

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Created attachment 8814503 [details] [diff] [review]
Part 1: Clean up PartialSHistories which are keeping old SHEntries alive

MozReview-Commit-ID: FY36NdOUM66
Attachment #8814503 - Flags: review?(ehsan)
(Assignee)

Comment 2

2 years ago
Created attachment 8814504 [details] [diff] [review]
Part 2: Add a test that old PartialSHistories are cleaned up correctly

MozReview-Commit-ID: 4Jd5UbmdEru
Attachment #8814504 - Flags: review?(ehsan)
(Assignee)

Comment 3

2 years ago
Comment on attachment 8814503 [details] [diff] [review]
Part 1: Clean up PartialSHistories which are keeping old SHEntries alive

samael, can you also look at this? I made some changes to the groupedshistory stuff and you know how it works best :).
Attachment #8814503 - Flags: feedback?(sawang)
Comment on attachment 8814503 [details] [diff] [review]
Part 1: Clean up PartialSHistories which are keeping old SHEntries alive

Review of attachment 8814503 [details] [diff] [review]:
-----------------------------------------------------------------

I kinda feel I should be responsible for implementing this part for you so you can focus on session store issues. Thanks for your help :)

It generally looks good to me, however I'm hoping you can reconsider the index issue below.

::: dom/ipc/TabChild.cpp
@@ +3091,5 @@
> +  int32_t index, count;
> +  nsresult rv = shistory->GetIndex(&index);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = shistory->GetCount(&count);
> +  NS_ENSURE_SUCCESS(rv, rv);

One annoying thing of nsISHistoryListener is that it's notified before changes are made, so the GetIndex here almost always gets an outdated index which is going to be updated soon by nsSHistory::UpdateIndex(). And since listeners can optionally cancel the operations we can't confirm the index is going to be changed either so we can not just +1/-1 or use GetRequestedIndex instead.

This would indicate parent PartialSHistory almost always holds an incorrect index. I guess it's not a big issue for counting how many frameloaders we want to destroy, but always keeping an incorrect index may potentially be a bug feeder in the future.

(This may be one of the reasons session store has a flush method)

I would personally prefer to add an OnIndexChanged to nsISHistoryListener instead, which would only be notified after index has been updated, much like OnLengthChange does. (I should have named it OnLengthChanged or so for distinguishability, BTW)
Attachment #8814503 - Flags: feedback?(sawang)

Comment 5

2 years ago
I'm gonna hold off reviewing these patches to give Michael a chance to look at comment 4...
(Assignee)

Comment 6

2 years ago
Created attachment 8814996 [details] [diff] [review]
Part 1: Clean up PartialSHistories which are keeping old SHEntries alive

I implemented :freesamael's suggestion of a OnIndexChanged method to accompany the OnLengthChanged method. Hopefully this version of the patch will be less of a potential footgun :).

MozReview-Commit-ID: FY36NdOUM66
Attachment #8814996 - Flags: review?(ehsan)
Attachment #8814996 - Flags: feedback?(sawang)
(Assignee)

Updated

2 years ago
Attachment #8814503 - Attachment is obsolete: true
Attachment #8814503 - Flags: review?(ehsan)
Comment on attachment 8814996 [details] [diff] [review]
Part 1: Clean up PartialSHistories which are keeping old SHEntries alive

Review of attachment 8814996 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Thanks!
Attachment #8814996 - Flags: feedback?(sawang) → feedback+

Comment 8

2 years ago
Comment on attachment 8814996 [details] [diff] [review]
Part 1: Clean up PartialSHistories which are keeping old SHEntries alive

Review of attachment 8814996 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!  A few nits below.

::: docshell/shistory/nsIPartialSHistory.idl
@@ +17,5 @@
>    // The number of entries of its corresponding nsISHistory.
>    [infallible] readonly attribute unsigned long count;
>  
> +  // The current global index of the active shentry in this partialSHistory.
> +  [infallible] readonly attribute long globalIndex;

Elsewhere here index seems to be an unsigned long.  Any reason why this one is signed?

::: docshell/shistory/nsISHistoryListener.idl
@@ +110,5 @@
> +   * Called when nsISHistory::index has been updated. Unlike the other methods
> +   * on this interface, which happen before the modifications are actually done
> +   * and maybe cancellable, this function is called after these modifications.
> +   */
> +   void OnIndexChanged(in long aIndex);

Ditto, any reason why it's signed?

::: dom/base/PartialSHistory.h
@@ +49,5 @@
>    // be used at all.
>    uint32_t mCount;
>  
> +  // The current local index of the active document in this partial SHistory.
> +  uint32_t mIndex;

Please initialize the member.

::: dom/ipc/PBrowser.ipdl
@@ +605,5 @@
>       *
> +     * @param aCount
> +     *        The updated number of entries in child session history
> +     * @param aLocalIndex
> +     *        The local session history index which is loaded.

What about aTruncate? :-)
Attachment #8814996 - Flags: review?(ehsan) → review+

Updated

2 years ago
Attachment #8814504 - Flags: review?(ehsan) → review+
Update dependencies by patch order.
Depends on: 1318766
No longer depends on: 1318767
...I meant bug 1310766.

Andrew could you help to fix my silly change? I couldn't update the dependency due to no permission to bug 1318767.
Flags: needinfo?(overholt)
What should be fixed? This bug should depend on which other bugs?
Flags: needinfo?(sawang)
This way perhaps?
Depends on: 1310766
No longer depends on: 1318766
Flags: needinfo?(overholt)
What permission are you missing, Samael?
bug 1318766 is in sec-group and Samael wasn't CC'ed to that bug.
(In reply to Olli Pettay [:smaug] from comment #12)
> This way perhaps?

Yes, thanks!
I didn't know I was so bad at handling jet lag...
Flags: needinfo?(sawang)
Created attachment 8818797 [details] [diff] [review]
Part 1: Clean up PartialSHistories which are keeping old SHEntries alive

rebased
Created attachment 8818799 [details] [diff] [review]
Part 2: Add a test that old PartialSHistories are cleaned up correctly

rebased
Attachment #8814996 - Attachment is obsolete: true
Attachment #8814504 - Attachment is obsolete: true
Attachment #8818797 - Attachment description: Part 1: Clean up PartialSHistories which are keeping old SHEntries alive, → Part 1: Clean up PartialSHistories which are keeping old SHEntries alive
Attachment #8818797 - Flags: review+
Attachment #8818799 - Attachment description: Part 2: Add a test that old PartialSHistories are cleaned up correctly, → Part 2: Add a test that old PartialSHistories are cleaned up correctly
Attachment #8818799 - Flags: review+
Note it has patch dependency on bug 1310766.
Keywords: checkin-needed

Comment 20

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/529adeb3b22b
Part 1: Clean up PartialSHistories which are keeping old SHEntries alive, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/e578cbec1bdc
Part 2: Add a test that old PartialSHistories are cleaned up correctly, r=ehsan
Keywords: checkin-needed

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/529adeb3b22b
https://hg.mozilla.org/mozilla-central/rev/e578cbec1bdc
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.