Closed Bug 434838 Opened 12 years ago Closed 11 years ago
Storage Connection uses ns Data Hashtable when it should use ns Interface Hashtable
Doing so means we no longer need to manually addref (in CreateFunction) or release (in s_ReleaseFuncEnum), and we don't need the s_ReleaseFuncEnum enumerating function anymore.
You did mean nsInterfaceHashtable instead of nsInstanceHashtable, right? The patch compiles and runs the tests successfully. I hope I got everything right. Understanding all that addref&release stuff is still dificult for me.
Attachment #327149 - Flags: superreview?(sdwilsh) → review?(bent.mozilla)
Comment on attachment 327149 [details] [diff] [review] v1 >+ nsInterfaceHashtable<nsCStringHashKey, nsISupports> mFunctions; I think you want to use mozIStorageFunction here instead of nsISupports (and fix all the compiler errors that that should cause). r=sdwilsh, and thanks!
Attachment #327149 - Flags: review?(sdwilsh) → review+
Comment on attachment 327149 [details] [diff] [review] v1 Please see about adding the -p option to your diffs so we can see which function you're modifying. >- // We don't hold function object anymore -- remove ref to it >- NS_RELEASE(func); This will leak. The Get() function is an XPCOM getter (i.e. it always addrefs before returning). You should either change the Get call to use 'nsnull' instead of func (since it doesn't appear to actually be necessary) or make func an 'nsCOMPtr<nsISupports>' and use getter_AddRefs in the call to Get. >+ nsInterfaceHashtable<nsCStringHashKey, nsISupports> mFunctions; Agreed that this should be changed to a more specific interface if possible.
Attachment #327149 - Flags: review?(bent.mozilla) → review-
Summary: mozStorageConnection uses nsDataHashtable when it should use nsInstanceHashtable → mozStorageConnection uses nsDataHashtable when it should use nsInterfaceHashtable
Easy fix for that is to change http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageConnection.cpp#774 to an nsCOMPtr, and use getter_AddRefs(func) when you call get. It will actually fix another potential leak of the sqlite function call fails too.
Fixed the leak by removing the func variable (it isn't used anyway) and attached the patch with function names and 8 lines of context. But according to the IDL, both mozIStorageFunction and mozIStorageAggregateFunction inherit directly from nsISupports. So I don't know what more specific interface I can use there.
Attachment #327149 - Attachment is obsolete: true
Attachment #327150 - Flags: review?(bent.mozilla)
Comment on attachment 327150 [details] [diff] [review] v2 (In reply to comment #5) > But according to the IDL, both mozIStorageFunction and > mozIStorageAggregateFunction inherit directly from nsISupports. Bummer :( The rest looks fine.
Attachment #327150 - Flags: review?(bent.mozilla) → review+
Thanks for the help and the fast review :)
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/4f80171d205e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug]
Target Milestone: --- → mozilla1.9.1a1
You need to log in before you can comment on or make changes to this bug.