1151930 - Firefox Nightly and Aurora crash with deferred rendering WebGL demos (OOM)

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[AlteredQualia]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150407030207

Steps to reproduce:

Visit some of the XG deferred rendering demos with latest Firefox Nightly or Firefox Developer Edition, for example:

http://alteredqualia.com/xg/examples/deferred_skin.html
http://alteredqualia.com/xg/examples/animation_physics_terrain.html

Tested on:

Firefox Nightly 40.0a1 (2015-04-07)
Firefox Developer Edition (Aurora) 39.0a2 (2015-04-07)


Actual results:

Firefox crashed, closed itself and opened Mozilla Crash Reporter.


Expected results:

No crashes should happen.

These crashes seem to differ from some other WebGL crashes reported elsewhere:

https://bugzilla.mozilla.org/show_bug.cgi?id=1125466
https://bugzilla.mozilla.org/show_bug.cgi?id=1147447
https://bugzilla.mozilla.org/show_bug.cgi?id=1124427

Differences from other crash reports:

- both ANGLE and OpenGL WebGL rendering backends crash
- both notebooks with Optimus (Nvidia GTX 970M) and without Optimus (Nvidia Quadro 2000M) crash
- crashes happen on Windows 7 64-bit and Windows 8 64-bit
- crashes happen both with and without E10S flag enabled
- non-deferred XG demos do work, e.g.:

http://alteredqualia.com/xg/examples/forward_materials_skin_jonas.html

Comment 1

[thundercat]

Crashes in Nightly 40.0a1 for me also, ok in FF 37.0.1

Windows 10 64bit (build 10049) iMac NVIDIA GeForce GTX 660M

Comment 2

[Mayank Bansal]

got a crash with HWA disabled on win7 x64:

https://crash-stats.mozilla.com/report/index/abb83a42-4b18-4ffc-b6cd-fb93d2150407

Comment 3

[Kelsey Gilbert [:jgilbert]]

I confirm this.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Check-against-updated-length-while-appending.patch

I made a dumb in a previous patch. Oops.

Comment 5

[Loic]

With e10s enabled, it killed my computer, I needed to hard reboot.

Comment 6

[u480271]

Comment on attachment 8589289 [details] [diff] [review]
0001-Check-against-updated-length-while-appending.patch

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

::: dom/canvas/WebGLFramebuffer.cpp
@@ -829,5 @@
>      if (colorAttachmentId < ColorAttachmentCount())
>          return;
>  
> -    size_t colorAttachmentCount = ColorAttachmentCount();
> -    while (colorAttachmentCount < WebGLContext::kMaxColorAttachments) {

WAT? I hope I didn't write that.

@@ +834,1 @@
>          mMoreColorAttachments.AppendElement(AttachPoint(this, nextAttachPoint));

Why don't we just allocate this array at context creation time?

Comment 7

[Kelsey Gilbert [:jgilbert]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/7658ee66cba9

Comment 8

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #6)
> Comment on attachment 8589289 [details] [diff] [review]
> 0001-Check-against-updated-length-while-appending.patch
> 
> Review of attachment 8589289 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLFramebuffer.cpp
> @@ -829,5 @@
> >      if (colorAttachmentId < ColorAttachmentCount())
> >          return;
> >  
> > -    size_t colorAttachmentCount = ColorAttachmentCount();
> > -    while (colorAttachmentCount < WebGLContext::kMaxColorAttachments) {
> 
> WAT? I hope I didn't write that.
> 
> @@ +834,1 @@
> >          mMoreColorAttachments.AppendElement(AttachPoint(this, nextAttachPoint));
> 
> Why don't we just allocate this array at context creation time?
We can do it after GL initialization, but not before. It should probably be a UniquePtr<Foo[]> though.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7658ee66cba9

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 39+ since this is a recent regression.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Jeff, from the bug summary it looks like this also affects 39. Is this something that you may want to uplift to aurora?

Comment 12

[Kelsey Gilbert [:jgilbert]]

(In reply to Liz Henry (:lizzard) from comment #11)
> Jeff, from the bug summary it looks like this also affects 39. Is this
> something that you may want to uplift to aurora?

Yes.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8589289 [details] [diff] [review]
0001-Check-against-updated-length-while-appending.patch

Approval Request Comment
[Feature/regressing bug #]: 1017865
[User impact if declined]: OOM crashes
[Describe test coverage new/current, TreeHerder]: on nightly
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8589289 [details] [diff] [review]
0001-Check-against-updated-length-while-appending.patch

Approving for uplift to aurora as this may help with OOM crashes.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3cea876963c4

Comment 16

[suleman.ravla]

Hi,

Reproduced the bug in version Firefox Developer Edition (Aurora) 39.0a2 (2015-04-07).
Bug is resolved in version Firefox Developer Edition (Aurora) 39.0a2 (2015-05-03).

-Thanks
Suleman Rawla

Comment 17

[suleman.ravla]

Hi,

Bug is resolved in version FireFox Nightly 40.0a1 (2015-05-03).

-Thanks
Suleman Rawla

Comment 18

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Thank you for your work to verify this Suleman! Setting status flags accordingly.