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 78894 489f9e746213f7d4ae2ac51b3a0fb672e211c462
parent 78893 24bc89c8bcbe0ba51b8dff660bdd59cfcbb47c09
child 78895 35196c69cb12f5996d5681e6a8a577d7c3480546
push id78
push userclegnitto@mozilla.com
push dateFri, 16 Dec 2011 17:32:24 +0000
treeherdermozilla-release@79d24e644fdd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbsmedberg
bugs666414
milestone9.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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);