Last Comment Bug 709160 - ObjShrink broke nsXPConnect::Traverse's cycle collector dumping code
: ObjShrink broke nsXPConnect::Traverse's cycle collector dumping code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
: 710169 (view as bug list)
Depends on:
Blocks: 710170 ObjectShrink 709162
  Show dependency treegraph
 
Reported: 2011-12-09 09:50 PST by Andrew McCreight [:mccr8]
Modified: 2011-12-15 08:27 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch around ObjShrink fallout (2.69 KB, patch)
2011-12-09 17:49 PST, Andrew McCreight [:mccr8]
luke: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2011-12-09 09:50:57 PST
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] 2011-12-09 14:17:44 PST
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] 2011-12-09 15:19:18 PST
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] 2011-12-09 17:49:14 PST
Created attachment 580596 [details] [diff] [review]
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] 2011-12-09 17:56:15 PST
I checked that cycle collector dumping doesn't crash with this patch applied, at least for some basic browsing.
Comment 5 Andrew McCreight [:mccr8] 2011-12-13 06:43:15 PST
*** Bug 710169 has been marked as a duplicate of this bug. ***
Comment 6 Andrew McCreight [:mccr8] 2011-12-14 16:07:00 PST
https://hg.mozilla.org/mozilla-central/rev/a9394f00a379
Comment 7 Andrew McCreight [:mccr8] 2011-12-14 16:10:03 PST
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...

Note You need to log in before you can comment on or make changes to this bug.