Opened 8 years ago

Closed 7 years ago

#8628 closed defect (fixed)

Inconsistent image cache key usage

Reported by: mikeperry Owned by: mikeperry
Priority: High Milestone:
Component: Firefox Patch Issues Version:
Severity: Keywords: MikePerry201304, tbb-linkability
Cc: gk Actual Points: 2
Parent ID: Points:
Reviewer: Sponsor:

Description

While puzzling over our three image-cache related crash bugs (#8601, #8559, and #8618), I noticed that the cache key usage was inconsistent in one of the codepaths that caused a crash: We had reversed the first party URI vs the originating URI arguments on one of the calls to GetCacheKey().

I am filing a new ticket because I'm not sure this is the real source of all of the crashes, but it is certainly a bug on its own (possibly resulting in cache isolation flaws and potential third party linkability).

If by some miracle this patch actually does fix all the crashes, we'll just dup them to this bug.

Child Tickets

Attachments (1)

0024-Isolate-the-Image-Cache-per-url-bar-domain.patch (36.9 KB) - added by mikeperry 8 years ago.
Updated Image Isolation patch with fixed GetCacheKey arguments

Download all attachments as: .zip

Change History (11)

Changed 8 years ago by mikeperry

Updated Image Isolation patch with fixed GetCacheKey arguments

comment:1 Changed 8 years ago by mikeperry

If you prefer direct git branches, this patch plus a bunch of other fun goodies are in mikeperry/2.4-next.

comment:2 Changed 8 years ago by gk

Okay, the good news is that I did not get a single crash anymore. What makes me quite nervous, though, are loads of assertions of the type

###!!! ASSERTION: More UnblockOnload() calls than BlockOnload() calls; dropping call: 'Not Reached', file /home/firefox64/TBB/debug-build/mozilla-esr17/content/base/src/nsDocument.cpp, line 7174
###!!! ASSERTION: Shouldn't have a count of zero here, since we stabilized in PostUnblockOnloadEvent: 'mOnloadBlockCount != 0', file /home/firefox64/TBB/debug-build/mozilla-esr17/content/base/src/nsDocument.cpp, line 7224
###!!! ASSERTION: Double UnblockOnload!?: 'mCurrentRequestFlags & REQUEST_BLOCKS_ONLOAD', file /home/firefox64/TBB/debug-build/mozilla-esr17/content/base/src/nsImageLoadingContent.cpp, line 598

That happens especially on pages with lots of images. While I have not tested the old patch (or a default Fx build) again I am quite confident that these assertions would have caught my attention if they would have been there. But maybe I am wrong here, dunno...

comment:3 Changed 8 years ago by mikeperry

I am pretty sure I did see those while trying to reproduce the crash before writing the patch, along with numerous other assertions... The debug builds are quite noisy wrt assertions and warnings. I wonder if that is our patches fault, or due to our extensions, or ??.

Did you just try the one patch or the whole git branch?

comment:4 in reply to:  3 Changed 8 years ago by gk

Replying to mikeperry:

I am pretty sure I did see those while trying to reproduce the crash before writing the patch, along with numerous other assertions... The debug builds are quite noisy wrt assertions and warnings. I wonder if that is our patches fault, or due to our extensions, or ??.

Okay then. I'll dig around a bit and open a new ticket if I find something.

Did you just try the one patch or the whole git branch?

Just the patch to avoid weird bugs possibly caused by your goodies (not that there are any (bugs))... :)

comment:5 Changed 8 years ago by mikeperry

Awesome. Best case scenario. The attached patch by itself is extremely simple. I would give you whatever odds you wanted on whatever amount you wanted that it did not cause *every* assert and warning I see in the debug logs.. ;)

Keep an eye out for abort-inducing asserts though of course. (Let's hope they didn't magically change how *all* asserts behave in the 17.0.5-ESR point release, too :/).

comment:6 in reply to:  5 Changed 8 years ago by gk

Replying to mikeperry:

Awesome. Best case scenario. The attached patch by itself is extremely simple. I would give you whatever odds you wanted on whatever amount you wanted that it did not cause *every* assert and warning I see in the debug logs.. ;)

Ha! Sounds like "I just changed one line of code and that would never ever break the test suite!". But you were right, that's a Mozilla issue. Sorry for the noise.

Keep an eye out for abort-inducing asserts though of course. (Let's hope they didn't magically change how *all* asserts behave in the 17.0.5-ESR point release, too :/).

There are other assertions but I don't think they are related to Tor Browser patches/TBB related code.

comment:7 Changed 8 years ago by mikeperry

Actual Points: 2
Resolution: fixed
Status: newclosed

This has been fine for me for a while, so I merged it. I suppose at the end of the month we can decide if it closes the other 3 crash bugs.

comment:8 Changed 7 years ago by monka_01

Resolution: fixed
Status: closedreopened

It seems that this does not fix the whole problem. reopening it.

comment:9 Changed 7 years ago by monka_01

Explanation: I reopened this bug, because it seems to be not fixed completely. Please have a look at Ticket #8895 for further crash logs and explanation.

comment:10 Changed 7 years ago by mikeperry

Resolution: fixed
Status: reopenedclosed

This bug is a specific issue that was in fact fixed.

Note: See TracTickets for help on using tickets.