Closed Bug 1164581 Opened 5 years ago Closed 4 years ago

NS_ProxyRelease should accept already_AddRefed

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: bholley, Assigned: aidin, Mentored)

References

Details

Attachments

(1 file, 6 obsolete files)

Right now it accepts nsISupports*, nsCOMPtr<T>&, and nsRefPtr<T>&. This is a huge footgun. If, say, somebody uses one of our other smart pointer classes (like RefPtr), they'll end up with the nsISupports* overload, and the pointer will be freed twice.

Instead, we should have exactly declaration, which takes already_AddRefed<T>, and fix up callers to either .forget() their smartpointer or create an already_AddRefed out of a raw pointer via dont_AddRef.
Bug 1164581 - Adding an overload for NS_ProxyRelease that accepts already_AddRefed r?bent.mozilla
Here's the Try Server build.
Seems that those three failures are not related to my changes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e8cb8fba5b3
Assignee: nobody → aidin
Attachment #8654530 - Flags: review?(bent.mozilla)
Comment on attachment 8654530 [details]
MozReview Request: Bug 1164581 - Adding an overload for NS_ProxyRelease that accepts already_AddRefed, and removing all the others. r?bobbyholley

-> Bobby!
Attachment #8654530 - Flags: review?(bent.mozilla) → review?(bobbyholley)
Comment on attachment 8654530 [details]
MozReview Request: Bug 1164581 - Adding an overload for NS_ProxyRelease that accepts already_AddRefed, and removing all the others. r?bobbyholley

https://reviewboard.mozilla.org/r/17715/#review17147

::: xpcom/glue/nsProxyRelease.h:105
(Diff revision 1)
> +NS_ReleaseOnMainThread(already_AddRefed<T>& aDoomed,
> +                       bool aAlwaysProxy = false)

This should be already_AddRefed<T>, not already_AddRefed<T>&

Hey Aidin,

Sorry for the terribly long wait - I was on vacation. :-(

So, in order to make this patch do what we want, we need to _also_ remove the other NS_ReleaseOnMainThread overloads, and fix all the callers to pass someSmartPointer.forget(). The nice thing about removing the existing overloads is that it won't compile until we've fixed everything. :P
Attachment #8654530 - Flags: review?(bobbyholley)
Comment on attachment 8654530 [details]
MozReview Request: Bug 1164581 - Adding an overload for NS_ProxyRelease that accepts already_AddRefed, and removing all the others. r?bobbyholley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/17715/diff/1-2/
Attachment #8654530 - Attachment description: MozReview Request: Bug 1164581 - Adding an overload for NS_ProxyRelease that accepts already_AddRefed r?bent.mozilla → MozReview Request: Bug 1164581 - Adding an overload for NS_ProxyRelease that accepts already_AddRefed, and removing all the others. r?bobbyholley
Attachment #8654530 - Flags: review?(bobbyholley)
Comment on attachment 8654530 [details]
MozReview Request: Bug 1164581 - Adding an overload for NS_ProxyRelease that accepts already_AddRefed, and removing all the others. r?bobbyholley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/17715/diff/2-3/
Sorry for the long delay. I wasn't working on Mozilla during these three months.

I sent another patch. Hope it will be OK.
Here's the try-server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=48bb77f4444d

Also, please check this comment: https://dxr.mozilla.org/mozilla-central/source/xpcom/base/JSObjectHolder.h#24
I'm not sure if I should do anything to this class.

Thanks (:
Comment on attachment 8654530 [details]
MozReview Request: Bug 1164581 - Adding an overload for NS_ProxyRelease that accepts already_AddRefed, and removing all the others. r?bobbyholley

https://reviewboard.mozilla.org/r/17715/#review25207

This looks great!

However, IIUC, this only fixes NS_ReleaseOnMainThread, and we need an additional patch to fix NS_ProxyRelease, right?

This can certainly land on its own (and that's probably a good idea!), but if so, it should land as a dependent bug against bug 1164581.

Thanks for the awesome work!
Attachment #8654530 - Flags: review?(bobbyholley)
Attachment #8654530 - Flags: review?(bobbyholley)
Comment on attachment 8654530 [details]
MozReview Request: Bug 1164581 - Adding an overload for NS_ProxyRelease that accepts already_AddRefed, and removing all the others. r?bobbyholley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/17715/diff/3-4/
Thanks for all the help. I finally fixed it.

 Here's the try-server build, with a green V:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0a3d3a36c8f

Should I check for main thread existence on the NS_ReleaseOnMainThread and log an NS_WARNING? Some of the codes I refactored were did so, and wrote a warning like "released after thread shutdown, leaking".
https://reviewboard.mozilla.org/r/17713/#review27527

::: netwerk/base/TLSServerSocket.cpp:332
(Diff revision 4)
> -    MutexAutoLock lock(mLock);
> +  MutexAutoLock lock(mLock);

I'm not sure if it's correct to use Lock like this.
Comment on attachment 8654530 [details]
MozReview Request: Bug 1164581 - Adding an overload for NS_ProxyRelease that accepts already_AddRefed, and removing all the others. r?bobbyholley

https://reviewboard.mozilla.org/r/17715/#review27589

I haven't looked carefully at all the consumers yet, but they generally seem ok. I'll have a closer look once we get the nsProxyRelease.h stuff fully sorted out.

Thanks for working on this!

::: xpcom/glue/nsProxyRelease.h:57
(Diff revision 4)
> -  T* raw = nullptr;
> -  aDoomed.swap(raw);
> -  return NS_ReleaseOnMainThread(raw, aAlwaysProxy);
> +  nsresult rv;
> +  T* doomedPtr = aDoomed.take();
> +

We should scope the manual reference counting as tightly as possible, so I think we should avoid the .take() call at the top, and instead Move() the already_AddRefed to the nsProxyReleaseEvent constructor, which can perform the .take() call.

This will have the nice side-effect of cleaning up the logic in this function a bit.

::: xpcom/glue/nsProxyRelease.h:80
(Diff revision 4)
> +  if (!ev) {
> +    // we do not release aDoomed here since it may cause a delete on the
> +    // wrong thread.  better to leak than crash.
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }

you Can actually remove this null check, because |new| is infallible in Gecko now.

::: xpcom/glue/nsProxyRelease.h:119
(Diff revision 4)
> -  return NS_ProxyRelease(mainThread, aDoomed, aAlwaysProxy);
> +  return NS_ProxyRelease(mainThread, already_AddRefed<T>(aDoomed.take()), aAlwaysProxy);

In general, you should use dont_AddRef instead of already_AddRefed<T>.

But in this case, you can just do Move(aDoomed).

::: xpcom/glue/nsProxyRelease.h:120
(Diff revision 4)
> +  //return NS_InternalProxyRelease(mainThread, aDoomed.take(), aAlwaysProxy);

This should be removed, I'd think.

::: xpcom/glue/nsProxyRelease.h:187
(Diff revision 4)
> -      nsCOMPtr<nsIThread> mainThread;
> +      NS_ReleaseOnMainThread(already_AddRefed<T>(mRawPtr));

Same here.
Attachment #8654530 - Flags: review?(bobbyholley)
(In reply to Aidin Gharibnavaz from comment #12)
> I'm not sure if it's correct to use Lock like this.

To be safe here, I think you should extract the nsRefPtr into a local variable (also an nsRefPtr) within the scope of the lock, and then pass that to NS_ProxyRelease. That way, there's no worry that NS_ProxyRelease might deadlock (even though I really doubt it would).
(In reply to Aidin Gharibnavaz from comment #11)
> Thanks for all the help. I finally fixed it.
> 
>  Here's the try-server build, with a green V:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0a3d3a36c8f
> 
> Should I check for main thread existence on the NS_ReleaseOnMainThread and
> log an NS_WARNING? Some of the codes I refactored were did so, and wrote a
> warning like "released after thread shutdown, leaking".

Might as well do that, sure.
Attached patch 1164581.patch (obsolete) — Splinter Review
Really sorry for the long delay.

I fixed the things you mentioned. Again, there were some memory leaks which I resolved, but I couldn't resolve this one:
https://treeherder.mozilla.org/logviewer.html#?job_id=16048959&repo=try

Can you take a look please?

P.S: Seems that MozReview permissions restricted, and I can't push anything to it, temporary.
Attachment #8654530 - Attachment is obsolete: true
Attachment #8714876 - Flags: review?(bobbyholley)
Comment on attachment 8714876 [details] [diff] [review]
1164581.patch

Review of attachment 8714876 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there. As before, I didn't review all the callers, because I want to get the core stuff sorted out first. Please do it soon, because it takes a lot of time for me to remember all this stuff. :-)

::: xpcom/base/nsInterfaceRequestorAgg.cpp
@@ +63,3 @@
>    }
> +
> +  mSecond.swap(doomedPtr);

This is the source of your failures on try. The problem here is that you didn't null out doomedPtr, so you're now swapping that into mSecond.

Instead of all this junk, just do:

NS_ProxyRelease(mConsumerTarget, mFirst.forget());
NS_ProxyRelease(mConsumerTarget, mSecond.forget());

::: xpcom/glue/nsProxyRelease.h
@@ +26,3 @@
>  {
> +public:
> +  explicit nsProxyReleaseEvent(already_AddRefed<T>&& aDoomed)

You shouldn't need the && here - already_AddRefed has a Move constructor that will be invoked implicitly.

@@ +85,5 @@
> +    NS_WARNING("failed to post proxy release event");
> +    // It is better to leak the aDoomed object than risk crashing as
> +    // a result of deleting it on the wrong thread.
> +  }
> +  return rv;

Sorry, I may have misguided you here last time. Here's how I think this function should look:

{
  nsRefPtr<T> doomed = aDoomed;
  if (!aTarget) {
    return NS_OK;
  }

  if (!aAlwaysProxy) {
  ....
     return NS_OK;
  }

  nsCOMPtr<nsIRunnable> ev = new nsProxyReleaseEvent(aDoomed.forget());

  ....
}

This lets us avoid the manual refcounting.

@@ +100,5 @@
>   *        parameter is true, then an event will always be posted to the main
>   *        thread for asynchronous release.
>   */
> +template<class T>
> +inline NS_HIDDEN_(nsresult)

Do you need the NS_HIDDEN_ here? I'd be surprised.

@@ +185,5 @@
>    {
>      if (NS_IsMainThread()) {
>        NS_IF_RELEASE(mRawPtr);
>      } else if (mRawPtr) {
> +      NS_ReleaseOnMainThread(dont_AddRef<T>(mRawPtr));

I'm pretty sure you don't need the <T>.
Attachment #8714876 - Flags: review?(bobbyholley) → review-
Attached patch 1164581.patch (obsolete) — Splinter Review
Thanks for the review (:

I fixed the things you mentioned. I hope this time the nsProxyRelease will be OK!

> Do you need the NS_HIDDEN_ here? I'd be surprised.

It was there before (: But I think it's required. `inline' keyword only *suggest* that compiler should make this code inline, but it won't happen necessarily. So, functions in this header may become exported symbols in other libraries, increasing the number of exported symbols, which itself causes longer load times.
Attachment #8714876 - Attachment is obsolete: true
Attachment #8716371 - Flags: feedback?(bobbyholley)
Comment on attachment 8716371 [details] [diff] [review]
1164581.patch

Review of attachment 8716371 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there! Please post the new patch quickly so we can get this landed. Thanks for working on this. :-)

::: dom/base/WebSocket.cpp
@@ +632,5 @@
>    // until the end of the method.
>    RefPtr<WebSocketImpl> kungfuDeathGrip = this;
>  
> +  NS_ReleaseOnMainThread(mChannel.forget());
> +  NS_ReleaseOnMainThread(dont_AddRef(static_cast<nsIWebSocketEventService*>(mService.forget().take())));

I think this can just be mService.forget(). The casting was just to make it an nsISupports, which was needed by the old signature but not the new one.

::: dom/workers/WorkerPrivate.cpp
@@ +3617,5 @@
>        NS_WARNING("Failed to proxy release of listeners, leaking instead!");
>      }
>  
>      for (size_t index = 0; index < mListeners.Length(); ++index) {
> +      if (NS_FAILED(NS_ProxyRelease(mainThread, mListeners[index].forget()))) {

NS_ProxyRelease and NS_ReleaseOnMainThread already warn when they fail (which is basically all we can do in those circumstances). Given that, I think we should actually make NS_ProxyRelease and NS_ReleaseOnMainThread return void, and eliminate any duplicate warnings generated by callers.

::: extensions/gio/nsGIOProtocolHandler.cpp
@@ +616,5 @@
>    }
>  
>    if (mChannel)
>    {
> +    nsresult rv = NS_ReleaseOnMainThread(already_AddRefed<nsIChannel>(mChannel));

This should be dont_AddRef.

::: netwerk/base/TLSServerSocket.cpp
@@ +336,5 @@
>    }
>  
> +  if (observer)
> +  {
> +    NS_ReleaseOnMainThread(dont_AddRef<nsITLSServerSecurityObserver>(observer));

I think you don't need the <> to dont_AddRef.

::: netwerk/base/nsServerSocket.cpp
@@ +243,5 @@
>      // around bug 337492.
>      if (listener)
> +    {
> +      NS_ProxyRelease(mListenerTarget,
> +                      dont_AddRef<nsIServerSocketListener>(listener));

Same here about the <>

::: netwerk/base/nsStreamListenerTee.cpp
@@ +45,2 @@
>          mSink.swap(sink);
> +        NS_RELEASE(sink);

Even though we do more than warning in this case, I think it would be better to leak.

::: netwerk/base/nsTransportUtils.cpp
@@ +36,5 @@
>      virtual ~nsTransportEventSinkProxy()
>      {
>          // our reference to mSink could be the last, so be sure to release
>          // it on the target thread.  otherwise, we could get into trouble.
> +        NS_ProxyRelease(mTarget, already_AddRefed<nsITransportEventSink>(mSink));

This needs to be dont_AddRef.

::: netwerk/base/nsUDPSocket.cpp
@@ +522,5 @@
>  
>    if (mListener)
>    {
>      // need to atomically clear mListener.  see our Close() method.
> +    nsIUDPSocketListener* listener = nullptr;

Why get rid of the smart pointer here? Please keep using it, and then pass .forget() below.

::: netwerk/cache/nsCacheService.cpp
@@ +2690,5 @@
>      bool isCur;
>      if (!target || (NS_SUCCEEDED(target->IsOnCurrentThread(&isCur)) && isCur)) {
>          gService->mDoomedObjects.AppendElement(obj);
>      } else {
> +        NS_ProxyRelease(target, already_AddRefed<nsISupports>(obj));

dont_AddRef.

::: netwerk/ipc/RemoteOpenFileChild.cpp
@@ +122,5 @@
>            NS_NewNonOwningRunnableMethod(tabChild, &TabChild::Release);
>          MOZ_ASSERT(runnable);
>  
>          MOZ_ALWAYS_TRUE(NS_SUCCEEDED(mainThread->Dispatch(runnable,
>                                                            NS_DISPATCH_NORMAL)));

Why can't we also just NS_ReleaseOnMainThread for TabChild?

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +1222,5 @@
>    mListenerMT = nullptr;
>  
> +  NS_ReleaseOnMainThread(mLoadGroup.forget());
> +  NS_ReleaseOnMainThread(mLoadInfo.forget());
> +  NS_ReleaseOnMainThread(dont_AddRef(static_cast<nsIWebSocketEventService*>(mService.forget().take())));

Just pass mService.forget().

::: storage/StorageBaseStatementInternal.cpp
@@ +49,5 @@
>        mStatement->mAsyncStatement = nullptr;
>      }
> +
> +    nsCOMPtr<nsIThread> targetThread(mConnection->threadOpenedOn);
> +    (void)::NS_ProxyRelease(targetThread, mStatement.forget());

Please remove the (void) and the ::, here and below.

::: storage/mozStorageAsyncStatement.cpp
@@ +221,5 @@
>      // NS_ProxyRelase only magic forgets for us if mDBConnection is an
>      // nsCOMPtr.  Which it is not; it's an nsRefPtr.
> +    nsCOMPtr<nsIThread> targetThread(mDBConnection->threadOpenedOn);
> +    (void)::NS_ProxyRelease(targetThread,
> +                            mDBConnection.forget());

And here.

::: storage/mozStorageConnection.cpp
@@ +385,5 @@
>      nsCOMPtr<nsIThread> thread;
>      (void)NS_GetMainThread(getter_AddRefs(thread));
>      // Handle ambiguous nsISupports inheritance.
> +    NS_ProxyRelease(thread, mConnection.forget());
> +    (void)NS_ReleaseOnMainThread(mCallbackEvent.forget());

And here.

::: storage/mozStorageService.cpp
@@ +732,5 @@
>  
>    ~AsyncInitDatabase()
>    {
> +    (void)NS_ReleaseOnMainThread(mStorageFile.forget());
> +    (void)NS_ReleaseOnMainThread(mConnection.forget());

Remove the (void)

@@ +737,5 @@
>  
>      // Generally, the callback will be released by CallbackComplete.
>      // However, if for some reason Run() is not executed, we still
>      // need to ensure that it is released here.
> +    (void)NS_ReleaseOnMainThread(mCallback.forget());

And here.

::: toolkit/components/places/Helpers.h
@@ +195,5 @@
>    NS_IMETHOD Run()
>    {
>      mStatementCache.FinalizeStatements();
>      // Release the owner back on the calling thread.
> +    (void)NS_ProxyRelease(mCallingThread, mOwner.forget());

And here

::: xpcom/glue/nsProxyRelease.h
@@ +59,5 @@
> +  nsresult rv;
> +  RefPtr<T> doomed = aDoomed;
> +
> +  if (!doomed)
> +  {

Nit: { on the same line. Please fix it elsewhere in your changes as well.

@@ +67,5 @@
> +  if (!aTarget)
> +  {
> +    T* doomedPtr = doomed.forget().take();
> +    NS_RELEASE(doomedPtr);
> +    return NS_OK;

You don't need to do this. Just return NS_OK and the owning |doomed| RefPtr will automatically trigger a release. See comment 17.
Attachment #8716371 - Flags: feedback?(bobbyholley) → feedback-
Attached patch 1164581.patch (obsolete) — Splinter Review
Thanks for the painstaking review. Sorry that I can't submit patches quickly, I don't have enough free time.

> Why get rid of the smart pointer here? Please keep using it, and then pass .forget() below.

Is it correct now? I also fixed the same issue in "TLSServerSocket.cpp" and "nsServerSocket.cpp".

> Why can't we also just NS_ReleaseOnMainThread for TabChild?

Because I'm not sure what these lines are doing! I changed it to use NS_ReleaseOnMainThread, and it seems OK.

I test the patch with valgrind, and finally I got a green V!
When all problems resolved and review granted, I will test the patch on try server and will send the result here.
Thanks (:
Attachment #8716371 - Attachment is obsolete: true
Attachment #8716775 - Flags: review?(bobbyholley)
Comment on attachment 8716775 [details] [diff] [review]
1164581.patch

Review of attachment 8716775 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! This is awesome cleanup that will prevent serious bugs, and I really appreciate you seeing it through to completion. :-)

r=bholley
Attachment #8716775 - Flags: review?(bobbyholley) → review+
Keywords: checkin-needed
Looks like you accidentally removed the call to:

listener->OnStopListening(this, mCondition);

In nsUDPSocket.cpp. Sorry I missed that during review. :-(
Attached patch 1164581.patch (obsolete) — Splinter Review
I'm the one who should be sorry (:

Here's a try server build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1932c7361fd8

I ran some unit-tests too, and evrything seems OK.

P.S: At the time I'm posting this, 11 jobs are still in progress. But I'm going to sleep now. They should be fine :P
Attachment #8716775 - Attachment is obsolete: true
Flags: needinfo?(aidin)
Attachment #8717547 - Flags: review?(bobbyholley)
Comment on attachment 8717547 [details] [diff] [review]
1164581.patch

Review of attachment 8717547 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsUDPSocket.cpp
@@ +530,3 @@
>      }
>  
> +    listener->OnStopListening(this, mCondition);

You still need to null-check this like we did before, right?
Attachment #8717547 - Flags: review?(bobbyholley) → review-
Attached patch 1164581.patch (obsolete) — Splinter Review
Oh! Really sorry for my carelessness.
Attachment #8717547 - Attachment is obsolete: true
Attachment #8717761 - Flags: review?(bobbyholley)
Attachment #8717761 - Flags: review?(bobbyholley) → review+
(In reply to Aidin Gharibnavaz from comment #28)
> Created attachment 8717761 [details] [diff] [review]
> 1164581.patch
> 
> Oh! Really sorry for my carelessness.

No worries - happens to the best of us!
Keywords: checkin-needed
Hi Aidin, 
seems this patch has conflicts:

adding 1164581 to series file
renamed 1164581 -> 1164581.patch
applying 1164581.patch
patching file dom/base/Console.cpp
Hunk #1 FAILED at 715
1 out of 1 hunks FAILED -- saving rejects to file dom/base/Console.cpp.rej

can you take a look ? Thanks!
Flags: needinfo?(aidin)
Attached patch 1164581.patchSplinter Review
Hi Carsten (:

I fixed the conflict, and it builds fine in my machine.
Attachment #8717761 - Attachment is obsolete: true
Flags: needinfo?(aidin) → needinfo?(cbook)
Depends on: 1247570
https://hg.mozilla.org/mozilla-central/rev/c75f253f4335
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Flags: needinfo?(cbook)
Duplicate of this bug: 1210749
Depends on: 1247972
Blocks: 1248023
You need to log in before you can comment on or make changes to this bug.