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

RESOLVED FIXED in mozilla11

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

Trunk
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
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...
Assignee: nobody → continuation
Attachment #580596 - Flags: review?(luke)
Attachment #580596 - Flags: feedback?(peterv)
(Assignee)

Comment 4

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

Updated

6 years ago
Attachment #580596 - Flags: review?(luke) → review+
(Assignee)

Updated

6 years ago
Blocks: 637931
(Assignee)

Updated

6 years ago
Blocks: 709162
(Assignee)

Updated

6 years ago
Duplicate of this bug: 710169
(Assignee)

Comment 6

6 years ago
https://hg.mozilla.org/mozilla-central/rev/a9394f00a379
Target Milestone: --- → mozilla11
(Assignee)

Comment 7

6 years ago
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...
Attachment #580596 - Flags: feedback?(peterv)

Updated

6 years ago
Blocks: 710170
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.