Closed Bug 829427 Opened 11 years ago Closed 11 years ago

More fun with the orphan reporter

Categories

(Core :: DOM: HTML Parser, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: [MemShrink])

Attachments

(1 file)

(Here's a lightly edited version of an email bz just sent me.)

We have three different kinds of JS wrappers for DOM nodes right now: WebIDL, slimwrappers, and XPCWrappedNatives.

The orphan reporter uses XPCConvert::GetISupportsFromJSObject, will produce an nsISupports from the JS object if it has one.  But for WebIDL and slimwrappers that will be the actual C++ object, while for XPCWrappedNatives it will be an nsXPCWrappedNative, which has an mIdentity pointer to the real object.

One way to tell them apart is to QI the nsISupports to nsIXPConnectWrappedNative and if that succeeds, call ->Native() on it to get the real C++ object...

Now the good news is that non-WebIDL DOM nodes _usually_ have slimwrappers.  But there are some things (XBL, and a few others) which can trigger failover to XPCWrappedNatives.  And right now we're not handling them, I think (nor did we use to before your changes to the orphan reporter, so it's not like you broke this).
Assignee: nobody → n.nethercote
I think this does the trick.  I had to loosen some assertions in the JS engine
that were triggered by the QI to nsINode.

I had some debugging printfs in an earlier version and I was measuring a few
KiBs of orphaned XPCWrappedNatives with gmail and TechCrunch loaded.
Attachment #700859 - Flags: review?(bzbarsky)
Comment on attachment 700859 [details] [diff] [review]
Catch XPCWrappedNatives in the orphan memory reporter.

r=me
Attachment #700859 - Flags: review?(bzbarsky) → review+
Hmm.  Though... Those assertions worry me.  That QI call can call into script and run random JS.  Is that a safe thing to do at this point?
Good question.  Running arbitrary JS while iterating over the JS heap... I don't know.
So one option would be to have the JSObject there and check it's class so we'll know we really have an XPCWN...  That gets annoying.

Another option is to not worry about it and just wait for us to finish converting all nodes to WebIDL; with any luck that's a few more months.
> Another option is to not worry about it and just wait for us to finish
> converting all nodes to WebIDL; with any luck that's a few more months.

Let's do that.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
I guess we don't hit those assertions right now because the QIs for slim-wrappered objects (and XPCWNs) are not going to involve JS?
The QI for slimwrappered objects should not involve JS.

The QI for XPCWN I think could (thanks to XBL), but only for nodes, and those QI to nsINode without involving JS.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: