Bug 666414 - Prevent AddRef and Release from being called on the pointers wrapped in nsCOMPtr and nsRefPtr; r=bsmedberg
authorEhsan Akhgari <ehsan@mozilla.com>
Fri, 16 Sep 2011 16:22:44 -0400
changeset 77583 489f9e746213f7d4ae2ac51b3a0fb672e211c462
parent 77582 24bc89c8bcbe0ba51b8dff660bdd59cfcbb47c09
child 77584 35196c69cb12f5996d5681e6a8a577d7c3480546
push id3
push userfelipc@gmail.com
push dateFri, 30 Sep 2011 20:09:13 +0000
reviewersbsmedberg
bugs666414
milestone9.0a1
Bug 666414 - Prevent AddRef and Release from being called on the pointers wrapped in nsCOMPtr and nsRefPtr; r=bsmedberg
content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
content/xbl/src/nsXBLDocumentInfo.h
dom/base/nsGlobalWindow.cpp
dom/plugins/base/nsNPAPIPlugin.cpp
dom/plugins/base/nsPluginInstanceOwner.cpp
dom/src/notification/nsDesktopNotification.cpp
editor/libeditor/text/nsPlaintextEditor.cpp
gfx/thebes/GLContextProviderEGL.cpp
gfx/thebes/GLContextProviderGLX.cpp
modules/libpref/src/nsPrefBranch.h
netwerk/protocol/http/nsHttpConnectionMgr.cpp
toolkit/components/places/tests/cpp/test_IHistory.cpp
view/src/nsViewManager.cpp
widget/src/windows/TaskbarPreview.cpp
xpcom/base/nsAutoPtr.h
xpcom/build/Omnijar.cpp
xpcom/components/nsComponentManager.cpp
xpcom/glue/nsCOMPtr.h
xpcom/glue/nsInterfaceHashtable.h
xpcom/tests/TestHarness.h
--- a/content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
+++ b/content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ -4308,18 +4308,18 @@ nsCanvasRenderingContext2DAzure::GetTheb
       return NS_ERROR_FAILURE;
     }
   } else {
     // Normally GetThebesSurfaceForDrawTarget will handle the flush, when
     // we're returning a cached ThebesSurface we need to flush here.
     mTarget->Flush();
   }
 
-  mThebesSurface->AddRef();
   *surface = mThebesSurface;
+  NS_ADDREF(*surface);
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsCanvasRenderingContext2DAzure::CreateImageData()
 {
   /* Should never be called; handled entirely in the quickstub */
--- a/content/xbl/src/nsXBLDocumentInfo.h
+++ b/content/xbl/src/nsXBLDocumentInfo.h
@@ -53,17 +53,17 @@ class nsXBLDocumentInfo : public nsIScri
 {
 public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
 
   nsXBLDocumentInfo(nsIDocument* aDocument);
   virtual ~nsXBLDocumentInfo();
 
   already_AddRefed<nsIDocument> GetDocument()
-    { NS_ADDREF(mDocument); return mDocument.get(); }
+    { nsCOMPtr<nsIDocument> copy = mDocument; return copy.forget(); }
 
   PRBool GetScriptAccess() { return mScriptAccess; }
 
   nsIURI* DocumentURI() { return mDocument->GetDocumentURI(); }
 
   nsXBLPrototypeBinding* GetPrototypeBinding(const nsACString& aRef);
   nsresult SetPrototypeBinding(const nsACString& aRef,
                                nsXBLPrototypeBinding* aBinding);
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -499,18 +499,18 @@ nsDummyJavaPluginOwner::SetInstance(nsNP
   mInstance = aInstance;
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDummyJavaPluginOwner::GetInstance(nsNPAPIPluginInstance **aInstance)
 {
-  NS_IF_ADDREF(mInstance);
   *aInstance = mInstance;
+  NS_IF_ADDREF(*aInstance);
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDummyJavaPluginOwner::GetWindow(NPWindow *&aWindow)
 {
   aWindow = nsnull;
@@ -8958,25 +8958,27 @@ nsGlobalWindow::SetTimeoutOrInterval(nsI
 
     timeout->mWhen = TimeStamp::Now() + delta;
 
     timeout->mTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
     if (NS_FAILED(rv)) {
       return rv;
     }
 
+    nsRefPtr<nsTimeout> copy = timeout;
+
     rv = timeout->mTimer->InitWithFuncCallback(TimerCallback, timeout,
                                                realInterval,
                                                nsITimer::TYPE_ONE_SHOT);
     if (NS_FAILED(rv)) {
       return rv;
     }
 
     // The timeout is now also held in the timer's closure.
-    timeout->AddRef();
+    copy.forget();
   } else {
     // If we are frozen, however, then we instead simply set
     // timeout->mTimeRemaining to be the "time remaining" in the timeout (i.e.,
     // the interval itself). We don't create a timer for it, since that will
     // happen when we are thawed and the timeout will then get a timer and run
     // to completion.
 
     timeout->mTimeRemaining = delta;
--- a/dom/plugins/base/nsNPAPIPlugin.cpp
+++ b/dom/plugins/base/nsNPAPIPlugin.cpp
@@ -552,18 +552,18 @@ nsNPAPIPlugin::CreatePluginInstance(nsNP
     return NS_ERROR_NULL_POINTER;
 
   *aResult = NULL;
 
   nsRefPtr<nsNPAPIPluginInstance> inst = new nsNPAPIPluginInstance(this);
   if (!inst)
     return NS_ERROR_OUT_OF_MEMORY;
 
-  NS_ADDREF(inst);
   *aResult = inst;
+  NS_ADDREF(*aResult);
   return NS_OK;
 }
 
 nsresult
 nsNPAPIPlugin::Shutdown()
 {
   NPP_PLUGIN_LOG(PLUGIN_LOG_BASIC,
                  ("NPP Shutdown to be called: this=%p\n", this));
--- a/dom/plugins/base/nsPluginInstanceOwner.cpp
+++ b/dom/plugins/base/nsPluginInstanceOwner.cpp
@@ -471,18 +471,18 @@ NS_IMETHODIMP nsPluginInstanceOwner::Get
 {
   return CallQueryInterface(mContent, result);
 }
 
 nsresult nsPluginInstanceOwner::GetInstance(nsNPAPIPluginInstance **aInstance)
 {
   NS_ENSURE_ARG_POINTER(aInstance);
 
-  NS_IF_ADDREF(mInstance);
   *aInstance = mInstance;
+  NS_IF_ADDREF(*aInstance);
   return NS_OK;
 }
 
 NS_IMETHODIMP nsPluginInstanceOwner::GetURL(const char *aURL,
                                             const char *aTarget,
                                             nsIInputStream *aPostStream,
                                             void *aHeadersData,
                                             PRUint32 aHeadersDataLen)
--- a/dom/src/notification/nsDesktopNotification.cpp
+++ b/dom/src/notification/nsDesktopNotification.cpp
@@ -135,20 +135,20 @@ nsDOMDesktopNotification::nsDOMDesktopNo
       return;
 
     // because owner implements nsITabChild, we can assume that it is
     // the one and only TabChild for this docshell.
     TabChild* child = GetTabChildFrom(mOwner->GetDocShell());
     
     // Retain a reference so the object isn't deleted without IPDL's knowledge.
     // Corresponding release occurs in DeallocPContentPermissionRequest.
-    request->AddRef();
+    nsRefPtr<nsDesktopNotificationRequest> copy = request;
 
     nsCString type = NS_LITERAL_CSTRING("desktop-notification");
-    child->SendPContentPermissionRequestConstructor(request, type, IPC::URI(mURI));
+    child->SendPContentPermissionRequestConstructor(copy.forget().get(), type, IPC::URI(mURI));
     
     request->Sendprompt();
     return;
   }
 
   // otherwise, dispatch it
   NS_DispatchToMainThread(request);
 
--- a/editor/libeditor/text/nsPlaintextEditor.cpp
+++ b/editor/libeditor/text/nsPlaintextEditor.cpp
@@ -1712,18 +1712,18 @@ nsPlaintextEditor::SelectEntireDocument(
   }
 
   return NS_OK;
 }
 
 already_AddRefed<nsIDOMEventTarget>
 nsPlaintextEditor::GetDOMEventTarget()
 {
-  NS_IF_ADDREF(mEventTarget);
-  return mEventTarget.get();
+  nsCOMPtr<nsIDOMEventTarget> copy = mEventTarget;
+  return copy.forget();
 }
 
 
 nsresult
 nsPlaintextEditor::SetAttributeOrEquivalent(nsIDOMElement * aElement,
                                             const nsAString & aAttribute,
                                             const nsAString & aValue,
                                             PRBool aSuppressTransaction)
--- a/gfx/thebes/GLContextProviderEGL.cpp
+++ b/gfx/thebes/GLContextProviderEGL.cpp
@@ -1549,20 +1549,18 @@ public:
         }
 
         sEGLLibrary.fUnlockSurfaceKHR(EGL_DISPLAY(), mSurface);
         mIsLocked = PR_FALSE;
     }
 
     virtual already_AddRefed<gfxASurface> GetBackingSurface()
     {
-        if (mBackingSurface) {
-            NS_ADDREF(mBackingSurface);
-        }
-        return mBackingSurface.get();
+        nsRefPtr<gfxASurface> copy = mBackingSurface;
+        return copy.forget();
     }
 
     virtual PRBool CreateEGLSurface(gfxASurface* aSurface)
     {
 #ifdef MOZ_X11
         if (!aSurface) {
             NS_WARNING("no surface");
             return PR_FALSE;
--- a/gfx/thebes/GLContextProviderGLX.cpp
+++ b/gfx/thebes/GLContextProviderGLX.cpp
@@ -874,18 +874,18 @@ public:
 
     virtual void ReleaseTexture()
     {
         sGLXLibrary.ReleaseTexImage(mPixmap);
     }
 
     virtual already_AddRefed<gfxASurface> GetBackingSurface()
     {
-        NS_ADDREF(mUpdateSurface);
-        return mUpdateSurface.get();
+        nsRefPtr<gfxASurface> copy = mUpdateSurface;
+        return copy.forget();
     }
 
     virtual PRBool InUpdate() const { return mInUpdate; }
 
     virtual GLuint GetTextureID() {
         return mTexture;
     };
 
--- a/modules/libpref/src/nsPrefBranch.h
+++ b/modules/libpref/src/nsPrefBranch.h
@@ -153,20 +153,18 @@ class PrefCallback : public PLDHashEntry
       return const_cast<PrefCallback*>(this);
     }
 
     // Get a reference to the callback's observer, or null if the observer was
     // weakly referenced and has been destroyed.
     already_AddRefed<nsIObserver> GetObserver() const
     {
       if (!IsWeak()) {
-        NS_IF_ADDREF(mStrongRef);
-        // We need to call get() here because we can't convert an nsCOMPtr to
-        // an already_AddRefed.
-        return mStrongRef.get();
+        nsCOMPtr<nsIObserver> copy = mStrongRef;
+        return copy.forget();
       }
 
       nsCOMPtr<nsIObserver> observer = do_QueryReferent(mWeakRef);
       return observer.forget();
     }
 
     const nsCString& GetDomain() const
     {
--- a/netwerk/protocol/http/nsHttpConnectionMgr.cpp
+++ b/netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ -1584,18 +1584,18 @@ nsHalfOpenSocket::OnOutputStreamReady(ns
         // mgr perceives this socket as suitable for persistent connection reuse
         conn->SetIdleTimeout(NS_MIN((PRUint16) 5, gHttpHandler->IdleTimeout()));
 
         // After about 1 second allow for the possibility of restarting a
         // transaction due to server close. Keep at sub 1 second as that is the
         // minimum granularity we can expect a server to be timing out with.
         conn->SetIsReusedAfter(950);
 
-        NS_ADDREF(conn);  // because onmsg*() expects to drop a reference
-        gHttpHandler->ConnMgr()->OnMsgReclaimConnection(NS_OK, conn);
+        nsRefPtr<nsHttpConnection> copy(conn);  // because onmsg*() expects to drop a reference
+        gHttpHandler->ConnMgr()->OnMsgReclaimConnection(NS_OK, conn.forget().get());
     }
 
     return rv;
 }
 
 // method for nsITransportEventSink
 NS_IMETHODIMP
 nsHttpConnectionMgr::nsHalfOpenSocket::OnTransportStatus(nsITransport *trans,
--- a/toolkit/components/places/tests/cpp/test_IHistory.cpp
+++ b/toolkit/components/places/tests/cpp/test_IHistory.cpp
@@ -394,17 +394,18 @@ test_observer_topic_dispatched()
   PRBool urisEqual;
   nsresult rv = visitedURI->Equals(notVisitedURI, &urisEqual);
   do_check_success(rv);
   do_check_false(urisEqual);
   addURI(visitedURI);
 
   // Need two Link objects as well - one for each URI.
   nsCOMPtr<Link> visitedLink(new mock_Link(expect_visit, false));
-  NS_ADDREF(visitedLink); // It will release itself when notified.
+  nsCOMPtr<Link> visitedLinkCopy = visitedLink;
+  visitedLinkCopy.forget(); // It will release itself when notified.
   nsCOMPtr<Link> notVisitedLink(new mock_Link(expect_no_visit));
 
   // Add the right observers for the URIs to check results.
   bool visitedNotified = false;
   nsCOMPtr<nsIObserver> vistedObs =
     new statusObserver(visitedURI, true, visitedNotified);
   bool notVisitedNotified = false;
   nsCOMPtr<nsIObserver> unvistedObs =
--- a/view/src/nsViewManager.cpp
+++ b/view/src/nsViewManager.cpp
@@ -1420,18 +1420,18 @@ NS_IMETHODIMP nsViewManager::SetViewZInd
     UpdateView(view, NS_VMREFRESH_NO_SYNC);
   }
 
   return rv;
 }
 
 NS_IMETHODIMP nsViewManager::GetDeviceContext(nsDeviceContext *&aContext)
 {
-  NS_IF_ADDREF(mContext);
   aContext = mContext;
+  NS_IF_ADDREF(aContext);
   return NS_OK;
 }
 
 void nsViewManager::TriggerRefresh(PRUint32 aUpdateFlags)
 {
   if (!IsRootVM()) {
     RootViewManager()->TriggerRefresh(aUpdateFlags);
     return;
--- a/widget/src/windows/TaskbarPreview.cpp
+++ b/widget/src/windows/TaskbarPreview.cpp
@@ -96,18 +96,18 @@ GetRenderingContext(nsIDocShell *shell, 
 
   if (!ctx) {
     // create the canvas rendering context
     ctx = do_CreateInstance("@mozilla.org/content/2dthebes-canvas-rendering-context;1", &rv);
     if (NS_FAILED(rv)) {
       NS_WARNING("Could not create nsICanvasRenderingContext2D for tab previews!");
       return rv;
     }
-    NS_ADDREF(ctx);
     gCtx = ctx;
+    NS_ADDREF(gCtx);
   }
 
   nsCOMPtr<nsICanvasRenderingContextInternal> ctxI = do_QueryInterface(ctx, &rv);
   if (NS_FAILED(rv))
     return rv;
 
   // Set the surface we'll use to render.
   return ctxI->InitializeWithSurface(shell, surface, width, height);
--- a/xpcom/base/nsAutoPtr.h
+++ b/xpcom/base/nsAutoPtr.h
@@ -1044,21 +1044,21 @@ class nsRefPtr
 
             Prefer the implicit use of this operator to calling |get()|, except where
             necessary to resolve ambiguity.
           */
         {
           return get();
         }
 
-      T*
+      nsCOMPtr_base::nsDerivedSafe<T>*
       operator->() const
         {
           NS_PRECONDITION(mRawPtr != 0, "You can't dereference a NULL nsRefPtr with operator->().");
-          return get();
+          return reinterpret_cast<nsCOMPtr_base::nsDerivedSafe<T>*> (get());
         }
 
       // This operator is needed for gcc <= 4.0.* and for Sun Studio; it
       // causes internal compiler errors for some MSVC versions.  (It's not
       // clear to me whether it should be needed.)
 #ifndef _MSC_VER
       template <class U, class V>
       U&
--- a/xpcom/build/Omnijar.cpp
+++ b/xpcom/build/Omnijar.cpp
@@ -112,17 +112,17 @@ Omnijar::InitOne(nsIFile *aPath, Type aT
     if (NS_FAILED(zipReader->OpenArchive(file))) {
         delete zipReader;
         return;
     }
 
     CleanUpOne(aType);
     sReader[aType] = zipReader;
     sPath[aType] = file;
-    NS_IF_ADDREF(file);
+    NS_IF_ADDREF(sPath[aType]);
 }
 
 void
 Omnijar::Init(nsIFile *aGrePath, nsIFile *aAppPath)
 {
     InitOne(aGrePath, GRE);
     InitOne(aAppPath, APP);
     sInitialized = PR_TRUE;
--- a/xpcom/components/nsComponentManager.cpp
+++ b/xpcom/components/nsComponentManager.cpp
@@ -1982,18 +1982,19 @@ nsFactoryEntry::GetFactory()
         }
         else {
             NS_ASSERTION(mCIDEntry->constructorProc, "no getfactory or constructor");
             mFactory = new mozilla::GenericFactory(mCIDEntry->constructorProc);
         }
         if (!mFactory)
             return NULL;
     }
-    NS_ADDREF(mFactory);
-    return mFactory.get();
+    nsIFactory* factory = mFactory.get();
+    NS_ADDREF(factory);
+    return factory;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Static Access Functions
 ////////////////////////////////////////////////////////////////////////////////
 
 nsresult
 NS_GetComponentManager(nsIComponentManager* *result)
--- a/xpcom/glue/nsCOMPtr.h
+++ b/xpcom/glue/nsCOMPtr.h
@@ -424,16 +424,41 @@ nsCOMPtr_base
       
         template <class T> class Foo { ... };
         template <> class Foo<void*> { ... };
         template <class T> class Foo<T*> : private Foo<void*> { ... };
     */
   {
     public:
 
+      template <class T>
+      class
+        NS_FINAL_CLASS
+        NS_STACK_CLASS
+      nsDerivedSafe : public T
+          /*
+            No client should ever see or have to type the name of this class.  It is the
+            artifact that makes it a compile-time error to call |AddRef| and |Release|
+            on a |nsCOMPtr|.  DO NOT USE THIS TYPE DIRECTLY IN YOUR CODE.
+
+            See |nsCOMPtr::operator->| and |nsRefPtr::operator->|.
+          */
+        {
+          private:
+            using T::AddRef;
+            using T::Release;
+
+          protected:
+            nsDerivedSafe(); // NOT TO BE IMPLEMENTED
+              /*
+                This ctor exists to avoid compile errors and warnings about nsDerivedSafe using the
+                default ctor but inheriting classes without an empty ctor.  See bug 209667.
+              */
+        };
+
       nsCOMPtr_base( nsISupports* rawPtr = 0 )
           : mRawPtr(rawPtr)
         {
           // nothing else to do here
         }
 
       NS_COM_GLUE NS_CONSTRUCTOR_FASTCALL ~nsCOMPtr_base();
 
@@ -798,21 +823,21 @@ nsCOMPtr
 
             Prefer the implicit use of this operator to calling |get()|, except where
             necessary to resolve ambiguity.
           */
         {
           return get();
         }
 
-      T*
+      nsCOMPtr_base::nsDerivedSafe<T>*
       operator->() const
         {
           NS_PRECONDITION(mRawPtr != 0, "You can't dereference a NULL nsCOMPtr with operator->().");
-          return get();
+          return reinterpret_cast<nsCOMPtr_base::nsDerivedSafe<T>*> (get());
         }
 
       nsCOMPtr<T>*
       get_address()
           // This is not intended to be used by clients.  See |address_of|
           // below.
         {
           return this;
@@ -1105,21 +1130,21 @@ class nsCOMPtr<nsISupports>
 
             Prefer the implicit use of this operator to calling |get()|, except where
             necessary to resolve ambiguity.
           */
         {
           return get();
         }
 
-      nsISupports*
+      nsDerivedSafe<nsISupports>*
       operator->() const
         {
           NS_PRECONDITION(mRawPtr != 0, "You can't dereference a NULL nsCOMPtr with operator->().");
-          return get();
+          return reinterpret_cast<nsCOMPtr_base::nsDerivedSafe<nsISupports>*> (get());
         }
 
       nsCOMPtr<nsISupports>*
       get_address()
           // This is not intended to be used by clients.  See |address_of|
           // below.
         {
           return this;
--- a/xpcom/glue/nsInterfaceHashtable.h
+++ b/xpcom/glue/nsInterfaceHashtable.h
@@ -144,18 +144,18 @@ nsInterfaceHashtable<KeyClass,Interface>
 template<class KeyClass, class Interface>
 already_AddRefed<Interface>
 nsInterfaceHashtable<KeyClass,Interface>::Get(KeyType aKey) const
 {
   typename base_type::EntryType* ent = this->GetEntry(aKey);
   if (!ent)
     return NULL;
 
-  NS_IF_ADDREF(ent->mData);
-  return already_AddRefed<Interface>(ent->mData);
+  nsCOMPtr<Interface> copy = ent->mData;
+  return copy.forget();
 }
 
 template<class KeyClass,class Interface>
 Interface*
 nsInterfaceHashtable<KeyClass,Interface>::GetWeak
   (KeyType aKey, PRBool* aFound) const
 {
   typename base_type::EntryType* ent = this->GetEntry(aKey);
--- a/xpcom/tests/TestHarness.h
+++ b/xpcom/tests/TestHarness.h
@@ -215,18 +215,18 @@ class ScopedXPCOM : public nsIDirectoryS
     PRBool failed()
     {
       return mServMgr == NULL;
     }
 
     already_AddRefed<nsIFile> GetProfileDirectory()
     {
       if (mProfD) {
-        NS_ADDREF(mProfD);
-        return mProfD.get();
+        nsCOMPtr<nsIFile> copy = mProfD;
+        return copy.forget();
       }
 
       // Create a unique temporary folder to use for this test.
       nsCOMPtr<nsIFile> profD;
       nsresult rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR,
                                            getter_AddRefs(profD));
       NS_ENSURE_SUCCESS(rv, nsnull);