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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

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

Attachment: Part 1: Clean up PartialSHistories which are keeping old SHEntries alive

MozReview-Commit-ID: FY36NdOUM66

Comment 2

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

Attachment: Part 2: Add a test that old PartialSHistories are cleaned up correctly

MozReview-Commit-ID: 4Jd5UbmdEru

Comment 3

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

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 :).

Comment 4

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

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)

Comment 5

[(no longer active)]

I'm gonna hold off reviewing these patches to give Michael a chance to look at comment 4...

Comment 6

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

Attachment: 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

Comment 7

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

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!

Comment 8

[(no longer active)]

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? :-)

Comment 9

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

Update dependencies by patch order.

Comment 10

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

...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.

Comment 11

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

What should be fixed? This bug should depend on which other bugs?

Comment 12

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

This way perhaps?

Comment 13

[Andrew Overholt [:overholt]]

What permission are you missing, Samael?

Comment 14

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

bug 1318766 is in sec-group and Samael wasn't CC'ed to that bug.

Comment 15

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

(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...

Comment 16

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dbf55bef575631faac72522394d7e2577b6e1aa&group_state=expanded

Comment 17

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

Attachment: Part 1: Clean up PartialSHistories which are keeping old SHEntries alive

rebased

Comment 18

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

Attachment: Part 2: Add a test that old PartialSHistories are cleaned up correctly

rebased

Comment 19

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

Note it has patch dependency on bug 1310766.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/529adeb3b22b
https://hg.mozilla.org/mozilla-central/rev/e578cbec1bdc