Opened 5 years ago

Closed 4 years ago

#13035 closed task (fixed)

Make sure our cache isolation works with cache2

Reported by: gk Owned by: mcs
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: ff38-esr, tbb-linkability, tbb-fingerprinting, TorBrowserTeam201507
Cc: arthuredelstein, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Mozilla wrote a new cache back-end which landed in the 32 release cycle and has a bunch of new features like

The new HTTP cache back end has many improvements like request prioritization optimized for first-paint time, ahead of read data preloading to speed up large content load, delayed writes to not block first paint time, pool of most recently used response headers to allow 0ms decisions on reuse or re-validation of a cached payload, 0ms miss-time look-up via an index, smarter eviction policies using frecency algorithm

(http://www.janbambas.cz/new-firefox-http-cache-enabled/)

We should make sure that our cache isolation patches get properly rewritten and no new information leaks occur.

See: https://bugzilla.mozilla.org/show_bug.cgi?id=913806 and https://developer.mozilla.org/en-US/docs/HTTP_Cache for further information.

Child Tickets

Change History (8)

comment:1 Changed 4 years ago by gk

Take https://bugzilla.mozilla.org/show_bug.cgi?id=962334 into account while reviewing the new API.

comment:2 Changed 4 years ago by mcs

Owner: changed from tbb-team to mcs
Status: newassigned

Kathy and I will take this ticket.

comment:3 Changed 4 years ago by mcs

Cc: brade added

comment:4 Changed 4 years ago by mcs

Cc: arthuredelstein added

Our initial conclusion is that the we are in good shape: isolation is being done correctly. This conclusion is mainly based on reviewing the ESR 38 code and the patch that Arthur rebased, along with some time spent in gdb with cache2 logging enabled.

With respect to https://bugzilla.mozilla.org/show_bug.cgi?id=962334, based on code inspection the Mozilla people are correct: the cache entries are immediately (synchronously) removed from hash tables so the old cache items will not be returned to callers.

Kathy and I want to do a little more testing before we close this ticket though.

Does anyone know if the netwerk/test/browser/browser_cacheFirstParty.js tests are working when run on Arthur's ESR 38 branch? Kathy and I have not tried to run those yet.

comment:5 Changed 4 years ago by mcs

Keywords: TorBrowserTeam201506 added
Status: assignedneeds_information

I opened a new ticket for updating the automated tests (and posted a patch). See #16356.

The only remaining question that Kathy and I have for this ticket is whether we should patch nsHttpChannel::DoInvalidateCacheEntry() to use our modified (isolated) cache keys. That would involve passing a non-empty string as the second parameter to cacheStorage->AsyncDoomURI() within that method. This is not new code and not something we patched in the past... and Kathy and I do not understand the implications of not patching it. But it seems like the wrong key is being used there.

Mike, what is your opinion?

comment:6 in reply to:  5 Changed 4 years ago by mikeperry

Replying to mcs:

I opened a new ticket for updating the automated tests (and posted a patch). See #16356.

The only remaining question that Kathy and I have for this ticket is whether we should patch nsHttpChannel::DoInvalidateCacheEntry() to use our modified (isolated) cache keys. That would involve passing a non-empty string as the second parameter to cacheStorage->AsyncDoomURI() within that method. This is not new code and not something we patched in the past... and Kathy and I do not understand the implications of not patching it. But it seems like the wrong key is being used there.

Mike, what is your opinion?

I have not dug through all of the eviction code (there sure are a lot of codepaths involved there), but my initial take is that since Mozilla has been using this same extension key to isolate caching for POST requests, it probably is not a serious issue to omit it, since the original code would have been experiencing similar problems even before our isolation made further use of this key...

It is vaguely concerning, though. We should keep an eye on about:cache after New Identity to ensure that nothing sticks around. I think that is the failure mode I'm most concerned with. I suppose the next most likely issue is a memory leak? Again, this seems like something they should have seen, though..

comment:7 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201507 added; TorBrowserTeam201506 removed

Move over remaining June items to July

comment:8 Changed 4 years ago by mikeperry

Resolution: fixed
Status: needs_informationclosed

I took a look at about:cache, and it seems to be *mostly* behaving OK. Several items have no keys, but they are all 0 size. I also noticed that the network predictor was storing information about resources in pages.

I filed #16625 about the predictor, and I filed #16624 to remind us to ask Mozilla about the cache key if they decide to accept our cache isolation patch.

Otherwise, it seems this is behaving correctly for actual cache isolation. Closing this ticket.

Note: See TracTickets for help on using tickets.