383939 - All RDF datasources must implement cycle collection to avoid leaking

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

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...

Comment 1

[Scott MacGregor]

This could explain the jscontext & dom window leak I've been trying to track down in Bug 381992.

Comment 2

[Damon Sicore (:damons)]

Marking as blocking.  Sayre to take a swing.

Comment 3

[Mike Schroepfer]

Sayre this still on the list?

Comment 4

[Robert Sayre]

Yep.

Comment 5

[Robert Sayre]

cycle-collected  class

            [x]  nsGlobalHistory
            [ ]  LocalStoreImpl
            [ ]  FileSystemDataSource
            [x]  InMemoryDataSource   
            [x]  CompositeDataSourceImpl
            [x]  RDFXMLDataSourceImpl
            [x]  nsChromeUIDataSource
            [ ]  nsHTTPIndex (used?)
            [x]  nsWindowDataSource
            [x]  nsCharsetMenu
            [ ]  mozSqlService (used?)
            

Comment 6

[Robert Sayre]

> Including all RDF datasources in all extensions, and so forth...

Not sure what we can do about extensions.

Comment 7

[Robert Sayre]

Attachment: cycle collect nsILocalStore

Comment 8

[Robert Sayre]

FileSystemDataSource looks unused. Can we delete it?

Comment 9

[Robert Sayre]

Attachment: cycle collect nsILocalStore

now with traversal from nsXULDocument

Comment 10

[Axel Hecht]

(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.

Comment 11

[Robert Sayre]

(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.

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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. :)

Comment 13

[Robert Sayre]

well, Mossop tells me people actually do use it in extensions. so I guess we get to keep it. 

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 15

[Robert Sayre]

Attachment: Add FileSystemDataSource

Comment 16

[Robert Sayre]

nsHTTPIndex is going to be more work, because it has a threadsafe impl.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

[Benjamin Smedberg]

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.

Comment 19

[Robert Sayre]

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?)

Comment 20

[Robert Sayre]

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.

Comment 21

[Robert Sayre]

(In reply to comment #19)
>
>             [ ]  mozSqlService (used?)

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

Comment 22

[Robert Sayre]

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.