862909 - Use CurrentStatusTracker() more in RasterImage and make the idiom safer

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

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.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: Assert that we own the decoding mutex in CurrentStatusTracker.

Here's a patch that adds the assertion.

Try job here:
https://tbpl.mozilla.org/?tree=Try&rev=2bacaf376871

Comment 3

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8335116 [details] [diff] [review]
Assert that we own the decoding mutex in CurrentStatusTracker.

This seems pretty orange. So clearing review request.

Comment 4

[Seth Fowler [:seth] [:s2h]]

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.

Comment 5

[Seth Fowler [:seth] [:s2h]]

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.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Call CurrentStatusTracker() with the decode lock held in OnImageDataComplete.

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

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Assert that we own the decoding mutex in CurrentStatusTracker.

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

Comment 8

[Seth Fowler [:seth] [:s2h]]

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.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Make sure we hold the decode lock when calling DoError.

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.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Assert that we own the decoding mutex in CurrentStatusTracker.

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

Comment 11

[Seth Fowler [:seth] [:s2h]]

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

Comment 12

[Timothy Nikkel (:tnikkel)]

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.

Comment 13

[Timothy Nikkel (:tnikkel)]

comment 12

Comment 14

[Seth Fowler [:seth] [:s2h]]

(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!

Comment 15

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Make sure we hold the decode lock when calling DoError.

Updated patch.

Comment 16

[Seth Fowler [:seth] [:s2h]]

New try job here:

https://tbpl.mozilla.org/?tree=Try&rev=60259522219c

Comment 17

[Seth Fowler [:seth] [:s2h]]

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.)

Comment 18

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, Timothy!

The patch will need a rebase before landing.

Comment 19

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Call CurrentStatusTracker() with the decode lock held in OnImageDataComplete.

Trivial rebase against bug 943803.

Comment 20

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Make sure we hold the decode lock when calling DoError.

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.

Comment 21

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Assert that we own the decoding mutex in CurrentStatusTracker.

Trivial rebase against bug 943803.

Comment 22

[Seth Fowler [:seth] [:s2h]]

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

Comment 23

[Seth Fowler [:seth] [:s2h]]

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

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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