Add cycle collection to RDF datasources

RESOLVED FIXED

Status

defect
RESOLVED FIXED
13 years ago
11 months ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

13 years ago
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.
Assignee

Comment 1

13 years ago
Posted patch v1 (obsolete) — Splinter Review
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Assignee

Updated

13 years ago
Assignee

Comment 2

13 years ago
Posted patch v1.1 (obsolete) — Splinter Review
Do let me know if you don't feel like reviewing this or you don't have time.
Attachment #257380 - Attachment is obsolete: true
Attachment #258007 - Flags: review?
Assignee

Updated

13 years ago
Attachment #258007 - Flags: review? → review?(benjamin)

Comment 3

12 years ago
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.
Attachment #258007 - Flags: review?(benjamin) → review-
Assignee

Comment 4

12 years ago
Posted patch v1.2 (obsolete) — Splinter Review
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 :-(.
Attachment #258007 - Attachment is obsolete: true
Attachment #260019 - Flags: review?
Assignee

Updated

12 years ago
Attachment #260019 - Flags: review? → review?(benjamin)

Comment 5

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

I'd like somebody else to review the nsAgg macros as well... maybe dbaron?
Attachment #260019 - Flags: review?(benjamin) → review+
Assignee

Updated

12 years ago
Attachment #260019 - Flags: superreview?(dbaron)
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...)
Attachment #260019 - Flags: superreview?(dbaron) → superreview+
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)
 
Assignee

Comment 8

12 years ago
Posted patch v1.2Splinter Review
This is the one I'm checking in (and it compiles).
Attachment #260019 - Attachment is obsolete: true
Looks like this broke the Sunbird tinderboxes.
This also appears to have increased the leak numbers for Seamonkey
(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.
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".
I checked in the fix for the above comment with peterv's review (in person).

Updated

12 years ago
Depends on: 377423

Comment 14

12 years ago
This might have caused a shutdown crash, bug 377423.
Should this bug be marked as fixed?
Or is there still work to do on leak regressions?
Assignee

Comment 17

12 years ago
Closing this, remaining work will be tracked in different bugs (see bug 383939).
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

Updated

11 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.