Right now there is only one caller of CurrentStatusTracker() in RasterImage, but there are multiple places where we've effectively manually inlined the contents of that method. We should change those places to call CurrentStatusTracker. CurrentStatusTracker also needs to assert that we hold the lock when it's called (since the caller is probably about to do something unsafe if we don't). During the process of making this change, we should identify any callers that aren't holding the lock and triage them as needed.

Delightfully, this ended up getting done as part of bug 867755. All that's left to do here is add the assertion to CurrentStatusTracker() and fix any issues that may arise.

Here's a patch that adds the assertion. Try job here: https://tbpl.mozilla.org/?tree=Try&rev=2bacaf376871

Comment on attachment 8335116 [details] [diff] [review] Assert that we own the decoding mutex in CurrentStatusTracker. This seems pretty orange. So clearing review request.

Oh dear, that is really bad. We are doing probably something that we shouldn't be. I'm looking at the stack traces now; hopefully this will be an easy fix.

It's all the same function: > XUL!mozilla::image::RasterImage::OnImageDataComplete(nsIRequest*, nsISupports*, tag_nsresult, bool) [Mutex.h:2bacaf376871 : 104 + 0xb] Looks like fallout from bug 867755. Taking to look to see about putting together a fix.

It's a simple fix. This is exactly why we needed this assertion.

Updated the commit message to reflect the addition of part 1. Try job for both parts here: https://tbpl.mozilla.org/?tree=Try&rev=d6950292e5c3

Looks like we found another one. This time all of the stacks involve: > XUL!mozilla::image::RasterImage::DoError() [Mutex.h:d6950292e5c3 : 104 + 0xb] I'm testing a patch to fix this one as well.

Fixing this one was a little nastier. There's not one policy that can work everywhere since some DoError callers hold the lock and some don't. I ended up creating DoErrorOutsideLock and requiring that direct callers of DoError hold the lock. The reverse approach was taken for CONTAINER_ENSURE_* - I added CONTAINER_ENSURE_SUCCESS_ALREADY_LOCKED for the case where the lock is already held, while the normal CONTAINER_ENSURE_* macros now take the lock. In each case (DoError*, CONTAINER_ENSURE_*) the approach taken was motivated by which kind of usage was most frequent.

Updated the commit message to reflect the addition of part 2. This time I tested OS X locally pretty thoroughly, at least for image/tests/*. This new try jobs is thus for Windows instead: https://tbpl.mozilla.org/?tree=Try&rev=5fe997205797

Looks like try is green at this point, so I think we've found all the misuse of CurrentStatusTracker.

Comment on attachment 8336445 [details] [diff] [review] (Part 2) - Make sure we hold the decode lock when calling DoError. >@@ -1230,17 +1248,17 @@ RasterImage::SetSize(int32_t aWidth, int >- DoError(); >+ DoErrorOutsideLock(); Even if we do not hold the mutex here, shouldn't we be? We are accessing mDecoder and mError.

(In reply to Timothy Nikkel (:tn) from comment #12) > Even if we do not hold the mutex here, shouldn't we be? We are accessing > mDecoder and mError. Hmm, yes, I concur. And in fact we are holding the mutex, because RasterImage::SetSize is always called from Decoder::SetSizeOnImage which is always called from RasterImage::FinishedSomeDecoding, where we always have the lock. I'll update the patch to use DoError instead of DoErrorOutsideLock in SetSize, and add an assertion that we hold the lock there. This will require a new try job, alas. =) Good catch though!

Updated patch.

New try job here: https://tbpl.mozilla.org/?tree=Try&rev=60259522219c

Note that the fact that try didn't catch the issue you pointed out indicates that we don't have adequate test coverage for this issue. Unfortunately I don't see a realistic way to create one. Our image code is just not constructed in a testable manner. We should really do something about this. =( (But not in this bug.)

Thanks for the review, Timothy! The patch will need a rebase before landing.

Trivial rebase against bug 943803.

Rebase against bug 943803. This is a huge change that removes 95% of the content in the previous version of the patch (since the rebase makes it obsolete), so rerequesting review.

Trivial rebase against bug 943803.

Here's a new try job. Given the amount of churn in RasterImage lately, I felt that a full run was warranted. https://tbpl.mozilla.org/?tree=Try&rev=43c73eb248cd

All seems well. Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/73c56ad7bc59 https://hg.mozilla.org/integration/mozilla-inbound/rev/8be832270608 https://hg.mozilla.org/integration/mozilla-inbound/rev/4ed374e9a290

https://hg.mozilla.org/mozilla-central/rev/73c56ad7bc59 https://hg.mozilla.org/mozilla-central/rev/8be832270608 https://hg.mozilla.org/mozilla-central/rev/4ed374e9a290