Closed Bug 958758 Opened 6 years ago Closed 6 years ago

WebGL ABORT: Calling InitDecoder() but already decoded!: '!mDecoded'

Categories

(Core :: ImageLib, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla29
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed

People

(Reporter: posidron, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase, Whiteboard: [fuzzblocker])

Attachments

(4 files)

Attached file callstack
The stack points to ImageLib.
Component: Canvas: WebGL → ImageLib
It's probably the FinishedSomeDecoding call in SyncDecode just before the problematic InitDecoder call that sets mDecoded to true and nulls out mDecoder. Which is fine, because we need to wrap up that decode and throw it away and start a new one with the new decode flags. So I guesss we should check if we are mDecoded after the FinishedSomeDecoding call, and ForceDiscard if we are so we can get a new decoder in place lower down.
Attached patch patchSplinter Review
This does comment 3.

I didn't test that it fixed the testcase, but I think it should.

try builds are here http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tnikkel@gmail.com-600b56b50bf0/ and here http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tnikkel@gmail.com-5d8fde1d9564/
Assignee: nobody → tnikkel
Attachment #8358773 - Flags: review?(seth)
Comment on attachment 8358773 [details] [diff] [review]
patch

Review of attachment 8358773 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. (Though we definitely have design issues here, but this is the right short-term fix.)

::: image/src/RasterImage.cpp
@@ +2377,5 @@
> +    if (mDecoded) {
> +      // If we've finished decoding we need to discard so we can re-decode
> +      // with the new flags. If we can't discard then there isn't
> +      // anything we can do.
> +      if (!CanForciblyDiscard() || mDecoder || mAnim)

This will introduce the third place we use this exact if-condition in RasterImage.cpp. Can we think up a name for this and convert it into a function? Perhaps CanForciblyDiscardAndRedecode()?

Having all of CanDiscard / CanForciblyDiscard / CanForciblyDiscardAndRedecode suggests to me that we don't have these functions factored correctly, but that's probably best handled in a separate bug.
Attachment #8358773 - Flags: review?(seth) → review+
Christoph, it would be great if your fuzzing in this area (images used as webgl textures) could include all four combinations of images with/without transparency and animating/non-animating images. That would test all codepaths in that area. You might already be doing this for all I know. :)
Flags: needinfo?(cdiehl)
Thanks for the hint. I am not sure about transparent images but animating and non-animating images are already used during fuzzing.
Flags: needinfo?(cdiehl)
I made it as a patch on top, I think that'll be clearer.

The case in RequestDiscard I was very conservative. It originally called CanDiscard and then ForceDiscard. Not sure if that was an oversight or what, so I just preserved the CanDiscard and added CanForciblyDiscardAndRedecode.
Attachment #8362037 - Flags: review?(seth)
Christoph, sorry to needinfo you again but I just wanted to add to Timothy's suggestion: we should also fuzz both premultiplied alpha and non-premultiplied-alpha textures for webgl, since the two can trigger significantly different behavior, especially in a mixed-HTML-and-WebGL situation where the same image is used in both contexts on the same page.
Flags: needinfo?(cdiehl)
Comment on attachment 8362037 [details] [diff] [review]
Factor out discard checks.

Review of attachment 8362037 [details] [diff] [review]:
-----------------------------------------------------------------

Love it!
Attachment #8362037 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/cb09583bf5ca
https://hg.mozilla.org/mozilla-central/rev/27b19b03da11
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(In reply to Seth Fowler [:seth] from comment #9)
> Christoph, sorry to needinfo you again but I just wanted to add to Timothy's
> suggestion: we should also fuzz both premultiplied alpha and
> non-premultiplied-alpha textures for webgl, since the two can trigger
> significantly different behavior, especially in a mixed-HTML-and-WebGL
> situation where the same image is used in both contexts on the same page.

Noted. :-)
Flags: needinfo?(cdiehl)
Reproduced in Nightly 2014-01-09-mozilla-central-debug.
Verified fixed Nightly 2014-01-26-mozilla-central-debug, win 7 x64.
Status: RESOLVED → VERIFIED
Blocks: 962670
blocking-b2g: --- → 1.3T?
blocks Bug 962670
1.3T+
blocking-b2g: 1.3T? → 1.3T+
You need to log in before you can comment on or make changes to this bug.