Closed
Bug 241758
Opened 21 years ago
Closed 19 years ago
Bulletproof RDF globals for xpcom-restart
Categories
(Core Graveyard :: RDF, defect, P3)
Core Graveyard
RDF
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: benjamin, Assigned: benjamin)
References
Details
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
axel
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #147060 -
Flags: superreview?(darin)
Attachment #147060 -
Flags: review?(axel)
Comment 2•21 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•21 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 | ||
Updated•21 years ago
|
Attachment #147723 -
Flags: review?(axel)
Comment 5•21 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•21 years ago
|
||
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•21 years ago
|
Attachment #147723 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #148299 -
Flags: review?(axel)
Comment 7•21 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•21 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
Assignee | ||
Updated•21 years ago
|
Attachment #148299 -
Flags: superreview?(alecf)
Comment 10•21 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•21 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•21 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•19 years ago
|
Priority: -- → P3
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Updated•19 years ago
|
Attachment #148299 -
Flags: superreview?(alecf) → superreview?(neil.parkwaycc.co.uk)
Comment 13•19 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•19 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•19 years ago
|
||
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•19 years ago
|
||
Assignee | ||
Comment 17•19 years ago
|
||
Oh well, interdiff doesn't work, checked in on trunk.
Status: NEW → RESOLVED
Closed: 19 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
•