274793 - leaking JS atoms and GC roots

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

I get these errors when trying to parse a ics file into events, and put those
events in a memory calendar:

JS engine warning: 2 atoms remain after destroying the JSRuntime.
                   These atoms may point to freed memory. Things reachable
                   through them have not been finalized.
JS engine warning: leaking GC root 'XPCVariant::mJSVal' at 0x8147e4c
JS engine warning: 1 GC root remains after destroying the JSRuntime.
                   This root may point to freed memory. Objects reachable
                   through it have not been finalized.

i see the first error all the time, but the seconds seems to be more interesting

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: testcase

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: another testcase

Using this testcase, i get warnings about leaks. But when i remove the rrule,
the warnings are gone.

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

If I remove the ref from calItemBase to the parent, the leaks are gone. There is
a circular reference: the calendar has a ref to the events, and the events back
to the calendar.

Comment 4

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

I propose making the link back to the parent be a weak ref (internally to
calEvent; we would QIReferent and pass back a strong ref to callers, or else
null, as we currently do).

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

The changes in comment 3 won't fix testcase 2 (rrule based leaks), but i guess
that has the same kind of problem (circular ref between calRecurrenceInfo and
calItemBase)

Comment 6

[Daniel Boelzle [:dbo]]

Attachment: getting rid of wrappers

I suspect that native wrappers (+circular references) are the problem, too, thus spidermonkey doesn't gc those objects. This patch evaluates to get rid of them by storing the "wrappedJSObject"s being passed items/calendars only. This fixes the unit test warnings, but doesn't make bug 367456 any better. mvl, do you see any missing code pieces?

Comment 7

[Simon Paquet [:sipaq]]

Daniel, is there a reason why you did not request review? from mvl?

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 284953 [details] [diff] [review]
getting rid of wrappers

>                 var event = Components.classes["@mozilla.org/calendar/event;1"]
>                                       .createInstance(Components.interfaces.calIEvent);
>-                item = event;
>+                item = event.wrappedJSObject;

I was sure I already posted my comments, but it seems I remembered wrong. So here it my comment: I don't like this code pattern. It totally breaks xpcom. What if calIEvent is implemented in C++? It would not work at all.
I don't have huge problems with forcing the calIEvent implementation to be written in javascript, but then we should not use the xpcom way to create an instance. But I would prefer to not force things like that, and stay more general.

And, if we get rid of the wrappers, I don't think that this would be fixed. The error might go away, but I think there are still the same circular references, and thus leaks. They are just not detected by xpconnect, because that's no longer in the loop.

Comment 9

[Daniel Boelzle [:dbo]]

(In reply to comment #8)
> I was sure I already posted my comments, but it seems I remembered wrong. So
...

Yes, we've already chatted about this some months ago. The mentioned patch was just a proof of concept that those js wrappers actually cause some leaks. After unwrapping those references, the leak warnings went away. A real fix of the problem will probably make use of weak references.


> And, if we get rid of the wrappers, I don't think that this would be fixed. The
> error might go away, but I think there are still the same circular references,
> and thus leaks. They are just not detected by xpconnect, because that's no
> longer in the loop.

Circular references per se are not provoking leaks if gc could properly track all objects that take part. Since js/native wrappers hinder the engine from doing so, we could use weak references here.
There might be more leaks, but we this shouldn't hinder us from fixing the known ones.

Comment 10

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

What's the bug number for the underlying leak problem?  It doesn't sound like this pattern should cause leaks, so if it is please file an XPConnect bug with test case.

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

I suspect that the cause of the leaks here is that there are also C++ components. calRecurrenceInfo has a list of calRecurrenceRule and calReucrrenceDate, which are  c++. There might be some cycle there, with closures, refcounting and all that.

Comment 12

[Daniel Boelzle [:dbo]]

The test case has been fixed with checkins for bug 423172: The leaking gc roots have vanished (though 5 atoms remain).