Closed Bug 434838 Opened 12 years ago Closed 11 years ago

mozStorageConnection uses nsDataHashtable when it should use nsInterfaceHashtable

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1a1

People

(Reporter: sdwilsh, Assigned: Swatinem)

Details

Attachments

(1 file, 1 obsolete file)

v2
5.68 KB, patch
bent.mozilla
: review+
sdwilsh
: review+
Details | Diff | Splinter Review
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.
Whiteboard: [good first bug]
Attached patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → arpad.borsos
Status: NEW → ASSIGNED
Attachment #327149 - Flags: superreview?(sdwilsh)
Attachment #327149 - Flags: review?(sdwilsh)
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.
Attached patch v2Splinter Review
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 :)
Keywords: checkin-needed
Attachment #327150 - Flags: review+
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/4f80171d205e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Flags: in-litmus-
Keywords: checkin-needed
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.