Closed Bug 383939 Opened 13 years ago Closed 12 years ago

All RDF datasources must implement cycle collection to avoid leaking

Categories

(Core Graveyard :: RDF, defect, P2)

x86
Linux
defect

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)

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?
Depends on: 383942
Keywords: mlk
This could explain the jscontext & dom window leak I've been trying to track down in Bug 381992.
Depends on: 387709
Assignee: nobody → sayrer
Marking as blocking.  Sayre to take a swing.
Flags: blocking1.9? → blocking1.9-
Flags: blocking1.9- → blocking1.9+
Sayre this still on the list?
Yep.
Priority: -- → P3
cycle-collected  class

            [x]  nsGlobalHistory
            [ ]  LocalStoreImpl
            [ ]  FileSystemDataSource
            [x]  InMemoryDataSource   
            [x]  CompositeDataSourceImpl
            [x]  RDFXMLDataSourceImpl
            [x]  nsChromeUIDataSource
            [ ]  nsHTTPIndex (used?)
            [x]  nsWindowDataSource
            [x]  nsCharsetMenu
            [ ]  mozSqlService (used?)
            
> Including all RDF datasources in all extensions, and so forth...

Not sure what we can do about extensions.
Attached patch cycle collect nsILocalStore (obsolete) — Splinter Review
Attachment #298700 - Flags: review?
Attachment #298700 - Flags: review? → review?(benjamin)
FileSystemDataSource looks unused. Can we delete it?
Attached patch cycle collect nsILocalStore (obsolete) — Splinter Review
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)
(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.
(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. :)
well, Mossop tells me people actually do use it in extensions. so I guess we get to keep it. 
(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.
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)
nsHTTPIndex is going to be more work, because it has a threadsafe impl.
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 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+
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?)

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.
Flags: tracking1.9+ → blocking1.9+
Priority: P3 → P2
(In reply to comment #19)
>
>             [ ]  mozSqlService (used?)

I'm not going to fix this one, it looks like a dead extension. 

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: 12 years ago
Resolution: --- → FIXED
Depends on: 476823
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.