Last Comment Bug 771669 - Crash with memory-pressure, WebGL bufferData
: Crash with memory-pressure, WebGL bufferData
Status: RESOLVED FIXED
: crash, testcase
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla17
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 326633 379903
  Show dependency treegraph
 
Reported: 2012-07-06 14:54 PDT by Jesse Ruderman
Modified: 2012-08-10 07:16 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
fixed


Attachments
testcase (requires extension & pref) (310 bytes, text/html)
2012-07-06 14:54 PDT, Jesse Ruderman
no flags Details
stack trace (10.29 KB, text/plain)
2012-07-06 14:55 PDT, Jesse Ruderman
no flags Details
check for lost context in this overload of BufferData (804 bytes, patch)
2012-07-06 22:20 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-07-06 14:54:11 PDT
Created attachment 639813 [details]
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 2012-07-06 14:55:34 PDT
Created attachment 639817 [details]
stack trace

Nightly: bp-10007a87-6a62-42a7-8fac-0698c2120706
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2012-07-06 19:46:32 PDT
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 2012-07-06 21:53:45 PDT
"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) 2012-07-06 22:10:02 PDT
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) 2012-07-06 22:14:33 PDT
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) 2012-07-06 22:20:05 PDT
Created attachment 639934 [details] [diff] [review]
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) 2012-07-06 23:56:47 PDT
On Windows: bp-0c1bd681-bc4d-4f64-8054-972b92120707.
Comment 8 Jesse Ruderman 2012-07-07 15:46:56 PDT
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) 2012-07-07 15:53:32 PDT
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 2012-07-07 15:59:04 PDT
I see.  Thanks for explaining.
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2012-07-24 16:12:10 PDT
Oops, forgot to land.
https://tbpl.mozilla.org/?tree=Try&rev=237a172fc1c4
Comment 12 Benoit Jacob [:bjacob] (mostly away) 2012-07-24 16:39:55 PDT
syntax error.
https://tbpl.mozilla.org/?tree=Try&rev=a2a80830d77c
Comment 13 Benoit Jacob [:bjacob] (mostly away) 2012-07-25 11:35:10 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/573c09668364
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2012-07-25 11:52:39 PDT
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] 2012-07-26 05:12:22 PDT
https://hg.mozilla.org/mozilla-central/rev/573c09668364
Comment 16 Alex Keybl [:akeybl] 2012-07-26 14:44:04 PDT
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) 2012-08-02 14:42:43 PDT
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 B [:simonab ] 2012-08-10 07:16:23 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.