Bug 1114999 - Part 2: Apply MOZ_NO_ADDREF_RELEASE_ON_RETURN to all smart pointer arrow operators that can return refcounted objects; r=jrmuizel

dom/archivereader/ArchiveZipFile.cpp

@@ -378,9 +378,8 @@ ArchiveZipFileImpl::GetInternalStream(nsIInputStream** aStream)
                                                                mStart,
                                                                mLength,
                                                                mCentral);
-  NS_ADDREF(stream);
 
-  *aStream = stream;
+  stream.forget(aStream);
   return NS_OK;
 }
 

dom/asmjscache/AsmJSCache.cpp

@@ -354,7 +354,7 @@ public:
     }
 
     File*
-    operator->() const
+    operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN
     {
       MOZ_ASSERT(mFile);
       return mFile;

dom/base/nsGlobalWindow.cpp

@@ -12438,7 +12438,7 @@ nsGlobalWindow::RunTimeout(nsTimeout *aTimeout)
   dummy_timeout->mFiringDepth = firingDepth;
   dummy_timeout->mWhen = now;
   last_expired_timeout->setNext(dummy_timeout);
-  dummy_timeout->AddRef();
+  nsRefPtr<nsTimeout> timeoutExtraRef(dummy_timeout);
 
   last_insertion_point = mTimeoutInsertionPoint;
   // If we ever start setting mTimeoutInsertionPoint to a non-dummy timeout,
@@ -12489,6 +12489,7 @@ nsGlobalWindow::RunTimeout(nsTimeout *aTimeout)
       // through a timeout that fired while a modal (to this window)
       // dialog was open or through other non-obvious paths.
       MOZ_ASSERT(dummy_timeout->HasRefCntOne(), "dummy_timeout may leak");
+      unused << timeoutExtraRef.forget().take();
 
       mTimeoutInsertionPoint = last_insertion_point;
 
@@ -12517,7 +12518,7 @@ nsGlobalWindow::RunTimeout(nsTimeout *aTimeout)
 
   // Take the dummy timeout off the head of the list
   dummy_timeout->remove();
-  dummy_timeout->Release();
+  timeoutExtraRef = nullptr;
   MOZ_ASSERT(dummy_timeout->HasRefCntOne(), "dummy_timeout may leak");
 
   mTimeoutInsertionPoint = last_insertion_point;

dom/canvas/CanvasRenderingContext2D.cpp

@@ -578,7 +578,7 @@ public:
     return mTarget;
   }
 
-  DrawTarget* operator->()
+  DrawTarget* operator->() MOZ_NO_ADDREF_RELEASE_ON_RETURN
   {
     return mTarget;
   }

dom/canvas/WebGLObjectModel.h

@@ -219,7 +219,7 @@ public:
         return get();
     }
 
-    T* operator->() const {
+    T* operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN {
         MOZ_ASSERT(mRawPtr != 0, "You can't dereference a nullptr WebGLRefPtr with operator->()!");
         return get();
     }

dom/indexedDB/ActorsParent.cpp

@@ -3667,7 +3667,7 @@ public:
   }
 
   mozIStorageStatement*
-  operator->()
+  operator->() MOZ_NO_ADDREF_RELEASE_ON_RETURN
   {
     MOZ_ASSERT(mStatement);
     return mStatement;
@@ -4077,7 +4077,7 @@ struct FactoryOp::MaybeBlockedDatabaseInfo MOZ_FINAL
   }
 
   Database*
-  operator->()
+  operator->() MOZ_NO_ADDREF_RELEASE_ON_RETURN
   {
     return mDatabase;
   }

dom/media/MediaResource.h

@@ -744,7 +744,7 @@ class MOZ_STACK_CLASS AutoPinned {
   }
 
   operator T*() const { return mResource; }
-  T* operator->() const { return mResource; }
+  T* operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN { return mResource; }
 
 private:
   T* mResource;

dom/media/eme/CDMProxy.h

@@ -280,7 +280,7 @@ private:
       mPtr = nullptr;
     }
 
-    Type* operator->() const {
+    Type* operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN {
       MOZ_ASSERT(NS_IsMainThread());
       return mPtr;
     }

dom/mobilemessage/ipc/SmsIPCService.cpp

@@ -14,6 +14,7 @@
 #include "mozilla/Preferences.h"
 #include "nsString.h"
 #include "mozilla/dom/ipc/BlobChild.h"
+#include "mozilla/unused.h"
 
 using namespace mozilla::dom;
 using namespace mozilla::dom::mobilemessage;
@@ -70,7 +71,8 @@ SendCursorRequest(const IPCMobileMessageCursor& aRequest,
 
   // Add an extra ref for IPDL. Will be released in
   // SmsChild::DeallocPMobileMessageCursor().
-  actor->AddRef();
+  nsRefPtr<MobileMessageCursorChild> actorCopy(actor);
+  mozilla::unused << actorCopy.forget().take();
 
   smsChild->SendPMobileMessageCursorConstructor(actor, aRequest);
 

dom/xul/templates/nsResourceSet.h

@@ -60,7 +60,7 @@ public:
         /*const*/ nsIRDFResource* operator*() const {
             return *mCurrent; }
 
-        /*const*/ nsIRDFResource* operator->() const {
+        /*const*/ nsIRDFResource* operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN {
             return *mCurrent; }
 
         bool operator==(const ConstIterator& aConstIterator) const {

embedding/components/printingui/unixshared/nsPrintingPromptService.cpp

@@ -42,7 +42,7 @@ public:
     nsresult Init() {
       return CallCreateInstance(NS_DIALOGPARAMBLOCK_CONTRACTID, &mBlock);
     }
-    nsIDialogParamBlock * operator->() const { return mBlock; }
+    nsIDialogParamBlock * operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN { return mBlock; }
     operator nsIDialogParamBlock * const ()  { return mBlock; }
 
 private:

embedding/components/printingui/win/nsPrintingPromptService.cpp

@@ -59,7 +59,7 @@ public:
     nsresult Init() {
       return CallCreateInstance(NS_DIALOGPARAMBLOCK_CONTRACTID, &mBlock);
     }
-    nsIDialogParamBlock * operator->() const { return mBlock; }
+    nsIDialogParamBlock * operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN { return mBlock; }
     operator nsIDialogParamBlock * const ()  { return mBlock; }
 
 private:

gfx/layers/ImageContainer.cpp

@@ -282,8 +282,8 @@ ImageContainer::LockCurrentAsSourceSurface(gfx::IntSize *aSize, Image** aCurrent
   ReentrantMonitorAutoEnter mon(mReentrantMonitor);
 
   if (aCurrentImage) {
-    NS_IF_ADDREF(mActiveImage);
-    *aCurrentImage = mActiveImage.get();
+    nsRefPtr<Image> activeImage(mActiveImage);
+    activeImage.forget(aCurrentImage);
   }
 
   if (!mActiveImage) {

gfx/layers/client/TiledContentClient.cpp

@@ -791,7 +791,7 @@ TileClient::GetTileDescriptor()
     // AddRef here and Release when receiving on the host side to make sure the
     // reference count doesn't go to zero before the host receives the message.
     // see TiledLayerBufferComposite::TiledLayerBufferComposite
-    mFrontLock->AddRef();
+    mFrontLock.get()->AddRef();
   }
 
   if (mFrontLock->GetType() == gfxSharedReadLock::TYPE_MEMORY) {

gfx/layers/composite/TiledContentHost.cpp

@@ -91,7 +91,7 @@ TiledLayerBufferComposite::TiledLayerBufferComposite(ISurfaceAllocator* aAllocat
           sharedLock = reinterpret_cast<gfxMemorySharedReadLock*>(ipcLock.get_uintptr_t());
           if (sharedLock) {
             // The corresponding AddRef is in TiledClient::GetTileDescriptor
-            sharedLock->Release();
+            sharedLock.get()->Release();
           }
         }
 

gfx/thebes/gfxQuartzSurface.cpp

@@ -179,7 +179,7 @@ already_AddRefed<gfxImageSurface> gfxQuartzSurface::GetAsImageSurface()
     // shares the refcounts of Cairo surfaces. However, Wrap also adds a
     // reference to the image. We need to remove one of these references
     // explicitly so we don't leak.
-    img->Release();
+    img.get()->Release();
 
     img->SetOpaqueRect(GetOpaqueRect());
 

mfbt/Attributes.h

@@ -524,6 +524,11 @@
  *   should be used for non-owning references that can be unsafe, and their
  *   safety needs to be validated through code inspection.  The string argument
  *   passed to this macro documents the safety conditions.
+ * MOZ_NO_ADDREF_RELEASE_ON_RETURN: Applies to function declarations.  Makes it
+ *   a compile time error to call AddRef or Release on the return value of a
+ *   function.  This is intended to be used with operator->() of our smart
+ *   pointer classes to ensure that the refcount of an object wrapped in a
+ *   smart pointer is not manipulated directly.
  */
 #ifdef MOZ_CLANG_PLUGIN
 #  define MOZ_MUST_OVERRIDE __attribute__((annotate("moz_must_override")))
@@ -542,6 +547,7 @@
 #  define MOZ_OWNING_REF __attribute__((annotate("moz_strong_ref")))
 #  define MOZ_NON_OWNING_REF __attribute__((annotate("moz_weak_ref")))
 #  define MOZ_UNSAFE_REF(reason) __attribute__((annotate("moz_strong_ref")))
+#  define MOZ_NO_ADDREF_RELEASE_ON_RETURN __attribute__((annotate("moz_no_addref_release_on_return")))
 /*
  * It turns out that clang doesn't like void func() __attribute__ {} without a
  * warning, so use pragmas to disable the warning. This code won't work on GCC
@@ -564,6 +570,7 @@
 #  define MOZ_OWNING_REF /* nothing */
 #  define MOZ_NON_OWNING_REF /* nothing */
 #  define MOZ_UNSAFE_REF(reason) /* nothing */
+#  define MOZ_NO_ADDREF_RELEASE_ON_RETURN /* nothing */
 #endif /* MOZ_CLANG_PLUGIN */
 
 /*

mfbt/RefPtr.h

@@ -275,7 +275,7 @@ public:
 
   T* get() const { return mPtr; }
   operator T*() const { return mPtr; }
-  T* operator->() const { return mPtr; }
+  T* operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN { return mPtr; }
   T& operator*() const { return *mPtr; }
   template<typename U>
   operator TemporaryRef<U>() { return TemporaryRef<U>(mPtr); }

mfbt/WeakPtr.h

@@ -195,7 +195,7 @@ public:
   operator T*() const { return mRef->get(); }
   T& operator*() const { return *mRef->get(); }
 
-  T* operator->() const { return mRef->get(); }
+  T* operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN { return mRef->get(); }
 
   T* get() const { return mRef->get(); }
 

netwerk/base/src/AutoClose.h

@@ -51,7 +51,7 @@ public:
     mPtr = nullptr;
   }
 
-  T* operator->() const
+  T* operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN
   {
     return mPtr.operator->();
   }

netwerk/cache/nsDiskCacheDeviceSQL.cpp

@@ -94,7 +94,7 @@ class AutoResetStatement
     explicit AutoResetStatement(mozIStorageStatement *s)
       : mStatement(s) {}
     ~AutoResetStatement() { mStatement->Reset(); }
-    mozIStorageStatement *operator->() { return mStatement; }
+    mozIStorageStatement *operator->() MOZ_NO_ADDREF_RELEASE_ON_RETURN { return mStatement; }
   private:
     mozIStorageStatement *mStatement;
 };

netwerk/sctp/datachannel/DataChannel.cpp

@@ -2105,8 +2105,10 @@ DataChannelConnection::OpenFinish(already_AddRefed<DataChannel>&& aChannel)
     LOG(("Queuing channel %p (%u) to finish open", channel.get(), stream));
     // Also serves to mark we told the app
     channel->mFlags |= DATA_CHANNEL_FLAGS_FINISH_OPEN;
-    channel->AddRef(); // we need a ref for the nsDeQue and one to return
-    mPending.Push(channel);
+    // we need a ref for the nsDeQue and one to return
+    DataChannel* rawChannel = channel;
+    rawChannel->AddRef();
+    mPending.Push(rawChannel);
     return channel.forget();
   }
 

netwerk/sctp/datachannel/DataChannel.h

@@ -264,7 +264,7 @@ private:
   // channels available from the stack must be negotiated!
   bool mAllocateEven;
   nsAutoTArray<nsRefPtr<DataChannel>,16> mStreams;
-  nsDeque mPending; // Holds already_AddRefed<DataChannel>s -- careful!
+  nsDeque mPending; // Holds addref'ed DataChannel's -- careful!
   // holds data that's come in before a channel is open
   nsTArray<nsAutoPtr<QueuedDataMessage> > mQueuedData;
 

parser/html/nsHtml5RefPtr.h

@@ -204,7 +204,7 @@ class nsHtml5RefPtr
         }
 
       T*
-      operator->() const
+      operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN
         {
           NS_PRECONDITION(mRawPtr != 0, "You can't dereference a NULL nsHtml5RefPtr with operator->().");
           return get();

widget/gtk/nsClipboard.cpp

@@ -979,7 +979,7 @@ wait_for_contents(GtkClipboard *clipboard, GdkAtom target)
 {
     RefPtr<RetrievalContext> context = new RetrievalContext();
     // Balanced by Release in clipboard_contents_received
-    context->AddRef();
+    context.get()->AddRef();
     gtk_clipboard_request_contents(clipboard, target,
                                    clipboard_contents_received,
                                    context.get());
@@ -1001,7 +1001,7 @@ wait_for_text(GtkClipboard *clipboard)
 {
     RefPtr<RetrievalContext> context = new RetrievalContext();
     // Balanced by Release in clipboard_text_received
-    context->AddRef();
+    context.get()->AddRef();
     gtk_clipboard_request_text(clipboard, clipboard_text_received, context.get());
     return static_cast<gchar*>(context->Wait());
 }

xpcom/base/nsRefPtr.h

@@ -7,6 +7,7 @@
 #ifndef nsRefPtr_h
 #define nsRefPtr_h
 
+#include "mozilla/Attributes.h"
 #include "AlreadyAddRefed.h"
 #include "nsDebug.h"
 #include "nsISupportsUtils.h"
@@ -222,7 +223,7 @@ public:
   }
 
   T*
-  operator->() const
+  operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN
   {
     NS_PRECONDITION(mRawPtr != 0,
                     "You can't dereference a NULL nsRefPtr with operator->().");

xpcom/glue/nsCOMPtr.h

@@ -693,7 +693,7 @@ public:
   // necessary to resolve ambiguity.
   operator T*() const { return get(); }
 
-  T* operator->() const
+  T* operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN
   {
     NS_ABORT_IF_FALSE(mRawPtr != 0,
                       "You can't dereference a NULL nsCOMPtr with operator->().");
@@ -972,7 +972,7 @@ public:
   // necessary to resolve ambiguity/
   operator nsISupports* () const { return get(); }
 
-  nsISupports* operator->() const
+  nsISupports* operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN
   {
     NS_ABORT_IF_FALSE(mRawPtr != 0,
                       "You can't dereference a NULL nsCOMPtr with operator->().");

xpcom/glue/nsProxyRelease.h

@@ -219,7 +219,7 @@ public:
   }
 
   operator T*() { return get(); }
-  T* operator->() { return get(); }
+  T* operator->() MOZ_NO_ADDREF_RELEASE_ON_RETURN { return get(); }
 
   // These are safe to call on other threads with appropriate external locking.
   bool operator==(const nsMainThreadPtrHandle<T>& aOther) const

xpcom/glue/nsTWeakRef.h

@@ -157,7 +157,7 @@ public:
    * Allow |*this| to be treated as a |Type*| for convenience.  Use with
    * caution since this method will crash if the referenced object is null.
    */
-  Type* operator->() const
+  Type* operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN
   {
     NS_ASSERTION(mRef && mRef->mObj,
                  "You can't dereference a null weak reference with operator->().");

xpcom/tests/TestTimers.cpp

@@ -42,7 +42,7 @@ public:
     return mThread;
   }
 
-  nsIThread* operator->() const {
+  nsIThread* operator->() const MOZ_NO_ADDREF_RELEASE_ON_RETURN {
     return mThread;
   }