Closed Bug 924117 Opened 9 years ago Closed 9 years ago

crash in mozilla::image::RasterImage::RequestDecodeCore(mozilla::image::RasterImage::RequestDecodeType)


(Core :: ImageLib, defect)

27 Branch
Not set





(Reporter: tracy, Assigned: sworkman)


(Keywords: crash)

Crash Data


(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-5b4760b4-3e29-4576-93ef-9f0382131006.

This cash began happening on 20130929 and is climbing up near topcrash status (currently #6) on Nightly.

URL's don't point to anything in particular.
Did something in the relevant code change recently?
There seem to be some perhaps related crashes earlier, but the "new" ones seem to come through WantDecodedFrames(), where the earlier ones are from other ways to get to RequestDecodeCore().
All sorts of Windows, Linux, but only Mac 10.6 - no higher version Macs.  Perhaps a hint, perhaps just statistics.  Perhaps related to bug 867755?
Flags: needinfo?(sworkman)
Flags: needinfo?(seth)
Agreed, my bet is on 867755. Steve, I'll be interested to hear your take on this.
Flags: needinfo?(seth)
(Sorry it took a while to get to this - I have a beta crasher as well :) ).

Something I noticed, but I think is unrelated: imgRequestProxy::GetImage should use do_QueryObject instead of do_QueryInterface for image to imageToReturn.

Something more relevant: Assuming this is some kind of race, I'm thinking it's to do with mSourceData. There are stacktrace variants, but they all end on the same line. If it were to do with RasterImage (being deleted), I would have thought that access to other member variables would have caused exceptions too. But it's always the same location.

I noticed that many other functions wrap access to mSourceData in the decoding mutex, but in at least one of the crashes, no call is made in the stacktrace to lock it. So, I'm wondering if something like mSourceData.Compact() is being called while RequestDecodeCore is executing.

All very speculative, but adding a mutex lock seems like it could be a smart idea here anyway.

What do you think?
Flags: needinfo?(sworkman)
Seth, this is the sort of speculative patch I mean.

Any issues you foresee?

Assignee: nobody → sworkman
Attachment #817451 - Flags: feedback?(seth)
Comment on attachment 817451 [details] [diff] [review]
v1.0 Wrap access to RasterImage::mSourceData in mDecodingMutex

Review of attachment 817451 [details] [diff] [review]:

::: image/src/RasterImage.cpp
@@ +2232,5 @@
> +    // mBytesDecoded can be bigger than mSourceData.Length() if we're not storing
> +    // the source data.
> +    if (mBytesDecoded > mSourceData.Length())
> +      return NS_OK;
> +  }

Hmm.. I really don't like the lock bouncing here, especially for such a trivial reason.

I'd prefer we move this entire check down below the existing MutexAutoLock line in RequestDecodeCode. This change in the order of the checks has the potential to change behavior slightly, since we may switch to a full decode even if we don't have any bytes to flush to the decoder, but (famous last words) I don't think it's a problem.

Obviously needs a try run, though.
Attachment #817451 - Flags: feedback?(seth)
Moved check into existing locked code.

Cancelled previous try. New one:
Attachment #817451 - Attachment is obsolete: true
Attachment #817488 - Flags: review?(seth)
Comment on attachment 817488 [details] [diff] [review]
v1.1 Wrap access to RasterImage::mSourceData in mDecodingMutex

Review of attachment 817488 [details] [diff] [review]:

Looks good!
Attachment #817488 - Flags: review?(seth) → review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.