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

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/23e682c34d75

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

The stack points to ImageLib.

Comment 3

[Timothy Nikkel (:tnikkel)]

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.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: patch

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/[e-mail]-600b56b50bf0/ and here http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5d8fde1d9564/

Comment 5

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

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.

Comment 6

[Timothy Nikkel (:tnikkel)]

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

Comment 7

[Christoph Diehl [:posidron]]

Thanks for the hint. I am not sure about transparent images but animating and non-animating images are already used during fuzzing.

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: Factor out discard checks.

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.

Comment 9

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

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.

Comment 10

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

Comment on attachment 8362037 [details] [diff] [review]
Factor out discard checks.

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

Love it!

Comment 11

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb09583bf5ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/27b19b03da11

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb09583bf5ca
https://hg.mozilla.org/mozilla-central/rev/27b19b03da11

Comment 13

[Christoph Diehl [:posidron]]

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

Comment 14

[Paul Silaghi, QA [:pauly]]

Reproduced in Nightly 2014-01-09-mozilla-central-debug.
Verified fixed Nightly 2014-01-26-mozilla-central-debug, win 7 x64.

Comment 15

[Joe Cheng [:jcheng] (please needinfo)]

blocks Bug 962670
1.3T+

Comment 16

[Michael Wu [:mwu]]

http://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/f6f76e254724
http://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/7cd55887a507