Closed Bug 198655 Opened 17 years ago Closed 17 years ago

JS Component loader cause a leak of the component manager

Categories

(Core :: XPConnect, defect)

x86
Windows XP
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.4alpha

People

(Reporter: dougt, Assigned: dbradley)

References

Details

(Whiteboard: edt_b3)

Attachments

(2 files, 2 obsolete files)

if there are any js components in my components directory, leak the component
manager impl.
Blocks: 78861
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
Attached patch patch (obsolete) — Splinter Review
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.
Attached patch diff -w of the above (obsolete) — Splinter Review
Whiteboard: edt_b3
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
Attachment #118084 - Flags: superreview+
Oops, I guess I didn't compile this after adding one of those "goto out"'s in
mozJSComponentLoader.cpp.  Fixed.
Attachment #118083 - Attachment is obsolete: true
Attachment #118084 - Attachment is obsolete: true
Comment on attachment 118093 [details] [diff] [review]
fix compile error

shaver, what do you think?  (and carrying over brendan's sr)
Attachment #118093 - Flags: superreview+
Attachment #118093 - Flags: review?(shaver)
Comment on attachment 118093 [details] [diff] [review]
fix compile error

Yeah, looks good.  Thanks for the style fixes around ResolveStandardClass,
especially. =)
Attachment #118093 - Flags: review?(shaver) → review+
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
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"?
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.
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.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.4alpha
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 -
Status: RESOLVED → VERIFIED
Yeah, brendan and I discussed that change.  It simply prevents a redundant
clearing of newborn roots.
You need to log in before you can comment on or make changes to this bug.