Closed Bug 864785 Opened 12 years ago Closed 11 years ago

exactly root JS code in toolkit/

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: froydnj, Assigned: dzbarsky)

References

Details

No description provided.
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.
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)
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)
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.
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?
Flags: needinfo?(bhackett1024)
Blocks: 898606
No longer blocks: ExactRootingBrowser
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee: nobody → dzbarsky
Depends on: 867459
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.