241758 - Bulletproof RDF globals for xpcom-restart

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect, P3)

Description

[Benjamin Smedberg]

Going through the tree for xpcom-restart. If we leak the RDF service (which is
unfortunately pretty common), we are left with globals in the RDF modules that
aren't cleaned up. This patch doesn't fix everthing (there are more globals in
the XMLRDFDataSource and the resource staticlib that need cleaning), but it does
clean up the globals in rdfservice and the filedatasource, and it adds an
assertion if we leak on shutdown, so that devs will hopefully be more careful.

Comment 1

[Benjamin Smedberg]

Attachment: Fix globals in rdfservice and filesystemdatasource

Comment 2

[Axel Hecht]

Comment on attachment 147060 [details] [diff] [review]
Fix globals in rdfservice and filesystemdatasource

I'd like us to fail gracefully for those guys without a clue of xpcom services
and return a success value and the singleton rdf service for multiple
createInstance calls. But do assert for debug builds.
Should the datasource impls ensure that, too?

Comment 3

[Darin Fisher]

Comment on attachment 147060 [details] [diff] [review]
Fix globals in rdfservice and filesystemdatasource

request sr again when axel has given r=

Comment 4

[Benjamin Smedberg]

Attachment: RDFservice globals v2

Comment 5

[Axel Hecht]

Comment on attachment 147723 [details] [diff] [review]
RDFservice globals v2

ouch, bad me.

a) this patch misses nsFileSystemDataSource.h

b) I don't like member vars having kNC names. Could have said that the first
time around, I guess.

c) did you not make the filesystem datasource a singleton on purpose?

d) For those factory methods that do create singletons, change the name, esp.
RDFServiceImpl::Create to RDFServiceImpl::CreateSingleton.

e-) I wonder if we could do something sweet resource getter pile. Maybe just
putting that into a macro
#define GET_RESOURCE(res, ns, name) \
    rv |= mRDFService->GetResource(NS_LITERAL_CSTRING(ns name,
getter_AddRefs(res));

with a special casing for NC,
define GET_NC_RESOURCE(res, name) \
    GET_RESOURCE(res, NC_NAMESPACE_URI, name)

or so. I really stopped to early reading thru the patch the first time around.
I read 'til the end this time, rest assured.

Comment 6

[Benjamin Smedberg]

Attachment: RDFservice globals v3

Updated kFoo to mFoo and RDFServiceImpl::CreateSingleton

Yes, I removed the singleton-check from filesystemdatasource. It's not needed.

I also didn't use macros. Don't need em, and they make the code slightly less
readable IMO.

Comment 7

[Alec Flett]

Hmm.. using "k" is pretty common for static variables - these are not member
variables (i.e. they shouldn't have an "m")

Comment 8

[Axel Hecht]

(In reply to comment #7)
> Hmm.. using "k" is pretty common for static variables - these are not member
> variables (i.e. they shouldn't have an "m")

They are now, see
http://bugzilla.mozilla.org/attachment.cgi?id=148299&action=diff#mozilla/rdf/datasource/src/nsFileSystemDataSource.h_sec1

Comment 9

[Alec Flett]

oh, heh :)

Comment 10

[Axel Hecht]

Comment on attachment 148299 [details] [diff] [review]
RDFservice globals v3

+static NS_DEFINE_CID(mRDFServiceCID,		    NS_RDFSERVICE_CID);

was a bit eager, and isn't used anymore, AFAICT. Just drop it.

with that, r=me

Comment 11

[Axel Hecht]

Comment on attachment 148299 [details] [diff] [review]
RDFservice globals v3

+static NS_DEFINE_CID(mRDFServiceCID,		    NS_RDFSERVICE_CID);

was a bit eager, and isn't used anymore, AFAICT. Just drop it.

with that, r=me

Comment 12

[Axel Hecht]

Comment on attachment 148299 [details] [diff] [review]
RDFservice globals v3

>Index: datasource/src/nsFileSystemDataSource.cpp
>===================================================================

<...>
>+//static
>+nsresult
>+FileSystemDataSource::Create(nsISupports* aOuter, const nsIID& aIID, void **aResult)

<...>
>+    nsCOMPtr<FileSystemDataSource> self = new FileSystemDataSource();
>+    if (!self) return NS_ERROR_OUT_OF_MEMORY;
> 
>-        NS_RELEASE(kLiteralTrue);
>-        NS_RELEASE(kLiteralFalse);
>+    nsresult rv = ((FileSystemDataSource*) self)->Init();
>+    NS_ENSURE_SUCCESS(rv, rv);
> 

Why did you introduce the cast here?

Comment 13

[[e-mail]]

Comment on attachment 148299 [details] [diff] [review]
RDFservice globals v3

sr=me with these fixed:

>+    RDFServiceImpl* serv = new RDFServiceImpl();
Nit: should be nsRefPtr<RDFServiceImpl> serv; rather than wasting an nsCOMPtr on a grip.

>+    nsCOMPtr<FileSystemDataSource> self = new FileSystemDataSource();
Nit: should be nsRefPtr again, also saving you from having to cast back.

Isn't using nsCOMPtr on concrete classes on the list of heinous crimes? If NS_DECL_ISUPPORTS included static const nsIID& GetIID(); would that detect them by way of a link error (except in special cases such as nsStandardURL.cpp where an explicit definition would be provided)?

Comment 14

[[e-mail]]

Comment on attachment 148299 [details] [diff] [review]
RDFservice globals v3

setting the flag helps ;-)

Comment 15

[Benjamin Smedberg]

Attachment: RDFservice globals for checkin

Patch for checkin, this needed hand-applying so I'm going to attach it here and make sure the interdiff looks correct.

Comment 16

[Benjamin Smedberg]

Attachment: again

Comment 17

[Benjamin Smedberg]

Oh well, interdiff doesn't work, checked in on trunk.

Comment 18

[Sergei Dolgov]

killed BeOS build:
https://bugzilla.mozilla.org/show_bug.cgi?id=318669#c4