864785 - exactly root JS code in toolkit/

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Comment 1

[(no longer active)]

This is the only remaining hazard in toolkit:

Function 'Telemetry.cpp:uint32 {anonymous}::ReflectHistogramAndSamples(JSContext*, class JS::Handle<JSObject*>, base::Histogram*, base::Histogram::SampleSet*)' has unrooted '__temp_12' of type 'JSObject*' live across GC call base::Histogram.histogram_type at toolkit/components/telemetry/Telemetry.cpp:549
    toolkit/components/telemetry/Telemetry.cpp:549: Call(15,16, __temp_14 := h*.histogram_type*())
    toolkit/components/telemetry/Telemetry.cpp:549: Call(16,17, __temp_13 := INT_TO_JSVAL(__temp_14*))
    toolkit/components/telemetry/Telemetry.cpp:549: Call(17,18, __temp_11 := JS_DefineProperty(cx*,__temp_12*,"histogram_type",__temp_13*,0,0,1))


I have no idea how to fix it.

Comment 2

[Till Schneidereit [:till]]

Neither do I. Also, I can't even see how there would be a hazard there. AFAICT, `__temp_12` is `obj`, really, which is a handle and thus guaranteed to be rooted.

Pinging Dr. sfink for help.

Comment 3

[Steve Fink [:sfink] [:s:]]

Uh... man, it really looks like it's caching the pointer it gets from obj somewhere (stack or register) and holding it across multiple calls that could GC, which... er, shouldn't be valid, since the compiler should know perfectly well that it can't do that, because it could get updated. Ignoring the possibility of compiler error, the only thing I can think of would be lingering aliasing problem. Paging bhackett, since I'm not confident of my ability to read the compilation trace output.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Pretty sure this got fixed when we made Handle return a "const T&" instead of a "T" from its conversion operator.  Before that there was a hazard where the second arg triggered a conversion, then fourth arg triggered a virtual call and boom.

Comment 5

[Steve Fink [:sfink] [:s:]]

Oh, right! I think it makes sense to me now. Restating my understanding: we have a call

 JS_DefineProperty(cx, obj, "histogram_type", INT_TO_JSVAL(h->histogram_type()), NULL, NULL, JSPROP_ENUMERATE)

where obj is a Handle<JSObject*>. The hazard is that you could pull the JSObject* out of obj, then call h->histogram_type(), which could GC. The compiler knows that it can't cache (JSObject*)obj across anything that could change it, but there's no sequence point in between the conversion and the histogram_type() call. So the compiler is allowed to not care.

Removing the conversion fixes the problem.

Does that sound right?

Comment 6

[Chris Peterson [:cpeterson]]

I think this bug was fixed in bug 867459:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=867459&attachment=744401