1627022 - Clear about:home cache when appropriate when doing sanitization

Status: RESOLVED FIXED

(Firefox :: New Tab Page, task, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

The about:home cache contains information about user history and activity in the Highlights section. We should make sure to clear that from the disk when appropriate inside of Sanitizer.jsm.

Comment 1

[Scott [:thecount] Downe]

Hey Mike, if there was an action in https://searchfox.org/mozilla-central/source/browser/components/newtab/common/Actions.jsm for clearing cache, that newtab feeds could listen to, and use to clear their cache. Would Sanitizer.jsm have what it needs to trigger that action?

Or is it more complex than that?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to Scott [:thecount] Downe from comment #1)

Sorry, this question dropped off my radar. I don't think the feeds need to know this - to clear the cache, AboutHomeStartupCache.clearCache in BrowserGlue.jsm needs to be called. I suspect Sanitizer will have to call that directly, or AboutHomeStartupCache will need to listen to some observer notification to know when to do it.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Hey ewright, I know you've recently done work around cookie cleanup... are you familiar at all with Sanitizer.jsm?

What I want to do here is make sure that the about:home startup cache gets cleared when the user has indicated that there's some past state (history, cookies, cache, etc) that they want to clear. Since about:home can contain a composite of past state (history, bookmarks, screenshots, etc), it probably makes sense to be fairly aggressive about clearing the cache any time sanitization occurs. Does that make sense? And if so, do you think Sanitizer.jsm should call AboutHomeStartupCache.clearCache directly, or should there be some indirection via observer notifications or something?

Comment 5

[Erica Wright [:ewright]]

That all sounds reasonable to me, I would probably choose to clear the cache directly, though I wouldn't consider myself an expert on Sanitizer.jsm, so there may be some reasons not to.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1627022 - Add a new cleaner that sanitizes the about:home startup cache when appropriate. r?baku!

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1627022 - Add a test that makes sure we clear the about:home startup cache when sanitizing. r?k88hudson!

Depends on D82089

Comment 8

[Mike Conley (:mconley) (:⚙️)]

ni?ing baku for johannh's question in https://phabricator.services.mozilla.com/D82089#inline-470365

Comment 9

[Andrea Marchesini [:baku]]

I don't understand why we want to touch nsIClearDataService and add a new cleaner for about:home. Can you tell me more?
Can we simply use existing cleaers and call: Services.clearData.deleteDataFromPincipal(the_about_home_principal) ?
In particular, why https://phabricator.services.mozilla.com/D82091 fails without the first patch?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

(In reply to Andrea Marchesini [:baku] from comment #9)

In particular, why https://phabricator.services.mozilla.com/D82091 fails without the first patch?

It fails because clearing the downloads, places or history data doesn't clear out the HTTP cache, which is where the about:home startup cache lives.

I don't understand why we want to touch nsIClearDataService and add a new cleaner for about:home. Can you tell me more?
Can we simply use existing cleaers and call: Services.clearData.deleteDataFromPincipal(the_about_home_principal) ?

We could do this - we could update the DownloadsCleaner, HistoryCleaner and SessionHistoryCleaner to also clear the about:home HTTP cache entry when they clear. My only concern with this is:

1. We're distributing the responsibility of clearing the about:home startup cache over several cleaners. Suppose in the future that the about:home document is composed of something new that we want to clear... we'll have to remember to add the about:home clearing to that cleaner as well. I suspect that's easier to forget if there isn't a dedicated cleaner, but that's conjecture. As it spreads out the responsibility for clearing the about:home startup cache, it also can spread out things like comments / inline documentation to make it clearer what's going on. The comment at line 1112 in ClearDataService.jsm - where would that live if cleaning was spread over multiple cleaners?
2. All of the pre-existing cleaners apply to and work on mobile. The about:home startup cache is Desktop-only. It seems simpler to condition the running of the cleaner on the platform when it's separate, and not individually within several cleaners.

This seemed the simplest and most "encapsulated" solution, but to be honest, I'm willing to acquiesce to the folks who are more likely to work on / maintain this code. If you think it makes more sense to spread out cleaning across the DownloadsCleaner, HistoryCleaner and SessionHistoryCleaner, I can do that - just say the word.

Comment 11

[Andrea Marchesini [:baku]]

1. We're distributing the responsibility of clearing the about:home startup cache over several cleaners. Suppose in the future that the about:home document is composed of something new that we want to clear... we'll have to remember to add the about:home clearing to that cleaner as well. I suspect that's easier to forget if there isn't a dedicated cleaner, but that's conjecture.

we have a similar issue with forget-about-site. In order to do the "right" thing we added a special flag here:
https://searchfox.org/mozilla-central/rev/7ec7ee4a9bde171ba195ab46ed6077e4baaef34d/toolkit/components/cleardata/nsIClearDataService.idl#251-256

we can add: CLEAR_ABOUTHOME = CLEAR_HISTORY | CLER_DOWNLOAD...;
and then you call: Services.clearData.deleteDataFromPincipal(the_about_home_principal, CLEAR_ABOUT_HOME);

2. All of the pre-existing cleaners apply to and work on mobile. The about:home startup cache is Desktop-only. It seems simpler to condition the running of the cleaner on the platform when it's separate, and not individually within several cleaners.

You still need to call the cleanup in the sanitizer.jsm. Right? Can we add the if(desktop){}-stmt there?

Comment 12

[Mike Conley (:mconley) (:⚙️)]

ni?ing baku for https://phabricator.services.mozilla.com/D82089#2531810.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5510a39f3ed
Add a new cleaner that sanitizes the about:home startup cache when appropriate. r=baku
https://hg.mozilla.org/integration/autoland/rev/e8a1d0ac7b50
Add a test that makes sure we clear the about:home startup cache when sanitizing. r=k88hudson

Comment 14

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/d5510a39f3ed
https://hg.mozilla.org/mozilla-central/rev/e8a1d0ac7b50