Closed
Bug 372713
Opened 18 years ago
Closed 18 years ago
Add cycle collection to RDF datasources
Categories
(Core Graveyard :: RDF, defect)
Core Graveyard
RDF
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: peterv, Assigned: peterv)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•18 years ago
|
||
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Assignee | ||
Updated•18 years ago
|
Blocks: cycle-collector
Assignee | ||
Comment 2•18 years ago
|
||
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•18 years ago
|
Attachment #258007 -
Flags: review? → review?(benjamin)
Comment 3•18 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•18 years ago
|
||
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•18 years ago
|
Attachment #260019 -
Flags: review? → review?(benjamin)
Comment 5•18 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•18 years ago
|
Attachment #260019 -
Flags: superreview?(dbaron)
Comment 6•18 years ago
|
||
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+
Comment 7•18 years ago
|
||
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•18 years ago
|
||
This is the one I'm checking in (and it compiles).
Attachment #260019 -
Attachment is obsolete: true
Comment 9•18 years ago
|
||
Looks like this broke the Sunbird tinderboxes.
Comment 10•18 years ago
|
||
This also appears to have increased the leak numbers for Seamonkey
Comment 11•18 years ago
|
||
(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•18 years ago
|
||
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•18 years ago
|
||
I checked in the fix for the above comment with peterv's review (in person).
Comment 14•18 years ago
|
||
This might have caused a shutdown crash, bug 377423.
Comment 15•18 years ago
|
||
Should this bug be marked as fixed?
Comment 16•18 years ago
|
||
Or is there still work to do on leak regressions?
Assignee | ||
Comment 17•18 years ago
|
||
Closing this, remaining work will be tracked in different bugs (see bug 383939).
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•