434838 - mozStorageConnection uses nsDataHashtable when it should use nsInterfaceHashtable

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Shawn Wilsher :sdwilsh]

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.

Comment 1

[Arpad Borsos [:Swatinem]]

Attachment: v1

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.

Comment 2

[Shawn Wilsher :sdwilsh]

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!

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 4

[Shawn Wilsher :sdwilsh]

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.

Comment 5

[Arpad Borsos [:Swatinem]]

Attachment: v2

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.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 7

[Arpad Borsos [:Swatinem]]

Thanks for the help and the fast review :)

Comment 8

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/4f80171d205e