709160 - ObjShrink broke nsXPConnect::Traverse's cycle collector dumping code

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

See for instance this crash report:
https://crash-stats.mozilla.com/report/index/c521651d-6b07-442f-86a3-12ecc2111208

The problematic code is here:
JSFunction* fun = (JSFunction*) xpc_GetJSPrivate(obj);
JSString* str = JS_GetFunctionId(fun);

I could fix that, but I think it is better to just tear out this part of the code and use a JS engine toString function.

Also, I need to add a crash test for cycle collector dumps.

Comment 1

[Andrew McCreight [:mccr8]]

Doing this right will require fixing bug 682353, so I think I'll just do a quick fix to get cycle collector dumps working again, and I'll file a new bug for factoring things out.

Also, looking at MXR this is the only instance in the code where the result of xpc_GetJSPrivate is cast to a JSFunction, so there shouldn't be any other instances of this problem.

Comment 2

[Andrew McCreight [:mccr8]]

Problem 2:
  JSObject *global = JS_GetGlobalForObject(NULL, static_cast<JSObject*>(p));

This line seems to crash in what looks like in an assertion due to trying to deref the NULL.  When I change NULL to cx, the cycle collector context, then about 1.4mb into the log file it crashes with an assertion failure isGlobal() apparently because the global it finds isn't actually a global.

There's also weird stuff like this in the cycle collector log:

0x11a11dd30 [gc.marked] JS Object (nsXPCComponents_Classes) (global=1185dc420)
> 0x11a11dd00 type_proto
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent
> 0x1185dc420 parent

(etc.)

I don't know if that is a problem with JS_TraceChildren or what.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: patch around ObjShrink fallout

This fixes a few problems that make cycle collector dumping crash, due to fallout from ObjShrink.

1. A JSObject's function should be extracted via JS_GetObjectFunction, as it isn't in the private slot any more.
2. GetGlobalForObject is failing various asserts now, and I haven't been able to debug it, so I've comment out the part that prints the global.  That isn't ideal, but it is better than the CC dump crashing.

I'm not really sure what the best way to review this is.  The first change is pretty non-controversial, but I'd like to get peterv's (or some XPConnect peer's) super review, if you will, on disabling this part of the CC dumping...

Comment 4

[Andrew McCreight [:mccr8]]

I checked that cycle collector dumping doesn't crash with this patch applied, at least for some basic browsing.

Comment 6

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/mozilla-central/rev/a9394f00a379

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 580596 [details] [diff] [review]
patch around ObjShrink fallout

I landed this to get it in a state where it isn't crashing.  I'm not sure how important having the global of each object is.  And it isn't clear that it can be safely computed in a post-ObjShrink world...