Inconsistent image cache key usage
While puzzling over our three image-cache related crash bugs (#8601 (closed), #8559 (closed), and #8618 (closed)), 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.
Activity
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...
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?
-
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))... :)
-
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 :/).
-
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.
Explanation: I reopened this bug, because it seems to be not fixed completely. Please have a look at Ticket #8895 (closed) for further crash logs and explanation.
Trac:
Username: monka_01mentioned in issue #8708 (closed)
mentioned in issue #8726 (closed)
mentioned in issue #8856 (closed)
mentioned in issue #8895 (closed)