Closed
Bug 864785
Opened 12 years ago
Closed 11 years ago
exactly root JS code in toolkit/
Categories
(Toolkit :: General, defect)
Toolkit
General
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: froydnj, Assigned: dzbarsky)
References
Details
No description provided.
Comment 1•12 years ago
|
||
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•12 years ago
|
||
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.
Flags: needinfo?(sphink)
Comment 3•12 years ago
|
||
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.
Flags: needinfo?(sphink) → needinfo?(bhackett1024)
Comment 4•11 years ago
|
||
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•11 years ago
|
||
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?
Updated•11 years ago
|
Flags: needinfo?(bhackett1024)
Updated•11 years ago
|
Comment 6•11 years ago
|
||
I think this bug was fixed in bug 867459:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=867459&attachment=744401
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•