1710818 - imgLoader::RemoveEntriesFromPrincipal only clears cache entries for current process

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Paul Zühlcke [:pbz]]

Calling imgICache#removeEntriesFromPrincipal only clears entries for the current process. This is unexpected and leads to a bug where the ImageCacheCleaner in ClearDataService.jsm will only clear image cache for the parent process:
https://searchfox.org/mozilla-central/rev/27722db2f164add7047d7db03169966cb806e927/toolkit/components/cleardata/ClearDataService.jsm#222-223,233

Comment 1

[Timothy Nikkel (:tnikkel)]

The image cache is a per process thing, it stays strictly in its own process, and it knows nothing about other processes.

Comment 2

[Timothy Nikkel (:tnikkel)]

Hmm, I guess there is one place that deals with other processes in imgLoader::ClearCache.

Comment 3

[Timothy Nikkel (:tnikkel)]

The fact that everything else with the image cache is inside one process except for that function is not a very good design.

Comment 4

[Paul Zühlcke [:pbz]]

(In reply to Timothy Nikkel (:tnikkel) from comment #1)

The image cache is a per process thing, it stays strictly in its own process, and it knows nothing about other processes.

That may be true for normal cache read/writes. However, we need to be able to reliably clear this cache cross-process. Bug 1274516 already added this capability for clearing all entries. I propose we add a similar method for clearing by principal.

Comment 5

[Timothy Nikkel (:tnikkel)]

Adding a little utilty function outside of the imgLoader class called ClearCacheInAllProcesses and RemoveEntriesFromPrincipalInAllProcesses would be a better interface and lead to less confusing in the future.

Comment 6

[Timothy Nikkel (:tnikkel)]

Or a static function in imgLoader with a name like that, something so that this confusion doesn't arise.

Comment 7

[Paul Zühlcke [:pbz]]

(In reply to Timothy Nikkel (:tnikkel) from comment #6)

Or a static function in imgLoader with a name like that, something so that this confusion doesn't arise.

Sounds good. I'll work on a patch for that.
For Bug 1705033 we'll also need a RemoveEntriesFromBaseDomain which should work cross-process too.

Comment 8

[Paul Zühlcke [:pbz]]

Looking at the code a bit more, I think I prefer the existing implicit pattern, where content process calls will clear only the current process, but parent process calls will be forwarded to all content processes.
https://searchfox.org/mozilla-central/rev/27722db2f164add7047d7db03169966cb806e927/image/imgLoader.cpp#1341

What do you think?

Comment 9

[Timothy Nikkel (:tnikkel)]

I don't like that pattern because every other function etc on imgLoader applies to the local process only. We need something to distinguish these functions as special.

Comment 10

[Timothy Nikkel (:tnikkel)]

Ie at minimum something to make it explicit.

Comment 11

[Paul Zühlcke [:pbz]]

(In reply to Timothy Nikkel (:tnikkel) from comment #10)

Ie at minimum something to make it explicit.

I have a WIP patch which renames clearCache to clearCacheInAllProcesses and adds removeEntriesFromPrincipalInAllProcesses. I've created a similar method for base domain for Bug 1705033.

I'm currently working on a test. Can you advise on how to best test the image cache? Ideally I'd like to use an xpcshell test and explicitly check for entries.
I've found this one: toolkit/components/antitracking/test/xpcshell/test_staticPartition_image.js
However, I'm not sure if it properly tests the image cache, since the HTTP cache is also involved. If an image is in the HTTP cache, but not in the image cache, won't this test still treat it as cached?

Comment 12

[Paul Zühlcke [:pbz]]

There is a similar test which tests cache separation. Maybe I can do something similar:
https://searchfox.org/mozilla-central/rev/443f87caa5fadba920b0382e12874693d6c6133a/image/test/unit/test_private_channel.js

Comment 13

[Paul Zühlcke [:pbz]]

Attachment: Bug 1710818 - Updated imgLoader removeEntriesFromPrincipal to clear entries for all processes. r=tnikkel!

Comment 14

[Timothy Nikkel (:tnikkel)]

To properly test this you would need to spawn more than one process? Because it's currently working if everything is in one process, right? Can you use imgICache::findEntryProperties to directly check whats in the img cache?

(In reply to Paul Zühlcke [:pbz] from comment #11)

However, I'm not sure if it properly tests the image cache, since the HTTP cache is also involved. If an image is in the HTTP cache, but not in the image cache, won't this test still treat it as cached?

Yeah, if the test is checking if the http server gets a request for the image and the http cache doesn't have to send a request to the server, but you should be able to control this by the headers you send with the image.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75b03fc05a3b
Updated imgLoader removeEntriesFromPrincipal to clear entries for all processes. r=tnikkel

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/75b03fc05a3b