Closed
Bug 383939
Opened 18 years ago
Closed 17 years ago
All RDF datasources must implement cycle collection to avoid leaking
Categories
(Core Graveyard :: RDF, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: sayrer)
References
(Depends on 1 open bug)
Details
(Keywords: memory-leak)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
benjamin
:
review+
benjamin
:
superreview+
|
Details | Diff | Splinter Review |
Since the composite datasource has been switched to cycle collection instead of doing the special refcounting it used to do, we MUST have all RDF datasources implement cycle collection if we're going to avoid leaking all sorts of stuff. Including all RDF datasources in all extensions, and so forth...
Flags: blocking1.9?
Comment 1•18 years ago
|
||
This could explain the jscontext & dom window leak I've been trying to track down in Bug 381992.
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → sayrer
Comment 2•17 years ago
|
||
Marking as blocking. Sayre to take a swing.
Flags: blocking1.9? → blocking1.9-
Updated•17 years ago
|
Flags: blocking1.9- → blocking1.9+
Comment 3•17 years ago
|
||
Sayre this still on the list?
Assignee | ||
Comment 4•17 years ago
|
||
Yep.
Assignee | ||
Updated•17 years ago
|
Priority: -- → P3
Assignee | ||
Comment 5•17 years ago
|
||
cycle-collected class
[x] nsGlobalHistory
[ ] LocalStoreImpl
[ ] FileSystemDataSource
[x] InMemoryDataSource
[x] CompositeDataSourceImpl
[x] RDFXMLDataSourceImpl
[x] nsChromeUIDataSource
[ ] nsHTTPIndex (used?)
[x] nsWindowDataSource
[x] nsCharsetMenu
[ ] mozSqlService (used?)
Assignee | ||
Comment 6•17 years ago
|
||
> Including all RDF datasources in all extensions, and so forth...
Not sure what we can do about extensions.
Assignee | ||
Comment 7•17 years ago
|
||
Attachment #298700 -
Flags: review?
Assignee | ||
Updated•17 years ago
|
Attachment #298700 -
Flags: review? → review?(benjamin)
Assignee | ||
Comment 8•17 years ago
|
||
FileSystemDataSource looks unused. Can we delete it?
Assignee | ||
Comment 9•17 years ago
|
||
now with traversal from nsXULDocument
Attachment #298700 -
Attachment is obsolete: true
Attachment #298701 -
Flags: superreview?(peterv)
Attachment #298701 -
Flags: review?(benjamin)
Attachment #298700 -
Flags: review?(benjamin)
Comment 10•17 years ago
|
||
(In reply to comment #8)
> FileSystemDataSource looks unused. Can we delete it?
Not sure if extensions use it. I'd say as gecko1.9 is feature frozen, we shouldn't.
Assignee | ||
Comment 11•17 years ago
|
||
(In reply to comment #10)
>
> Not sure if extensions use it. I'd say as gecko1.9 is feature frozen, we
> shouldn't.
Everytime I try to delete something, you say gecko1.9 is feature frozen. It isn't.
Mark: can you make sure that whatever extension authors need to do for this is documented? You might have to ask sayrer what that actually is. :)
Assignee | ||
Comment 13•17 years ago
|
||
well, Mossop tells me people actually do use it in extensions. so I guess we get to keep it.
Comment 14•17 years ago
|
||
(In reply to comment #13)
> well, Mossop tells me people actually do use it in extensions. so I guess we
> get to keep it.
>
He's right. It's also used in XULRuner apps, but those are less of a concern and can be worked around easier.
Assignee | ||
Comment 15•17 years ago
|
||
Attachment #298701 -
Attachment is obsolete: true
Attachment #298721 -
Flags: superreview?(peterv)
Attachment #298721 -
Flags: review?(benjamin)
Attachment #298701 -
Flags: superreview?(peterv)
Attachment #298701 -
Flags: review?(benjamin)
Assignee | ||
Comment 16•17 years ago
|
||
nsHTTPIndex is going to be more work, because it has a threadsafe impl.
Reporter | ||
Comment 17•17 years ago
|
||
So the reason nsFileSystemDataSource didn't have cycle collection is that it has no refs to anything, so can't participate in a cycle. It probably doesn't hurt that much to just have the no-op traverse/unlink this gives it. Same for FileSystemDataSource.
Good catch on the XULDocument link to localstore! That might be what I missed the last time I tried to give it cycle collection (that time didn't fix the leak I was looking at).
As for extensions... if they implement their stuff in JS, there is no problem. If an extension implements a datasource in C++, and doesn't implement cycle collection on it, and it holds references to something interesting, we'll likely leak. Not much we can do about that.
Comment 18•17 years ago
|
||
Comment on attachment 298721 [details] [diff] [review]
Add FileSystemDataSource
I really think that we ought to be tracing mRDFService too, but sayrer says it's currently marked as threadsafe (WTF!)... so let's at least make sure we have a bug on that please.
Attachment #298721 -
Flags: superreview?(peterv)
Attachment #298721 -
Flags: superreview+
Attachment #298721 -
Flags: review?(benjamin)
Attachment #298721 -
Flags: review+
Assignee | ||
Comment 19•17 years ago
|
||
OK, that's checked in.
cycle-collected class
[x] nsGlobalHistory
[x] LocalStoreImpl
[x] FileSystemDataSource
[x] InMemoryDataSource
[x] CompositeDataSourceImpl
[x] RDFXMLDataSourceImpl
[x] nsChromeUIDataSource
[ ] nsHTTPIndex (used?)
[x] nsWindowDataSource
[x] nsCharsetMenu
[ ] mozSqlService (used?)
Assignee | ||
Comment 20•17 years ago
|
||
I had to back this out because it turned the tree orange. On the upside, that's a good sign this was probably worth doing.
Updated•17 years ago
|
Flags: tracking1.9+ → blocking1.9+
Priority: P3 → P2
Assignee | ||
Comment 21•17 years ago
|
||
(In reply to comment #19)
>
> [ ] mozSqlService (used?)
I'm not going to fix this one, it looks like a dead extension.
Assignee | ||
Comment 22•17 years ago
|
||
nsHTTPIndex will be hard to fix, and it's not on by default. I filed bug 421202 on that, nominate that bug for blocking if I've missed something.
Status: NEW → RESOLVED
Closed: 17 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
•