189296 - Plugin code takes address of nsCOMPtr's

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[David Bradley]

This is a risky thing to do, since there is no address of operator, and you'll
get the address of the object, and not necessarily the address of the pointer
inside. Currently they are the same so things work fine now.

Comment 1

[David Bradley]

Attachment: Uses raw pointers and then assigns

Comment 2

[Adam Lock]

Comment on attachment 111692 [details] [diff] [review]
Uses raw pointers and then assigns

r=adamlock

Thanks for catching this, looks right to me

Comment 3

[Bradley Baetz (:bbaetz)]

Whast wrong with getter_AddRefs?

Comment 4

[David Bradley]

The output parameter is taking a void *, and in some cases a void ** instead of
an nsISupports pointer, so I figured it wouldn't work.

Comment 5

[serge (gone)]

Comment on attachment 111692 [details] [diff] [review]
Uses raw pointers and then assigns

slightly corrected logic for
/cvsroot/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp,v
>retrieving revision 1.459
>diff -u -r1.459 nsPluginHostImpl.cpp
>--- modules/plugin/base/src/nsPluginHostImpl.cpp	8 Jan 2003 22:11:23 -0000	1.459
>+++ modules/plugin/base/src/nsPluginHostImpl.cpp	16 Jan 2003 13:09:41 -0000
>@@ -6588,9 +6588,11 @@
>   mStreamConverter = finalStreamListener;
>   mRemoveMagicNumber = PR_TRUE;
> 
>+  nsIStreamListener * temp = nsnull;
nsIStreamListener * temp = finalStreamListener;

>   //get nsPluginStreamListenerPeer* ptr from finalStreamListener 
>   nsPluginStreamListenerPeer *pslp = NS_REINTERPRET_CAST(nsPluginStreamListenerPeer*,
>-                                     *(NS_REINTERPRET_CAST(void**, &finalStreamListener)));
>+                                     *(NS_REINTERPRET_CAST(void**, &temp)));

and remove the next line 
>+  finalStreamListener = already_AddRefed<nsIStreamListener>(temp);

>   rv = pslp->ServeStreamAsFile(request, ctxt);
>   return rv;
> }

Comment 6

[David Bradley]

I was blindly reproducing the pattern, and missed the fact this was a cast and
not a function call. Can't this just be simplified to this, or did I miss
something? Or is there some magic behind the void ** cast I missing.

  //get nsPluginStreamListenerPeer* ptr from finalStreamListener 
  nsPluginStreamListenerPeer *pslp =
NS_REINTERPRET_CAST(nsPluginStreamListenerPeer*, finalStreamListener.get());
  rv = pslp->ServeStreamAsFile(request, ctxt);

And reassigning to me, since I've been working on the patch.

Comment 7

[serge (gone)]

finalStreamListener.get()looks good enough
I do not remember what problem w/ get forced me to use double cast:(

Comment 8

[David Bradley]

I know what you mean, and I was a little leary of simplifying it, as I've seen
similar trick to get around various compiler and other problems.

Bradley, are you ok with the already_AddRefed, or do you still think I should be
using getter_AddRefs. I don't mind changing it over, but was just a little
leary, and felt safer in the presence of void *'s to be a little more verbose.

Comment 9

[David Bradley]

Attachment: Alternative using getter_AddRefs

Ok, I dug around, and found the void** pattern for getter_AddRefs elsewhere.
It's not a direct match to what the plugin API function is doing, taking a void
* and then filling in the buffer, it's close though. So I verified that it does
do the right thing. It's definitely cleaner looking code.

So for the plugin API function that takes a void *, I cast getter_AddRef to a
pointer to a pointer nsIFace **. This passes the address of the pointer into
the API function, which it then fills in the pointer with a value, and all is
well.

Comment 10

[Bradley Baetz (:bbaetz)]

yeah, I think that thats better.

Comment 11

[Adam Lock]

Comment on attachment 111800 [details] [diff] [review]
Alternative using getter_AddRefs

The patch for XPCDocument.cpp uses a C-style cast for one call to NPN_GetValue
and NS_STATIC_CAST for the other.

Otherwise r=adamlock

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 111800 [details] [diff] [review]
Alternative using getter_AddRefs

+    NPN_GetValue(mData->pPluginInstance, NPNVDOMWindow, 
+	 NS_STATIC_CAST(nsIDOMWindow **,getter_AddRefs(mDOMWindow)));

Is that cast even needed there? Wouldn't the compiler accept
getter_AddRefs(mDOMWindow) w/o the cast? If it doesn't, then leave as is...

sr=jst

Comment 13

[David Bradley]

patched checked in

Changed the c-style cast.
getter_AddRef won't convert to a void * automatically.

Comment 14

[bmartin]

Marking Verified Fixed. 

Compared attached patch to checkins.