Closed Bug 412491 Opened 12 years ago Closed 12 years ago

function objects cloned by XPConnect still keep hidden window alive late into shutdown

Categories

(Core :: XPConnect, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: peterv, Assigned: peterv)

References

Details

(Keywords: memory-leak)

Attachments

(1 file, 5 obsolete files)

See bug 398219, in particular bug 398219, comment 32.

The attached patch fixes the problem by clearing the proto and parent of the object of the JSFunction that XPCNativeMember::Resolve creates. I haven't seen anything obviously broken but I'm looking for feedback from jst/igor/brendan.
Flags: blocking1.9?
Attached patch v1 (obsolete) — Splinter Review
Attachment #297231 - Flags: superreview?(jst)
Attachment #297231 - Flags: review?(igor)
Priority: -- → P1
Requesting blocking/P1 because it looks like this will fix a bunch of our extension-related leaks. They're not bad leaks (shutdown leak of the hidden window mostly), but we need to clear them to be able to assess how good/bad we are wrt leaking from extensions.
Flags: blocking1.9? → blocking1.9+
Keywords: mlk
Comment on attachment 297231 [details] [diff] [review]
v1

>Index: js/src/xpconnect/src/xpcwrappednativeinfo.cpp
>===================================================================
>RCS file: /Users/peterv/source/cvs.mozilla.org/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednativeinfo.cpp,v
>retrieving revision 1.16
>diff -u -p -u -1 -2 -r1.16 xpcwrappednativeinfo.cpp
>--- js/src/xpconnect/src/xpcwrappednativeinfo.cpp	12 Jun 2006 22:39:54 -0000	1.16
>+++ js/src/xpconnect/src/xpcwrappednativeinfo.cpp	15 Jan 2008 18:53:17 -0000
>@@ -184,24 +184,27 @@ XPCNativeMember::Resolve(XPCCallContext&
> 
>     JSFunction *fun = JS_NewFunction(cx, callback, argc, flags, nsnull,
>                                      iface->GetMemberName(ccx, this));
>     if(!fun)
>         return JS_FALSE;
> 
>     JSObject* funobj = JS_GetFunctionObject(fun);
>     if(!funobj)
>         return JS_FALSE;
> 
>     AUTO_MARK_JSVAL(ccx, OBJECT_TO_JSVAL(funobj));
> 
>+    STOBJ_SET_PARENT(funobj, nsnull);
>+    STOBJ_SET_PROTO(funobj, nsnull);

Clearing the parent prevents accessing properties that exists in the prototype such as apply, call, toString etc. On the other hand clearing the parent should not lead to any bad consequences. So would just single STOBJ_SET_PARENT(funobj, nsnull) work?

Note that the patch for JS engine that cleared proto and parent in the functions stored in JSScript instances do not need to worry about the issue as those function objects are never exposed to scripts.
Just clearing the parent still leaked when I tried that, same thing for just clearing the proto.

I checked the callers of XPCNativeMember::GetValue (which is the only way to get at those function objects), and all callers call xpc_CloneJSFunction afterwards (when we're dealing with a method or a getter/setter), so I think that means we're not exposing funobj to script?
(In reply to comment #4)
> I checked the callers of XPCNativeMember::GetValue (which is the only way to
> get at those function objects), and all callers call xpc_CloneJSFunction
> afterwards (when we're dealing with a method or a getter/setter), so I think
> that means we're not exposing funobj to script?

Right, then we can clear the proto for the same reason we can clear it when compiling.
Attachment #297231 - Flags: review?(igor) → review+
Attached patch Alternate patch v1 (obsolete) — Splinter Review
This is a bigger patch that ensures that the function objects that we create in XPCNativeMember::Resolve (and for which we set parent and proto to null) are never handed out directly, but are always cloned before being returned. This would prevent any new usage of XPCNativeMember::GetValue without subsequent cloning (the existing callers in the tree are fine).

Jst: I personally prefer this one (even though it's bigger), so do let me know if you prefer this one too.
(In reply to comment #6)
> Created an attachment (id=297334) [details]
> Alternate patch v1
> 
> This is a bigger patch that ensures that the function objects that we create in
> XPCNativeMember::Resolve (and for which we set parent and proto to null) are
> never handed out directly, but are always cloned before being returned.

This is much better. The question is why bother with clonning functions at all? I.e. in all those places where the patch asks for clone why not to create JS function directly? Yes, it would create an extra JSFunction in addition to JSObject wrapping it compared with the clonning case, but it would avoid creating the original function and would generate less long-term lived JS objects.  
(In reply to comment #7)
> why bother with clonning functions at all?

In some way this resembles bug 406356 but for native methods. I.e. the idea is to have JS API to create just JSFunction without the corresponding wrapping objects in Resolve and then wrap JSFunction as necessary. On the other hand the efforts to share this single JSFunction may be too big compared with not creating it all and just recording in Resolve the info necessary to create JS_NewFunction later.

(In reply to comment #7)
> This is much better. The question is why bother with clonning functions at all?
> I.e. in all those places where the patch asks for clone why not to create JS
> function directly?

I don't know exactly why, jst or jband would probably know. But I guess it's to avoid creating a lot of JSFunction objects, afaict there's only ever one XPCNativeMember per interface member function for the whole runtime (and so only one corresponding JSFunction).
Comment on attachment 297334 [details] [diff] [review]
Alternate patch v1

So how about we take this one then? I could rename GetClonedFunction into GetFunction and then we can always do away with the cloning in a different bug if we think it's worth it?
Attachment #297334 - Flags: review?(igor)
(In reply to comment #9)
> But I guess it's to
> avoid creating a lot of JSFunction objects

It does not save match. On the current tip the size of JSObject is 8 words while JSFunction takes 6 words. So with cloning we are saving 6 words per cloned function but waste 8+6 words to store the original function. In addition this initial JSFunction+JSObject pair creates long-term GC garbage that during cycle collection requires extra hash entries.
Attachment #297334 - Flags: review?(igor) → review+
Attachment #297334 - Flags: superreview?(jst)
Attachment #297231 - Flags: superreview?(jst)
(In reply to comment #10)
> (From update of attachment 297334 [details] [diff] [review])
> So how about we take this one then? I could rename GetClonedFunction into
> GetFunction...

A better name would be NewFunctionObject to emphases that this is not a getter but something that always allocates.
Blocks: 412611
Agree on better New* name implying allocation.

The economics shifted, maybe: JSFunction used to be bigger. Not sure there was any other reason for cloning. Great to get rid of it finally.

/be
Attachment #297334 - Flags: superreview?(jst) → superreview+
Had to back this out, it triggers

###!!! ASSERTION: Event listener manager hash not empty at shutdown!: 'sEventListenerManagersHash.entryCount == 0', file ../../../../source/mozilla/content/base/src/nsContentUtils.cpp, line 807

when running bloattest. A bit confusing, as this should make us leak less.
What's happening is that the last window and a couple of elements are destroyed from the JSGC_END callback in XPCJSRuntime::GCCallback (deferred releases). The elements are destroyed after the last window, destruction of the last window triggers layout shutdown, which asserts because the elements that haven't been destroyed yet are still in the hash. We can't really ensure that they are destroyed in the order that nsContentUtils expects, so we should either remove the assertion or trigger it from a different spot (after the last cycle collection?).
And I think that situation can already happen currently, but because we keep the hidden window alive far into shutdown we don't trigger the assert.
Attached patch Additional patch v1 (obsolete) — Splinter Review
This removes the assertion and makes us shut down the hash in nsContentUtils::Shutdown if there are no more event listener managers alive, or when the last event listener manager is removed after nsContentUtils::Shutdown.
Attachment #297827 - Flags: superreview?
Attachment #297827 - Flags: review?
Comment on attachment 297827 [details] [diff] [review]
Additional patch v1

r+sr=jst. I'm a bit sad to see this assertion go, as it has been a helpful indication about us leaking things before, though I realize the need to remove it.

I wonder if it'd be worth leaving it in as a warning, to still at least partially serve the same purpose?
Attachment #297827 - Flags: superreview?
Attachment #297827 - Flags: superreview+
Attachment #297827 - Flags: review?
Attachment #297827 - Flags: review+
It sounds like it fires all the time, so having it as a warning would just be noise.

Trace-refcnt does catch the leaks the assertion used to complain about, right?
You could also call nsLayoutStatics::AddRef/Release in the nodeinfomanagers ctor/dtor. That'll keep us from shutting down until the last node is destroyed.
(In reply to comment #19)
> Trace-refcnt does catch the leaks the assertion used to complain about, right?

It won't catch the leaking of the hash itself, but it will catch the leaking of the event listener manager that's still in the hash.

(In reply to comment #20)
> You could also call nsLayoutStatics::AddRef/Release in the nodeinfomanagers
> ctor/dtor. That'll keep us from shutting down until the last node is destroyed.

Maybe that's a better idea, yes. Though it would lead to bigger shutdown leaks whenever a node leaks.
Attached patch Additional patch v2 (obsolete) — Splinter Review
This leaves the assertion, makes nodeinfo managers keep nsLayoutStatics alive. Since documents keep their nodeinfo manager alive they don't need to keep nsLayoutStatics themselves. Feel free to r+sr.
Attachment #297827 - Attachment is obsolete: true
Attachment #297935 - Flags: superreview?(jst)
Attachment #297935 - Flags: review?(jonas)
Attachment #297935 - Flags: superreview?(jst)
Attachment #297935 - Flags: superreview+
Attachment #297935 - Flags: review?(jonas)
Attachment #297935 - Flags: review+
I've never seen us leak just elemenets anyway so in reality we're likely leaking big on shutdown if we leak an element.
It's possible to leak elements without leaking a *whole* lot more (e.g. documents or windows).  See bug 407072 for an example where 2644 total bytes leak according to trace-refcnt stats.
Attached patch Patch that was checked in (obsolete) — Splinter Review
Attachment #297231 - Attachment is obsolete: true
Attachment #297334 - Attachment is obsolete: true
Attachment #297935 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 413264
Attachment #298140 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.