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

RESOLVED FIXED in mozilla29

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

Trunk
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments, 6 obsolete attachments)

1.24 KB, patch
Details | Diff | Splinter Review
2.59 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
957 bytes, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Depends on: 867755
(Assignee)

Comment 2

5 years ago
Created attachment 8335116 [details] [diff] [review]
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
Attachment #8335116 - Flags: review?(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.
Attachment #8335116 - Flags: review?(tnikkel)
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
Created attachment 8335739 [details] [diff] [review]
(Part 1) - Call CurrentStatusTracker() with the decode lock held in OnImageDataComplete.

It's a simple fix. This is exactly why we needed this assertion.
Attachment #8335739 - Flags: review?(tnikkel)
(Assignee)

Comment 7

5 years ago
Created attachment 8335740 [details] [diff] [review]
(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
Attachment #8335740 - Flags: review?(tnikkel)
(Assignee)

Updated

5 years ago
Attachment #8335116 - Attachment is obsolete: true
Attachment #8335739 - Flags: review?(tnikkel) → review+
Attachment #8335740 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
Created attachment 8336445 [details] [diff] [review]
(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.
Attachment #8336445 - Flags: review?(tnikkel)
(Assignee)

Comment 10

5 years ago
Created attachment 8336448 [details] [diff] [review]
(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
(Assignee)

Updated

5 years ago
Attachment #8335740 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
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.
comment 12
Flags: needinfo?(seth)
(Assignee)

Comment 14

5 years ago
(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!
Flags: needinfo?(seth)
(Assignee)

Comment 15

5 years ago
Created attachment 8341427 [details] [diff] [review]
(Part 2) - Make sure we hold the decode lock when calling DoError.

Updated patch.
Attachment #8341427 - Flags: review?(tnikkel)
(Assignee)

Updated

5 years ago
Attachment #8336445 - Attachment is obsolete: true
Attachment #8336445 - Flags: review?(tnikkel)
(Assignee)

Comment 17

5 years ago
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.)
Attachment #8341427 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 18

5 years ago
Thanks for the review, Timothy!

The patch will need a rebase before landing.
(Assignee)

Comment 19

5 years ago
Created attachment 8347011 [details] [diff] [review]
(Part 1) - Call CurrentStatusTracker() with the decode lock held in OnImageDataComplete.

Trivial rebase against bug 943803.
(Assignee)

Updated

5 years ago
Attachment #8335739 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 8347012 [details] [diff] [review]
(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.
Attachment #8347012 - Flags: review?(tnikkel)
(Assignee)

Updated

5 years ago
Attachment #8341427 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
Created attachment 8347013 [details] [diff] [review]
(Part 3) - Assert that we own the decoding mutex in CurrentStatusTracker.

Trivial rebase against bug 943803.
(Assignee)

Updated

5 years ago
Attachment #8336448 - Attachment is obsolete: true
Attachment #8347012 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 22

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/73c56ad7bc59
https://hg.mozilla.org/mozilla-central/rev/8be832270608
https://hg.mozilla.org/mozilla-central/rev/4ed374e9a290
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.