966630 - WebGL crash [@mozilla::gl::GLContext::fCompressedTexImage2D / gleEvaluateTextureImageChange]

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/1e7b2bb3041b

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Doesn't seem to be the driver (crashes on Windows and Mac).

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Dan, does this fall under the "refactoring so that we don't have to do them one by one"?

Comment 4

[u480271]

Milan, yes. This is the one that I started on.

Comment 5

[u480271]

I've fixed this in the work I've done for 966624, so adding a dependency.

Instead of crashing, now detects invalid level. From console:
  "Error: WebGL: compressedTexImage2D: level > maximum texture level"

Comment 6

[u480271]

Fixed by Bug 966624.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Did this only affect trunk (Firefox 30)?

Comment 8

[u480271]

I tested again the following and all are affected:
 * aurora
 * ff-27
 * ff-24esr

Comment 9

[Al Billings [:abillings - ex-MoCo]]

We should get this fixed on Aurora, Beta, and ESR24.

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Can we get a nom for uplift in Bug 966624 in order to for the fix to make into into ESR24?

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

Dan - Can you handle the nom and uplift for ESR24?

Comment 12

[u480271]

I'm not comfortable uplifting the patches from Bug 966624 to ESR24. I'll look at creating a fix for just this specific issue.

Comment 13

[u480271]

Attachment: Patch for FF ESR 24

Patch to fix the issue in ESR 24 instead of uplifting the whole refactoring of texture image parameters.

Comment 14

[u480271]

Comment on attachment 8405996 [details] [diff] [review]
Patch for FF ESR 24

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Ability to crash browser with bad WebGL parameters
Fix Landed on Version: FF30
Risk to taking this patch (and alternatives if risky): This patch is very small and easy to code review. The risk is negligible.
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Approved for ESR assuming it passed review.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

What about beta29?

Comment 17

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

Comment on attachment 8405996 [details] [diff] [review]
Patch for FF ESR 24

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

::: content/canvas/src/WebGLContextValidate.cpp
@@ +512,5 @@
> +     * expecting.
> +     */
> +
> +    if (level > 31)
> +        level = 31;

Good, and here is another justification for this good change: GLsizei is int32_t, so it can't exceed 2^31 - 1, so, even if we allowed NPOT cubemaps, there can't be a texture level > 30  (or else, texture mipmap completeness rules are violated, and we fake the sampling as a black texture).

Comment 18

[u480271]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> What about beta29?

This patch can be applied to Beta 29.

Comment 19

[u480271]

Comment on attachment 8405996 [details] [diff] [review]
Patch for FF ESR 24

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Ability to crash browser with bad WebGL parameters
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): This patch is very small and easy to code review. The risk is negligible.
String or IDL/UUID changes made by this patch:

Comment 20

[u480271]

Attachment: Aurora patch

Ported patch for ESR24 to Aurora.

Comment 21

[u480271]

Comment on attachment 8406629 [details] [diff] [review]
Aurora patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Ability to crash firefox with bad WebGL parameters.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Very low risk. Patch is easy to code review.
String or IDL/UUID changes made by this patch:

Comment 22

[Ryan VanderMeulen [:RyanVM]]

I'm confused, I thought this was fixed on Aurora by bug 966624? Does that mean that trunk (31) needs this fix as well?

Comment 23

[u480271]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22)
> I'm confused, I thought this was fixed on Aurora by bug 966624? Does that
> mean that trunk (31) needs this fix as well?

Between comment 5 and my testing yesterday the bug regressed in m-c trunk.

Comment 24

[u480271]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b300ff71f9b0

Comment 25

[u480271]

Bear with me because I'm new to this, but who does the check-in of patch to ESR24, Aurora and Beta? Am I supposed to? And why in my name against the a? for Aurora?

Comment 26

[Al Billings [:abillings - ex-MoCo]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #25)
> Bear with me because I'm new to this, but who does the check-in of patch to
> ESR24, Aurora and Beta? Am I supposed to? And why in my name against the a?
> for Aurora?

Your name is against the a? because you nominated it for Aurora approval?

Either you check it in or you get a sheriff like Ryan to do so.

Comment 27

[u480271]

esr24: https://hg.mozilla.org/releases/mozilla-esr24/rev/06567ddea1d6

Comment 28

[Carsten Book [:Tomcat]]

landed on mozilla-central https://hg.mozilla.org/mozilla-central/rev/b300ff71f9b0

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5d9bf448a9a9
https://hg.mozilla.org/releases/mozilla-beta/rev/17432cc5074d
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/6d9cfa6f24dd
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/6a8a4bc602c6

Comment 30

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on ASan Fx29, 2013-12-19.
Verified fix on ASan Fx29, Fx30, Fx31, 2014-04-17.

Comment 31

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fix on ASan Fx24esr, 2014-04-19.