1343603 - Clearing history should clear cached session store history data as well

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jan Henning [:JanH]]

Discovered through bug 1342820 comment 2.

Comment 1

[Jan Henning [:JanH]]

Attachment: Bug 1343603 - Part 1 - Immediately clear cached session store history data when clearing history.

The session store keeps a serialised copy of a tab's session history around so that the gathering of the data (which can be somewhat expensive) can be decoupled from writing it to disk.

When the user clears Firefox's history, we therefore need to discard this data as well (except for the currently open entry), so it doesn't stick around until the next time some navigation/history change occurs in that tab. Otherwise, if Firefox or just the tab is closed before the purged state has been re-collected by the session store, the supposedly purged session history will resurface when the tab is restored again.

Bug 1337940 means that we'll now catch the history notifications caused by the session history being purged, however
- we still need to handle zombified tabs separately, since as far as the rest of Gecko is concerned, those simply consist of a plain "about:blank" browser with the true state being stashed away in the session store data, so the purging of the live session history data won't have any real effect
- the history purging on the tab happens after the session store receives the "browser:purge-session-history" notification, meaning that these changes received through the regular history notifications won't get written to disk immediately

Therefore we now explicitly purge the session history data of all tabs in our notification handler, so this state can then immediately be saved to disk.

Review commit: https://reviewboard.mozilla.org/r/116720/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116720/

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1343603 - Part 2 - Test that clearing history clears the cached session store data, too.

Review commit: https://reviewboard.mozilla.org/r/116722/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116722/

Comment 3

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Sounds like 53 is also affected. Tracking since it's a privacy issue

Comment 4

[Andrzej Hunt :ahunt]

Comment on attachment 8843024 [details]
Bug 1343603 - Part 1 - Immediately clear cached session store history data when clearing history.

https://reviewboard.mozilla.org/r/116720/#review119346

Comment 5

[Andrzej Hunt :ahunt]

Comment on attachment 8843025 [details]
Bug 1343603 - Part 2 - Test that clearing history clears the cached session store data, too.

https://reviewboard.mozilla.org/r/116722/#review119348

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d31fb49a844
Part 1 - Immediately clear cached session store history data when clearing history. r=ahunt
https://hg.mozilla.org/integration/autoland/rev/80934b637165
Part 2 - Test that clearing history clears the cached session store data, too. r=ahunt

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d31fb49a844
https://hg.mozilla.org/mozilla-central/rev/80934b637165

Comment 8

[Gerry Chang [:gchang]]

Hi :JanH, 
Since this bug also affects 54, do you think it's worth uplifting to 54 if this patch is not too risky?

Comment 9

[Jan Henning [:JanH]]

Comment on attachment 8843024 [details]
Bug 1343603 - Part 1 - Immediately clear cached session store history data when clearing history.

Approval Request Comment
[Feature/Bug causing the regression]: Clearing of private data
[User impact if declined]: Supposedly purged session history of open tabs might resurface if a tab/Firefox is closed after clearing history (with no navigation in between) and later restored.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Tested with Nightly build and found to work.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: All tabs have minimal session store data with session history available, so it should be safe to iterate over them and clear everything except the current session history entry.
[String changes made/needed]: none

Comment 10

[Jan Henning [:JanH]]

Comment on attachment 8843025 [details]
Bug 1343603 - Part 2 - Test that clearing history clears the cached session store data, too.

Test for Part 1

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8843024 [details]
Bug 1343603 - Part 1 - Immediately clear cached session store history data when clearing history.

Fix a private issue related to session store history data and include tests. Aurora54+.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/600604082021
https://hg.mozilla.org/releases/mozilla-aurora/rev/147c73709841

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8843024 [details]
Bug 1343603 - Part 1 - Immediately clear cached session store history data when clearing history.

The user impact seems pretty severe, Beta53+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f83bd2c01258
https://hg.mozilla.org/releases/mozilla-beta/rev/32d6ec3dc58f

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for test_session_clear_history.html failures. Looks like the new test is going to need some tweaking for Beta.
https://hg.mozilla.org/releases/mozilla-beta/rev/a1d99033e1f7

https://treeherder.mozilla.org/logviewer.html#?job_id=82904410&repo=mozilla-beta&lineNumber=2891

Comment 16

[Jan Henning [:JanH]]

Ah right, because of bug 1333046 zombification needs to be handles differently. I'll look into it.

Comment 17

[Jan Henning [:JanH]]

Attachment: Bug 1343603 - Part 2 - BETA.patch

Fixed tab zombification to use the MemoryObserver on Beta

Comment 18

[Jan Henning [:JanH]]

Attachment: Bug 1343603 - Part 2 - BETA.patch

Fixed a= for Beta in commit message.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3e68000c437c
https://hg.mozilla.org/releases/mozilla-beta/rev/6938c2c43242