Bulletproof RDF globals for xpcom-restart

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
RDF
P3
normal
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

Trunk
mozilla1.9alpha1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

14 years ago
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.
(Assignee)

Comment 1

14 years ago
Created attachment 147060 [details] [diff] [review]
Fix globals in rdfservice and filesystemdatasource
(Assignee)

Updated

14 years ago
Attachment #147060 - Flags: superreview?(darin)
Attachment #147060 - Flags: review?(axel)

Comment 2

14 years ago
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?
Attachment #147060 - Flags: review?(axel) → review-

Comment 3

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

request sr again when axel has given r=
Attachment #147060 - Flags: superreview?(darin)
(Assignee)

Comment 4

14 years ago
Created attachment 147723 [details] [diff] [review]
RDFservice globals v2
Attachment #147060 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #147723 - Flags: review?(axel)

Comment 5

14 years ago
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.
Attachment #147723 - Flags: review?(axel) → review-
(Assignee)

Comment 6

14 years ago
Created attachment 148299 [details] [diff] [review]
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.
(Assignee)

Updated

14 years ago
Attachment #147723 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #148299 - Flags: review?(axel)

Comment 7

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

Comment 8

14 years ago
(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

14 years ago
oh, heh :)
(Assignee)

Updated

14 years ago
Attachment #148299 - Flags: superreview?(alecf)

Comment 10

14 years ago
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
Attachment #148299 - Flags: review?(axel) → review+

Comment 11

14 years ago
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

14 years ago
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?
(Assignee)

Updated

13 years ago
Priority: -- → P3
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Updated

13 years ago
Attachment #148299 - Flags: superreview?(alecf) → superreview?(neil.parkwaycc.co.uk)

Comment 13

13 years ago
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

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

setting the flag helps ;-)
Attachment #148299 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
(Assignee)

Comment 15

13 years ago
Created attachment 204688 [details] [diff] [review]
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.
(Assignee)

Comment 16

13 years ago
Created attachment 204689 [details] [diff] [review]
again
(Assignee)

Comment 17

13 years ago
Oh well, interdiff doesn't work, checked in on trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 18

13 years ago
killed BeOS build:
https://bugzilla.mozilla.org/show_bug.cgi?id=318669#c4
You need to log in before you can comment on or make changes to this bug.