Fix reported rooting hazard in WebGLContextGL.cpp

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

unspecified
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

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.
Attachment #8340324 - Flags: review?(bjacob)
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!
Flags: needinfo?(jcoppeard)
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.
Flags: needinfo?(jcoppeard)
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.
Attachment #8340324 - Flags: review?(bjacob) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/90fb9ca640e4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.