This is a follow-up of the finding in bug 1339707 comment 10 to have the use of nsExpirationTracker in docshell/ being labeled.

Comment on attachment 8862761 [details] Bug 1347823 - Part 1: Reorder includes and data members. https://reviewboard.mozilla.org/r/134632/#review137618 Just random thought, is there some tool to detect when member variables are ordered in a bit silly way?

Comment on attachment 8862762 [details] Bug 1347823 - Part 2: Move HistoryTracker to nsSHistory and bind its event target to the TabGroup. https://reviewboard.mozilla.org/r/134634/#review137620 I think this could be simplified a bit without UniquePtr usage. Or explain why it is needed. ::: docshell/shistory/nsSHistory.h:63 (Diff revision 1) > + shistory->EvictExpiredContentViewerForEntry(aObj); > + } > + } > + > + private: > + nsWeakPtr mSHistory; Why nsWeakPtr? SHistory object itself controls the lifetime of HistoryTracker, so using raw pointer here should be fine. ::: docshell/shistory/nsSHistory.h:122 (Diff revision 1) > // If aKeepNext is true, aIndex is compared to aIndex + 1, > // otherwise comparison is done to aIndex - 1. > bool RemoveDuplicate(int32_t aIndex, bool aKeepNext); > > + // Track all bfcache entries and evict on expiration. > + mozilla::UniquePtr<HistoryTracker> mHistoryTracker; Why this needs to be UniquePtr? Why not just HistoryTracker mHistoryTracker?

Comment on attachment 8862763 [details] Bug 1347823 - Part 3: Add a test case. https://reviewboard.mozilla.org/r/134636/#review137622

Comment on attachment 8862762 [details] Bug 1347823 - Part 2: Move HistoryTracker to nsSHistory and bind its event target to the TabGroup. https://reviewboard.mozilla.org/r/134634/#review137620 > Why this needs to be UniquePtr? Why not just HistoryTracker mHistoryTracker? mHistoryTracker has to be lazy-init on nsSHistory::SetRootDocShell, otherwise there's no suitable nsIEventTarget I can pass to the constructor of nsExpirationTracker.

Comment on attachment 8862762 [details] Bug 1347823 - Part 2: Move HistoryTracker to nsSHistory and bind its event target to the TabGroup. https://reviewboard.mozilla.org/r/134634/#review137620 > mHistoryTracker has to be lazy-init on nsSHistory::SetRootDocShell, otherwise there's no suitable nsIEventTarget I can pass to the constructor of nsExpirationTracker. I'm dropping this issue as explained above. Let me know if you still have concerns.

Comment on attachment 8862762 [details] Bug 1347823 - Part 2: Move HistoryTracker to nsSHistory and bind its event target to the TabGroup. https://reviewboard.mozilla.org/r/134634/#review138370 ::: docshell/shistory/nsSHistory.h:59 (Diff revision 2) > + RemoveObject(aObj); > + mSHistory->EvictExpiredContentViewerForEntry(aObj); > + } > + > + private: > + nsSHistory* mSHistory; Add a comment that nsSHistory owns HistoryTracker, so it always outlives HistoryTracker object.

seems there is a open issue for part 2 that need to be fixed first.

(In reply to Carsten Book [:Tomcat] from comment #15) > seems there is a open issue for part 2 that need to be fixed first. Sorry I forgot to mark it fixed. It's fixed in revision 3.

np, landed!

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d2fbb471ba83 Part 1: Reorder includes and data members. r=smaug https://hg.mozilla.org/integration/autoland/rev/f8bebf994dbf Part 2: Move HistoryTracker to nsSHistory and bind its event target to the TabGroup. r=smaug https://hg.mozilla.org/integration/autoland/rev/05a2a58f887e Part 3: Add a test case. r=smaug

https://hg.mozilla.org/mozilla-central/rev/d2fbb471ba83 https://hg.mozilla.org/mozilla-central/rev/f8bebf994dbf https://hg.mozilla.org/mozilla-central/rev/05a2a58f887e