1247679 - Improve tracing of JS members

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Strongly type NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK

The first approach looks something like this.

Comment 3

[Andrew McCreight [:mccr8]]

With this patch, it looks like nsScriptErrorWithStack is the only place that is using the wrong macro, so that is good.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Make ClearJSHolder publicly inherit from TraceCallbacks.

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

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Null check inside TraceCallbackFunc::Trace.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Replace NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK with JS_MEMBER.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8721448 [details] [diff] [review]
part 1 - Make ClearJSHolder publicly inherit from TraceCallbacks.

(reviewing easy patches in my queue)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[Andrew McCreight [:mccr8]]

(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%).

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f5dd362cb2
https://hg.mozilla.org/integration/mozilla-inbound/rev/cead26ef21c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/71216a38ad10

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d1f5dd362cb2
https://hg.mozilla.org/mozilla-central/rev/cead26ef21c8
https://hg.mozilla.org/mozilla-central/rev/71216a38ad10