944675 - Fix reported rooting hazard in WebGLContextGL.cpp

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: fix-webgl-hazards

The rooting analysis reports the following hazards:

Function 'JS::Value mozilla::WebGLContext::GetUniform(JSContext*, mozilla::WebGLProgram*, mozilla::WebGLUniformLocation*)' has unrooted 'obj' of type 'JSObject*' live across GC call 'void mozilla::WebGLContext::ErrorOutOfMemory(int8*)' at content/canvas/src/WebGLContextGL.cpp:1904

Function 'JS::Value mozilla::WebGLContext::GetUniform(JSContext*, mozilla::WebGLProgram*, mozilla::WebGLUniformLocation*)' has unrooted 'obj:1' of type 'JSObject*' live across GC call 'void mozilla::WebGLContext::ErrorOutOfMemory(int8*)' at content/canvas/src/WebGLContextGL.cpp:1916

Function 'JS::Value mozilla::WebGLContext::GetUniform(JSContext*, mozilla::WebGLProgram*, mozilla::WebGLUniformLocation*)' has unrooted 'obj:2' of type 'JSObject*' live across GC call 'void mozilla::WebGLContext::ErrorOutOfMemory(int8*)' at content/canvas/src/WebGLContextGL.cpp:1931

The simplest fix for these is to return JS::NullValue() rather than touching the obj pointer after a potential GC.

Comment 1

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

I don't understand how this patch makes any difference?

             JSObject* obj = Float32Array::Create(cx, this, unitSize, fv);
             if (!obj) {
                 ErrorOutOfMemory("getUniform: out of memory");
+                return JS::NullValue();
             }
             return JS::ObjectOrNullValue(obj);

Here, we are in a if (!obj) branch, so the "return JS::NullValue();" is equivalent to the "return JS::ObjectOrNullValue(obj);" which we are doing at the next line.

Please explain!

Comment 2

[Jon Coppeard (:jonco)]

The rooting analysis sees that the expression JS::ObjectOrNullValue(obj) makes use of an unrooted pointer (obj) after a possible GC in ErrorOutOfMemory(), so it reports this as a hazard.

It's not actually a hazard because we can see that ErrorOutOfMemory() is only called if obj is null, but making the analysis understand that in the general case would be very difficult.

Comment 3

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

Hm, wait, are you saying that ErrorOutOfMemory can trigger a GC? All it does, that is related to JS, is generate a JS warning.

That would indeed make it reasonable to want to avoid passing a JSObject* around across it.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90fb9ca640e4

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90fb9ca640e4