771669 - Crash with memory-pressure, WebGL bufferData

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires extension & pref)

1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
2. Set 
     user_pref("dom.experimental_bindings", false);
3. Load the testcase.

Result: Crash [@ mozilla::WebGLContext::ErrorInvalidValue ]

Related to bug 764254 and/or bug 743753?

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Nightly: bp-10007a87-6a62-42a7-8fac-0698c2120706

Comment 2

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

This doesn't seem related to bug 764254. It is true that this wouldn't have happened before bug 743753 landed, but that is not the underlying problem. The problem seems that ErrorInvalidValue doesn't seem to behave gracefully under memory pressure. It should, as there is no reason to it to want to allocate much memory.

Comment 3

[Jesse Ruderman]

"memory-pressure" is a notification that triggers GC, CC, and flushes caches. In this case, it's unrelated to actually being low on memory.

Comment 4

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

Ah, that memory-pressure. OK, so memory-pressure causes the WebGL context to get 'lost' which means that all GL resources are freed. So I take back what I said in comment 2, this could well be the same issue as bug 764254, a general issue about WebGL contexts in lost state not behaving correctly.

Comment 5

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

OK, and the stack trace you attached shows what's happening, the GL context pointer is reset to NULL and we're dereferencing that when we attempt to update the GL error state.

Bug 764254 is a similar but distinct bug, where we are also dereferencing this null gl context pointer.

Comment 6

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

Attachment: check for lost context in this overload of BufferData

Indeed, this overload of BufferData wasn't properly checking for context loss like other overloads and other webgl functions do.

Comment 7

[Scoobidiver (away)]

On Windows: bp-0c1bd681-bc4d-4f64-8054-972b92120707.

Comment 8

[Jesse Ruderman]

Hmm, does sending a memory-pressure notification break all existing WebGL contexts?  That seems excessive compared to what memory-pressure does in the rest of Gecko.

(Indeed, pressing the "Minimize memory usage" button in about:memory causes the visualization on http://cubicvr.org/CubicVR.js/bd3/BeatDetektor3HD.html to go away.)

Comment 9

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

Yes and no.

memory-pressure causes all WebGLContext to be "lost" immediately. However, WebGL applications then receive a webglcontextlost event that they are supposed to handle to request getting restored, in which case they receive a webglcontextrestored event which they are supposed to handle to restore their WebGL state.

Here is an example of a WebGL page that handles this correctly:
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/demos/google/san-angeles/index.html

In practice, of course many WebGL pages do not handle this at all. This is a problem; having the "Minimize memory usage" button in about:memory causing WebGL context loss was considered an acceptable way of raising developer attention about this issue and providing an easy way to test any application. This can also be tested programatically using the WEBGL_lose_context extension.

Notice that context loss preexists WebGL. The OpenGL ES API that WebGL is based on, and that WebGL implementations build on, already have context loss. GPU resources are inherently non-permanent in many architectures (e.g. wrt device sleep).

Comment 10

[Jesse Ruderman]

I see.  Thanks for explaining.

Comment 11

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

Oops, forgot to land.
https://tbpl.mozilla.org/?tree=Try&rev=237a172fc1c4

Comment 12

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

syntax error.
https://tbpl.mozilla.org/?tree=Try&rev=a2a80830d77c

Comment 13

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/573c09668364

Comment 14

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

Comment on attachment 639934 [details] [diff] [review]
check for lost context in this overload of BufferData

[Approval Request Comment]
Bug caused by (feature/regressing bug #): since the switch of WebGLContext to new bindings, firefox 15
User impact if declined: occasional, non-security-sensitive webgl crash
Testing completed (on m-c, etc.): m-i
Risk to taking this patch (and alternatives if risky): no risk
String or UUID changes made by this patch: none

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/573c09668364

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 639934 [details] [diff] [review]
check for lost context in this overload of BufferData

[Triage Comment]
It's hard to argue with a no-risk crash fix for Aurora/Beta this early in the cycle. Approved.

Comment 17

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

http://hg.mozilla.org/releases/mozilla-aurora/rev/da26c9c61b2e
http://hg.mozilla.org/releases/mozilla-beta/rev/a6fba0be4e2d

Sorry for the delayed landing.

Comment 18

[Simona Badau, Desktop QA]

Verified using the steps to reproduce from the Description that Firefox 15 beta 4 does not crash when loading the attached test case. Verified on Windows XP, Ubuntu 12.04 and Mac OS X 10.6:
Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:15.0) Gecko/20100101 Firefox/15.0

Verified also the Socorro reports, and I didn't fond any crashes after this fix was pushed.