200030 - XPConnect should release wrappers at shutdown

Status: VERIFIED DUPLICATE of bug 180380

(Core :: XPConnect, defect)

Description

[David Bradley]

When XPConnect disconnects the wrapper from the JSObject, it should release a
reference. This will cleanup any wrappers that no longer have any references.
This appears it might be affecting the component manager on shutdown.

Comment 1

[David Bradley]

Attachment: Preliminary patch

This silences Purify's reported leaked wrapped natives. I need to do some
investigating to make sure this doesn't have any ill effects. I also need to
verify that this benefits the component manager leak. If it doesn't it may not
be as important to deal with this right now.

Comment 2

[David Bradley]

I did some preliminary testing. I removed the oji component, and ran without
this patch and the component manager didn't leak. This cleans up some shutdown
leaks, but don't think the risk is worth the reward. Removing the block of bug
78861 and moving to 1.5a

Comment 3

[David Bradley]

Pulling this back to 1.4b. I've verified that we're leaking wrapped natives
after previous patches have been applied. This gets rid of two of three shutdown
leaks in XPConnect.

I want to look a little deeper as to why these objects are leaking and make sure
that there's not a secondary reason that should be addressed. And I'm reviewing
the patch to double check that it's doing the right thing.

jband, brendan, others, feel free to take a look now and note any problems. I'll
be requesting formal reviews a little later.

We'll want to get this in soon so that it has a good amount of time to bake.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What's the difference between what this patch does and reordering ~nsXPConnect?
 Does this patch make the two comments in ~nsXPConnect invalid?

Comment 5

[David Bradley]

The first comment, yes. The code I added calls release on the natives during
shutdown. Normally this occurs when the object is finalized. It appears to be
absent in the shutdown code, and probably was done on purpose. What I'm looking
into now is why it was left out, and if the reason is still valid.

Comment 6

[John Bandhauer]

"probably was done on purpose" ??? I left a comment in the code to make it clear
that this was intended. I admit that the comment does not detail the
problems(and I don't really remember all the specific details).

http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappednative.cpp#914

What I *used* to see before giving up and not doing the releases, was a range of
places outside xpconnect that would do bad stuff upon final release if that
release came during xpcom shutdown - when various other parts of the entire
system were no longer in their 'normal' operating state. Note that we are
talking about fairly random shutdown ordering here. It may well be that many of
these cases are long since fixed. I dunno. If you can make this all be stable
then great. I long resisted messing with this since I saw perfect shutdown
cleanup as far less important than good shutdown stability. And, I just didn't
believe that all the wierd assumptions about lifetime all over the code were
very fixable. But that's just me. Perhaps the landscape has changed enough since
then. Anyway it's not my problem now :)

Comment 7

[David Bradley]

I was thinking that some of the blocking off of XPCOM's
CreateInstance/GetService during shutdown might make this viable again. But I
was still worried about other avenues that I wasn't seeing in simple tests. This
was the reason I initially pushed it back, but then was asked to take another
look to see if I can eliminate it.

There's a push to reduce shutdown leaks for embedding, and that's why I was
looking into this.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A reason not to release wrappers at shutdown is that it is (or should be)
against the rules of XPCOM to propagate releases into other modules during
module destructors, since module destructors are at the time of library
unloading (if we ever start unloading libraries) and you shouldn't attempt to
call code in libraries that have been unloaded.

I was attempting to fix the existing shutdown leak (there's really only one,
when we don't otherwise leak) related to this issue in bug 180380.  I had a
patch that worked for a little while, but wasn't really ideal.  Probably a
better approach would be to somehow disconnect the components object at some
point during shutdown so that its wrapper can be GCed before we shutdown
XPConnect.  (Or perhaps we could make a special exception for the wrapper of the
XPCComponents object during shutdown?  However, that, like the solution in this
bug, would still mean, I think, that a bunch of the JS objects involved would
leak, so I don't think we'd gain very much.)

Comment 9

[David Bradley]

I'm going to go ahead and dupe this against bug 180380 since the goal is the same.

*** This bug has been marked as a duplicate of 180380 ***

Comment 10

[Phil Schwartau]

Verified Duplicate -