Opened 3 years ago

Closed 3 years ago

#18632 closed defect (fixed)

Rewritten Image Cache isolation patch for TBB/ESR45

Reported by: arthuredelstein Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201604R, ff45-esr, tbb-6.0a5
Cc: Actual Points:
Parent ID: #15197 Points:
Reviewer: Sponsor:

Description

We needed to re-write the Image Cache isolation patch, because the Mozilla introduced a CacheKey that changed how caching works. Fortunately it simplifies our patch somewhat.

Here it is for review:
https://github.com/arthuredelstein/tor-browser/commit/1d3f3223629bc37e584fe6592cef83ab01218fce

Child Tickets

Change History (12)

comment:1 Changed 3 years ago by arthuredelstein

Status: newneeds_review

Original ticket: #6539.

comment:2 Changed 3 years ago by mcs

Overall, this looks good. Kathy and I have a couple of questions:

1) The old patch had the concept that isolation may not be possible, in which case the images where not added to the cache, e.g.

  if (isIsolated)
    PutIntoCache();

Can you explain why that concept is no longer needed? Is it because everything should be isolated (since a document is always available when generating the cache key)?

2) Have you run the automated tests? How much manual testing have you done? Kathy and I did not build or run with the new patch yet.

comment:3 Changed 3 years ago by gk

Keywords: TorBrowserTeam201603R added

comment:4 Changed 3 years ago by gk

Status: needs_reviewneeds_information

comment:5 Changed 3 years ago by gk

Could you explain to me why we don't need to patch anything in nsMenuItemIconX.mm anymore?

comment:6 Changed 3 years ago by gk

Keywords: TorBrowserTeam201604R added; TorBrowserTeam201603R removed

No easy way to do reviews in March anymore.

comment:7 Changed 3 years ago by gk

Keywords: ff45-esr tbb-6.0a5 added

comment:8 in reply to:  2 Changed 3 years ago by arthuredelstein

Replying to mcs:

Overall, this looks good. Kathy and I have a couple of questions:

1) The old patch had the concept that isolation may not be possible, in which case the images where not added to the cache, e.g.

  if (isIsolated)
    PutIntoCache();

Can you explain why that concept is no longer needed? Is it because everything should be isolated (since a document is always available when generating the cache key)?

Now I think it is needed... So I have added a fixup patch in
https://github.com/arthuredelstein/tor-browser/commits/15197+12
hash: ee7e5be3df465a9b32690e767796a3e258ef81b0

2) Have you run the automated tests? How much manual testing have you done? Kathy and I did not build or run with the new patch yet.

I have run the automated regression tests we wrote, and images appear to be cached correctly. I have also visited various sites and images manually, and it appears the images are landing in the cache with the correct first-party key.

comment:9 in reply to:  5 ; Changed 3 years ago by arthuredelstein

Replying to gk:

Could you explain to me why we don't need to patch anything in nsMenuItemIconX.mm anymore?

This was also my mistake -- I added a fixup patch in the same branch,
hash 74800d5a3add016574838af444cffbc5bc304849

comment:10 in reply to:  9 ; Changed 3 years ago by gk

Replying to arthuredelstein:

Replying to gk:

Could you explain to me why we don't need to patch anything in nsMenuItemIconX.mm anymore?

This was also my mistake -- I added a fixup patch in the same branch,
hash 74800d5a3add016574838af444cffbc5bc304849

Looks good to me. mcs/brade are you happy with ee7e5be3df465a9b32690e767796a3e258ef81b0?

comment:11 in reply to:  10 Changed 3 years ago by mcs

Replying to gk:

mcs/brade are you happy with ee7e5be3df465a9b32690e767796a3e258ef81b0?

Yes, this looks good to us.

comment:12 Changed 3 years ago by gk

Resolution: fixed
Status: needs_informationclosed
Note: See TracTickets for help on using tickets.