198655 - JS Component loader cause a leak of the component manager

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Doug Turner (:dougt)]

if there are any js components in my components directory, leak the component
manager impl.

Comment 1

[Brendan Eich [:brendan]]

I think we've got patches for this one.  Or did we fix something similar, that
has another bug tracking it?  Or is there a dup?  Bryner would know.

/be

Comment 2

[Brian Ryner (not reading)]

Attachment: patch

This is a combination of dbaron's patch for js component loader shutdown, and
some additional leaks that brendan and I fixed.  We were leaking some
JSPrincipals objects and also not clearing newborn roots when we should be.

Comment 3

[Brian Ryner (not reading)]

Attachment: diff -w of the above

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 118084 [details] [diff] [review]
diff -w of the above

Looks good to me, shaver might have a look.  Lose the :: before the
JS_ClearNewbornRoots call in nsXPConnect.cpp, thought -- I don't think that's
prevailing style.

/be

Comment 5

[Brian Ryner (not reading)]

Attachment: fix compile error

Oops, I guess I didn't compile this after adding one of those "goto out"'s in
mozJSComponentLoader.cpp.  Fixed.

Comment 6

[Brian Ryner (not reading)]

Attachment: diff -w of the above

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 118093 [details] [diff] [review]
fix compile error

shaver, what do you think?  (and carrying over brendan's sr)

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 118093 [details] [diff] [review]
fix compile error

Yeah, looks good.  Thanks for the style fixes around ResolveStandardClass,
especially. =)

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 118093 [details] [diff] [review]
fix compile error

Lose the :: before JS_ClearNewbornRoots, or use it consistently (but that'd be
too big a diff).

/be

Comment 10

[David Bradley]

This patch was checked in right? I'm still seeing a leak, but have the jvm and
such still leaking. So is this bug "fixed"?

Comment 11

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

Are you seeing bug 198647?

Comment 12

[David Bradley]

I was seeing the leaks from the JRE, after I this and another patch I had to
cleanup wrappers was in place. Probably is now, that some recent checkins have
created more leaks which make it more difficult to identify this problem.
Specifically bug 199465. And I've been unable to get the ref count logging to
give proper stacks on my system to add to the problem.

Is there anything more to do with respect to the JS component loader? Can we
close this bug?

I've filed bug 200030 for cleaning up the wrapped natives, but I really want to
make sure that patch is needed. Which is what I'm currently doing now.

Comment 13

[David Bradley]

I'm resolving this as fixed, as the patch was checked in, and I'm see no leak of
the component manager related to the JS Component Loader in recent builds.

Comment 14

[Phil Schwartau]

There's a difference between the patch for xpconnect/src/xpccallcontext.cpp
and the checkin for it. The patch shows an addition on line 321:
 
          if(mJSContext)
          {
321  +        JS_ClearNewbornRoots(mJSContext);


whereas bonsai shows the addition of an else-clause, farther down:

341         else
342         {
343             JS_ClearNewbornRoots(mJSContext);
344         }


Is this correct? I assume so and will mark this bug Verified,
but I wanted to point this out and get confirmation on it -

Comment 15

[Brian Ryner (not reading)]

Yeah, brendan and I discussed that change.  It simply prevents a redundant
clearing of newborn roots.