372713 - Add cycle collection to RDF datasources

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect)

Description

[Peter Van der Beken [:peterv]]

RDF datasources have weird refcounting to deal with cycles between them. We should use the cycle collector to deal with the cycles.
Additionally this solved some leaks of windows that I was seeing where a datasource kept a RDF template processor alive which kept a XUL element alive. The datasource was eventually released as part of the final cycle collection releasing other cycles, but the XUL element was in a small cycle with a JS event listener and because final cycle collection had already run that cycle wasn't released anymore, leaking the global object and thus the window. Making the datasources participate made us collect all the cycles during the final cycle collection.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Do let me know if you don't feel like reviewing this or you don't have time.

Comment 3

[Benjamin Smedberg]

Comment on attachment 258007 [details] [diff] [review]
v1.1

>Index: rdf/datasource/src/nsFileSystemDataSource.cpp

> FileSystemDataSource::AddObserver(nsIRDFObserver *n)

This, and several places below, should return NS_ERROR_NOT_IMPLEMENTED, since we don't actually use the observers.

>Index: rdf/datasource/src/nsLocalStore.cpp

>-    nsCOMPtr<nsISupportsArray> mObservers;

and again

>Index: toolkit/components/history/src/nsGlobalHistory.cpp

>+  PRUint32 i, count = mObservers.Length();
>+  for (i = 0; i < count; ++i) {
>+    rv = mObservers[i]->OnBeginUpdateBatch(this);
>   }

It would make me feel more comfortable if this and several places below counted downwards instead of upwards. We have known issues with observers removing themself during updates.

>Index: xpfe/components/search/src/nsLocalSearchService.cpp

> LocalSearchDataSource::AddObserver(nsIRDFObserver *n)

NOT_IMPLEMENTED, again.

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1.2

Turns out my changes to support cycle collection on aggregated classes wasn't correct. I've added a big comment in nsAgg.h which should explain the changes. Aggregation is painful :-(.

Comment 5

[Benjamin Smedberg]

Comment on attachment 260019 [details] [diff] [review]
v1.2

I'd like somebody else to review the nsAgg macros as well... maybe dbaron?

Comment 6

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

Comment on attachment 260019 [details] [diff] [review]
v1.2

I keep thinking I'm going to try to actually review this, but I don't really understand the nsAgg macros, and this isn't the time for me to learn.  If you really want somebody to review this, you'll have to find somebody else, but I'm happy to rubber-stamp this, so sr=dbaron.

(Sorry, should have said this last week...)

Comment 7

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

To get the above patch to compile I needed:

--- a/xpcom/base/nsAgg.h
+++ b/xpcom/base/nsAgg.h
@@ -302,7 +302,7 @@ _class::AggregatedQueryInterface(REFNSII
   {                                                                         \
     NS_ASSERTION(CheckForRightISupports(p),                                 \
                  "not the nsISupports pointer we expect");                  \
-    _class *tmp = NS_STATIC_CAST(_class*, Downcast(s));                     \
+    _class *tmp = NS_STATIC_CAST(_class*, Downcast(p));                     \
     if (!tmp->IsPartOfAggregated())                                         \
         NS_IMPL_CYCLE_COLLECTION_DESCRIBE(_class)
 

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v1.2

This is the one I'm checking in (and it compiles).

Comment 9

[(not currently active) Ted Mielczarek]

Looks like this broke the Sunbird tinderboxes.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This also appears to have increased the leak numbers for Seamonkey

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #10)
> This also appears to have increased the leak numbers for Seamonkey

Could this be because some of the SeaMonkey rdf components (e.g. under xpfe/components/bookmarks and mailnews/addrbook) haven't been converted to the cycle collector as well?

If so, that would imply Thunderbird leak numbers would also be worse if we had any.

Comment 12

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

I just hit a crash that I think is due to the changes this patch made to toolkit/components/history/src/nsGlobalHistory.cpp.  There are a number of occurrences of "i--" that should be "--i".

Comment 13

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

I checked in the fix for the above comment with peterv's review (in person).

Comment 14

[Jesse Ruderman]

This might have caused a shutdown crash, bug 377423.

Comment 15

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

Should this bug be marked as fixed?

Comment 16

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

Or is there still work to do on leak regressions?

Comment 17

[Peter Van der Beken [:peterv]]

Closing this, remaining work will be tracked in different bugs (see bug 383939).