1096632 - crash in mozilla::WebGLContext::TexImageFromVideoElement(StrongGLenum<TexImageTargetDetails>, int, unsigned int, unsigned int, unsigned int, mozilla::dom::Element&)

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Andreas Gal :gal]

This bug was filed from the Socorro interface and is 
report bp-c84f65b8-4332-47e5-9530-9ab672141110.
=============================================================

I get an instant crash trying to view the polar sea demo in Nightly/Mac.

Comment 1

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

E10S enabled, I assume?

Comment 2

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

I don't see it on 10.9 - Dan, you have 10.10 - can you see this crash with or without E10S enabled?

Comment 3

[Chiajung Hung [:chiajung]]

Attachment: v1

Fix the crash for a null image.

BTW, there must be some related video ready state bugs out side here, but this sanity check should be good.

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Chiajung Hung [:chiajung] from comment #3)
> Created attachment 8521101 [details] [diff] [review]
> v1
> 
> Fix the crash for a null image.
> 
> BTW, there must be some related video ready state bugs out side here, but
> this sanity check should be good.

Are you sure this is the right patch? This is huge for just checking for a null image.

Comment 5

[Chiajung Hung [:chiajung]]

Oh...sorry, I attached wrong file, since the mac device is not mine...I will upload correct one soon.

Comment 6

[Chiajung Hung [:chiajung]]

Attachment: v1_correct

Sorry for wrong attachment

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8521103 [details] [diff] [review]
v1_correct

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

::: dom/canvas/WebGLContext.cpp
@@ +1826,5 @@
>  
>      gl->MakeCurrent();
>      nsRefPtr<mozilla::layers::Image> srcImage = container->LockCurrentImage();
> +    if (!srcImage)
> +      return false;

4-space indents!

Comment 8

[Chiajung Hung [:chiajung]]

Attachment: v1_final

Fix idention, carry r+.

Comment 9

[Carsten Book [:Tomcat]]

Hi, could you provide a try run thanks!

Comment 10

[Chiajung Hung [:chiajung]]

Try ticket:
https://tbpl.mozilla.org/?tree=Try&rev=cbacef1a0f06

Comment 11

[u480271]

(In reply to Milan Sreckovic [:milan] from comment #2)
> I don't see it on 10.9 - Dan, you have 10.10 - can you see this crash with
> or without E10S enabled?

On 10.10 Yosemite, with FF 36.0a1 (2014-11-13) and e10s *off*, I get a reproducible crash. With e10s *on*, there is no crash and I see video.

Comment 13

[Tim Nguyen :ntim]

Just needinfo'ing in case you forgot this.

Comment 14

[Chiajung Hung [:chiajung]]

Oh.. I forgot to set checkin-needed flag.

Thanks!

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cbfba445b09e

Comment 17

[Tim Nguyen :ntim]

So looks like Firefox 36 (beta) is currently affected by the bug. Do we want to uplift this ?

Comment 18

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

Comment on attachment 8521118 [details] [diff] [review]
v1_final

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Crash
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low, we are avoiding using null pointer.
[String/UUID change made/needed]:

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8521118 [details] [diff] [review]
v1_final

The patch landed in 37 already.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6696e78c24ed

Comment 21

[Cornel Ionce [:noni] [Hubs QA]]

In the last 2 weeks there were:
- 10 crashes on 36.0b5 (none in beta 6/7)
- none for Firefox 37.

Also, no longer reproducing with the polar sea demo.
Closing.