Closed Bug 1247679 Opened 4 years ago Closed 4 years ago

Improve tracing of JS members

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(3 files, 1 obsolete file)

In bug 1247515, there was some code that was using NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK for a JSObject member, causing a crash. We should improve this situation. One approach would be to make that macro have a stronger type. Another approach would be to move the null check for JSObject into TraceCallbacks, then convert uses of TRACE_JSVAL_MEMBER to TRACE_JS_MEMBER_CALLBACK. The latter would be easier to use.
The first approach looks something like this.
Duplicate of this bug: 1247686
With this patch, it looks like nsScriptErrorWithStack is the only place that is using the wrong macro, so that is good.
Nothing particularly tricky going on in these patches. It does outline null checks for JS object fields in Traverse methods. The main goal here is to simplify writing TRACE methods.

There's no hurry on reviewing these so feel to take your time.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba45451db660
Attachment #8721448 - Flags: review?(bugs)
Assignee: nobody → continuation
Attachment #8718422 - Attachment is obsolete: true
Comment on attachment 8721448 [details] [diff] [review]
part 1 - Make ClearJSHolder publicly inherit from TraceCallbacks.

(reviewing easy patches in my queue)
Attachment #8721448 - Flags: review?(bugs) → review+
Comment on attachment 8721449 [details] [diff] [review]
part 2 - Null check inside TraceCallbackFunc::Trace.

I might be interesting to know the null vs non-null ratio of these cases.
Attachment #8721449 - Flags: review?(bugs) → review+
Comment on attachment 8721450 [details] [diff] [review]
part 3 - Replace NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK with JS_MEMBER.

I wonder... Why we have _CALLBACK.
NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER would be simpler.
But making that change could be done in a separate bug - or in this one.
(but r+ for such change anyhow.)

r+ for this patch.
Attachment #8721450 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #8)
> I might be interesting to know the null vs non-null ratio of these cases.

I ran a brief session where I loaded techcrunch and cnn. In one process (the child I think), there were 4373 null checks out of 101336 calls to the trace methods I changed in the patch (4%). In the other process, there were 43328 null checks out of 267568 calls to the trace methods (16%).
Depends on: 1255223
You need to log in before you can comment on or make changes to this bug.