Closed Bug 1186745 Opened 5 years ago Closed 5 years ago

Add NS_DispatchToCurrentThread(already_AddRefed<nsIRunnable>&&) overload

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 --- affected
firefox44 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(8 files, 2 obsolete files)

3.23 KB, patch
xidorn
: feedback+
Details | Diff | Splinter Review
6.31 KB, patch
froydnj
: feedback+
xidorn
: feedback+
Details | Diff | Splinter Review
40 bytes, text/x-review-board-request
froydnj
: review+
Details
40 bytes, text/x-review-board-request
froydnj
: review+
Details
40 bytes, text/x-review-board-request
froydnj
: review+
Details
40 bytes, text/x-review-board-request
froydnj
: review+
Details
40 bytes, text/x-review-board-request
froydnj
: review+
Details
40 bytes, text/x-review-board-request
froydnj
: review+
Details
This should land first.
Attached patch patch (obsolete) — Splinter Review
This is the patch from bug 1184729 comment 5.
Attachment #8637614 - Flags: review?(nfroyd)
Blocks: 1186750
I've been running try's and the previous patch was causing leaks; I realized that the current DispatchToCurrentThread() frees on failure, unlike DispatchToMainThread, and lots of stuff is depending on it.  Also, unlike the others, freeing immediately won't free the objects on the wrong thread.  However, I can imagine cases where freeing immediately might lead to a deadlock (and maybe even worse in some unlikely cases), so it would be best to try to avoid having to rely on this behavior, or find some way to ensure these are processed even in shutdown.  Perhaps put the event in a special in-shutdown queue in the thread that when we return to the event loop from the event where it was queued, we run any self-queued events before exiting for good.  Basically make DispatchToCurrentThread always succeed (and process them) even in shutdown, by whatever means.
Attachment #8637649 - Flags: review?(quanxunzhen)
Attachment #8637649 - Flags: review?(nfroyd)
Attachment #8637614 - Attachment is obsolete: true
Attachment #8637614 - Flags: review?(nfroyd)
Comment on attachment 8637649 [details] [diff] [review]
Allow DispatchToCurrentThread() to take already_AddRefed<nsIRunnable>&&'s

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

::: xpcom/glue/nsThreadUtils.cpp
@@ +162,5 @@
> +  // To keep us from leaking the runnable if the dispatch method fails, we
> +  // grab the ref on failure and release it
> +  rv = thread->Dispatch(mozilla::Move(aEvent), NS_DISPATCH_NORMAL);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    nsCOMPtr<nsIRunnable> temp(aEvent);

I don't think you can do this. After ::Move() call, the original variable is assumed to contain nothing. We cannot should never use a moved variable.

I just submited bug 1186706 request for static analysis to avoid this kind of mistakes.
Attachment #8637649 - Flags: review?(quanxunzhen) → review-
s/cannot should never/should never/
Ugly... but given all the requirements and pre-existing uses, some of these have to end up ugly, and it's nice if that's isolated
Attachment #8637700 - Flags: review?(quanxunzhen)
Attachment #8637700 - Flags: review?(nfroyd)
Attachment #8637649 - Attachment is obsolete: true
Attachment #8637649 - Flags: review?(nfroyd)
Comment on attachment 8637700 [details] [diff] [review]
Allow DispatchToCurrentThread() to take already_AddRefed<nsIRunnable>&&'s

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

::: xpcom/glue/nsThreadUtils.cpp
@@ +166,5 @@
> +    // Dispatch leaked the reference to the event, but due to caller's
> +    // assumptions we shouldn't leak here, and in this case it's mostly
> +    // safe.
> +    temp->Release();
> +  }

So it is not safe for nsThread::Dispatch to release an event in general, but for the specific case of DispatchToCurrentThread, it is safe to release. OK.

But it seems to me there are several branches in nsThread::DispatchInternal do release the event and return a failed result. The only place the event leaks is nsThread::PutEvent.

We probably should make nsThread::Dispatch leaks in all branches if it fails...
Attachment #8637700 - Flags: review?(quanxunzhen) → feedback+
> ::: xpcom/glue/nsThreadUtils.cpp
> @@ +166,5 @@
> > +    // Dispatch leaked the reference to the event, but due to caller's
> > +    // assumptions we shouldn't leak here, and in this case it's mostly
> > +    // safe.
> > +    temp->Release();
> > +  }
> 
> So it is not safe for nsThread::Dispatch to release an event in general, but
> for the specific case of DispatchToCurrentThread, it is safe to release. OK.
> 
> But it seems to me there are several branches in nsThread::DispatchInternal
> do release the event and return a failed result. The only place the event
> leaks is nsThread::PutEvent.

For DISPATCH_NORMAL, we release the object only in the case of:
  if (gXPCOMThreadsShutDown && MAIN_THREAD != mIsMainThread && !aTarget) {
    NS_ASSERTION(false, "Failed Dispatch after xpcom-shutdown-threads");
    return NS_ERROR_ILLEGAL_DURING_SHUTDOWN;
  }

For DISPATCH_SYNC, it will release on failure of GetCurrentThread() (and warn, perhaps it should NS_ASSERTION).  It will also release (via releasing the Wrapper) on PutEvent failure.  Note that while the wrapper is always released on the calling thread, the event is normally released on the target thread, so the event should be leaked in this case.

> We probably should make nsThread::Dispatch leaks in all branches if it
> fails...

Patch pending....

The alternative would be the drop 10 yards and re-run the play:  convert Dispatch()/etc to take an nsCOMPtr<nsIRunnable>& and have it take the value on success (clearing the parent's nsCOMPtr).  The downside would be having to make *every* point that needs thread-specific release behavior add leak code on failure (and ripple the behavior/failure modes through overlaying APIs).  This would be a Pain.

Another alternative would be to hold all such thread-sensitive values in nsRefPtrThreadSpecific which on wrong-thread release leaks.  This would also be a big, painful change (likely even bigger, though arguably safer when done).

Thus, I think the modification to Dispatch I'm doing is best.
Attachment #8637947 - Flags: feedback?(quanxunzhen)
Attachment #8637947 - Flags: feedback?(nfroyd)
Comment on attachment 8637947 [details] [diff] [review]
Make leak behavior of Dispatch consistent (leak always on failure)

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

Overall looks good to me.

::: xpcom/threads/nsThread.cpp
@@ +551,5 @@
>    if (gXPCOMThreadsShutDown && MAIN_THREAD != mIsMainThread && !aTarget) {
>      NS_ASSERTION(false, "Failed Dispatch after xpcom-shutdown-threads");
> +    // Explicitly leak the event since we can't release it on the target thread
> +    nsIRunnable* temp = event.forget().take(); // can't use unused << aEvent here due to Windows (boo)
> +    return temp ? NS_ERROR_ILLEGAL_DURING_SHUTDOWN : NS_ERROR_ILLEGAL_DURING_SHUTDOWN;

Instead of doing so, I guess it would be easier to do this at the start of the function
> nsIRunnable* event = aEvent.take();
and when we need it, reconstruct this like
> return PutEvent(already_AddRefed<nsIRunnable>(event), aTarget);
Then just return the result when we want to leak. So that even if we somehow add new failing branch, we would not incorrectly release the event there. That could also help us avoid hack like this.

(We probably can do the same thing in PutEvent)

::: xpcom/threads/nsThread.h
@@ +34,5 @@
>    nsresult Dispatch(nsIRunnable* aEvent, uint32_t aFlags) {
>      return Dispatch(nsCOMPtr<nsIRunnable>(aEvent).forget(), aFlags);
>    }
> +  // NOTE: Dispatch() will *leak* the event on failure, since if it drops
> +  // the ref the event may be released on the wrong thread.

Probably better be placed before the method instead of after.
Attachment #8637947 - Flags: feedback?(quanxunzhen) → feedback+
Comment on attachment 8637700 [details] [diff] [review]
Allow DispatchToCurrentThread() to take already_AddRefed<nsIRunnable>&&'s

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

This seems like a lot of hoop-jumping to fix a non-problem; we don't have the same "which thread should this release on?" problem with dispatching to the current thread as we do to the main thread...do we?

Is this done for symmetry, or is there a deeper reason that I'm missing?
Attachment #8637700 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #10)
> Comment on attachment 8637700 [details] [diff] [review]
> Allow DispatchToCurrentThread() to take already_AddRefed<nsIRunnable>&&'s
> 
> Review of attachment 8637700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems like a lot of hoop-jumping to fix a non-problem; we don't have
> the same "which thread should this release on?" problem with dispatching to
> the current thread as we do to the main thread...do we?
> 
> Is this done for symmetry, or is there a deeper reason that I'm missing?

I think it is mostly for consistency. It could also reduce one unnecessary refcount change in many cases. But not something as critical as NS_DispatchToMainThread or nsThread::Dispatch.

However, the second patch makes more sense, as we currently may sometimes release runnable in a wrong thread if Dispatch fails in some cases. The "always leak when fail" behavior would also affect the code in NS_DispatchToCurrentThread, because we don't want to leak in this particular case.
Comment on attachment 8637947 [details] [diff] [review]
Make leak behavior of Dispatch consistent (leak always on failure)

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

::: xpcom/threads/nsThread.cpp
@@ +550,5 @@
>  
>    if (gXPCOMThreadsShutDown && MAIN_THREAD != mIsMainThread && !aTarget) {
>      NS_ASSERTION(false, "Failed Dispatch after xpcom-shutdown-threads");
> +    // Explicitly leak the event since we can't release it on the target thread
> +    nsIRunnable* temp = event.forget().take(); // can't use unused << aEvent here due to Windows (boo)

This comment is because the linkage of |unused| is all wrong in some places?

@@ +551,5 @@
>    if (gXPCOMThreadsShutDown && MAIN_THREAD != mIsMainThread && !aTarget) {
>      NS_ASSERTION(false, "Failed Dispatch after xpcom-shutdown-threads");
> +    // Explicitly leak the event since we can't release it on the target thread
> +    nsIRunnable* temp = event.forget().take(); // can't use unused << aEvent here due to Windows (boo)
> +    return temp ? NS_ERROR_ILLEGAL_DURING_SHUTDOWN : NS_ERROR_ILLEGAL_DURING_SHUTDOWN;

Possibly worth a comment saying that we're using it to avoid unused variable warnings?
Attachment #8637947 - Flags: feedback?(nfroyd) → feedback+
Bug 1186745 part 1 - Make nsThread::Dispatch() always leak the event if it fails to dispatch it.
Attachment #8659627 - Flags: review?(nfroyd)
Bug 1186745 part 2 - Fix event leak when using NS_DispatchToCurrentThread.
Attachment #8659628 - Flags: review?(nfroyd)
Comment on attachment 8659628 [details]
MozReview Request: Bug 1186745 part 6 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj

https://reviewboard.mozilla.org/r/18991/#review17055

::: xpcom/glue/nsThreadUtils.cpp:159
(Diff revision 1)
> -  return thread->Dispatch(aEvent, NS_DISPATCH_NORMAL);
> +  // To keep us from leaking the runnable if dispatch method fails,
> +  // we grab the reference on failures and release it.
> +  nsIRunnable* temp = event.get();
> +  rv = thread->Dispatch(event.forget(), NS_DISPATCH_NORMAL);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    // Dispatch() leaked the reference to the event, but due to caller's
> +    // assumptions, we shouldn't leak here. And given we are on the same
> +    // thread as the dispatch target, it's mostly safe to do it here.
> +    NS_RELEASE(temp);
> +  }
> +  return rv;

This is better, thank you for fixing this.  The comments are very helpful.

But I think we can do better for the main thread case.  I'm not keen on adding the overhead that this patch requires for NS_DispatchToMainThread.  How about something like:

```
static nsresult
DispatchToThreadCarefully(nsIThread* aThread,  already_AddRefed<nsIRunnable>&& aEvent, uint32_t flags)
{
  nsCOMPtr<nsIRunnable> event(aEvent);
  // This is gross, but we need to have a way to release the
  // runnable on failure, and we don't want to add any
  // additional references.
  nsIRunnable* rawEvent = event.get();
  rv = thread->Dispatch(event.forget(), flags);

  if (NS_WARN_IF(NS_FAILED(rv)) {
    // Dispatch() leaked the reference to the event, but due
    // to caller's assumptions, we shouldn't leak here if we
    // were dispatching to the current thread.
    bool onCurrentThread = false;
    thread->IsOnCurrentThread(&bool);
    if (onCurrentThread) {
      NS_RELEASE(rawEvent);
    }
  }
  
  return rv;
}
```

This function can then be called from the NS_DispatchTo functions, once they've acquired the appropriate thread.  That way, the overhead is only added to the error path.  It is more expensive for NS_DispatchToCurrentThread, admittedly, but the cost is only in the error path.

::: xpcom/glue/nsThreadUtils.cpp:189
(Diff revision 1)
> +    NS_WARNING("Please call NS_DispatchToCurrentThread for dispatching "
> +               "event to the same thread");

I'm pretty sure this is going to cause a ton of warning spam in tests for no good reason.  This entire block won't be necessary with the alternate approch above.
Comment on attachment 8659627 [details]
MozReview Request: Bug 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj

https://reviewboard.mozilla.org/r/18989/#review17051

I'm not a huge fan of the manually-constructed already_AddRefed things here, but I think the alternative with lots of extra variables and liberal use of .forget() might be worse.

::: xpcom/threads/nsThread.cpp:516
(Diff revision 1)
> +  nsIRunnable* event = aEvent.take();

If I understand correctly, the changes to this function don't have anything to do with the DispatchInternal leaking changes, right?  Please make them a separate changeset.

In any event, the comment from DispatchInternal should be replicated here.

::: xpcom/threads/nsThread.cpp:568
(Diff revision 1)
> +  NS_RELEASE(event); // release event because it's now hold by tracedRunnable

We should make CreateTracedRunnable take an already_AddRefed to avoid this.

::: xpcom/threads/nsThread.cpp:586
(Diff revision 1)
> +    event = wrapper;

I think it's worth declaring a separate variable here--maybe it's a little clearer that we don't care about event after this point.  I suppose it would also help if we put the non-sync dispatch case first to help illustrate that...

::: xpcom/threads/nsThread.cpp:590
(Diff revision 1)
> +      // PutEvent leaked the wrapper runnable object on failure, so we

Nit: "...leaked the *wrapped* runnable object on failure"

::: xpcom/threads/nsThread.cpp:593
(Diff revision 1)
> +      // Explicitly leak the inner event since we fail to dispatch it.

Nit: "...since we failed to dispatch it."
Attachment #8659627 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #17)
> ::: xpcom/threads/nsThread.cpp:590
> (Diff revision 1)
> > +      // PutEvent leaked the wrapper runnable object on failure, so we
> 
> Nit: "...leaked the *wrapped* runnable object on failure"

It should indeed be *wrapper* because we pass the wrapper to PutEvent() here. Probably using "event" here is confusing. I'll change it.
Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default.

This class can be used instead of raw pointer for a sound leaking-by-default
behavior. Also it could take advantage of move semantic check in the future.
Attachment #8664084 - Flags: review?(nfroyd)
Bug 1186745 part 2 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called.
Attachment #8664085 - Flags: review?(nfroyd)
Bug 1186745 part 3 - Make TracedRunnable accept an already_AddRefed instead of a raw pointer.
Attachment #8664086 - Flags: review?(nfroyd)
Comment on attachment 8659627 [details]
MozReview Request: Bug 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj

Bug 1186745 part 4 - Make nsThread::Dispatch() always leak the event if it fails.
Attachment #8659627 - Attachment description: MozReview Request: Bug 1186745 part 1 - Make nsThread::Dispatch() always leak the event if it fails to dispatch it. → MozReview Request: Bug 1186745 part 4 - Make nsThread::Dispatch() always leak the event if it fails.
Attachment #8659627 - Flags: review?(nfroyd)
Comment on attachment 8659628 [details]
MozReview Request: Bug 1186745 part 6 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj

Bug 1186745 part 5 - Fix event leak when using NS_DispatchToCurrentThread.
Attachment #8659628 - Attachment description: MozReview Request: Bug 1186745 part 2 - Fix event leak when using NS_DispatchToCurrentThread. → MozReview Request: Bug 1186745 part 5 - Fix event leak when using NS_DispatchToCurrentThread.
Attachment #8659628 - Flags: review?(nfroyd)
Comment on attachment 8664086 [details]
MozReview Request: Bug 1186745 part 4 - Make TracedRunnable accept an already_AddRefed instead of a raw pointer. r=froydnj

https://reviewboard.mozilla.org/r/19919/#review17967
Attachment #8664086 - Flags: review?(nfroyd) → review+
Comment on attachment 8664084 [details]
MozReview Request: Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default.

https://reviewboard.mozilla.org/r/19915/#review17969

::: mfbt/LeakRefPtr.h:7
(Diff revision 1)
> +/* Smart pointer which leaks its owning refcounted object by default. */

Nit: "...leaks it's owning..."

::: mfbt/LeakRefPtr.h:17
(Diff revision 1)
> + * Instance of this class behaves like a raw pointer which leaks the
> + * resource its owning if not explicitly released.

This is a great way to encapsulate what's going on; thanks for writing it up.

I don't think it's general enough to be put in MFBT or even exported from XPCOM, though.  Please move this file to xpcom/threads/.  It shouldn't appear in EXPORTS, though, which means xpcom/glue/'s moz.build (and all the moz.build variants underneath xpcom/glue/) will need some changes to LOCAL_INCLUDES for things to build properly.

::: mfbt/LeakRefPtr.h:18
(Diff revision 1)
> + * resource its owning if not explicitly released.

Nit: "...resource it's owning".
Attachment #8664084 - Flags: review?(nfroyd)
Attachment #8664085 - Flags: review?(nfroyd) → review+
Comment on attachment 8664085 [details]
MozReview Request: Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called.

https://reviewboard.mozilla.org/r/19917/#review17971

I'm curious how often this happens, but I guess it's good for consistency...
Comment on attachment 8659627 [details]
MozReview Request: Bug 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj

https://reviewboard.mozilla.org/r/18989/#review17975

::: xpcom/threads/nsThread.cpp:616
(Diff revision 2)
> +      wrapper->Release();

I am reasonable certain that this construct will fail static analysis builds, so we have to find some other way of handling this.
Attachment #8659627 - Flags: review?(nfroyd)
Comment on attachment 8659628 [details]
MozReview Request: Bug 1186745 part 6 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj

https://reviewboard.mozilla.org/r/18991/#review17977
Attachment #8659628 - Flags: review?(nfroyd) → review+
Comment on attachment 8664084 [details]
MozReview Request: Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default.

Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default.

This class can be used instead of raw pointer for a sound leaking-by-default
behavior. Also it could take advantage of move semantic check in the future.
Attachment #8664084 - Flags: review?(nfroyd)
Bug 1186745 part 2 - Move nsThreadSyncDispatch class to its own header file.
Attachment #8665376 - Flags: review?(nfroyd)
Comment on attachment 8664085 [details]
MozReview Request: Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called.

Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called. r=froydnj
Attachment #8664085 - Attachment description: MozReview Request: Bug 1186745 part 2 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called. → MozReview Request: Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called. r=froydnj
Comment on attachment 8664086 [details]
MozReview Request: Bug 1186745 part 4 - Make TracedRunnable accept an already_AddRefed instead of a raw pointer. r=froydnj

Bug 1186745 part 4 - Make TracedRunnable accept an already_AddRefed instead of a raw pointer. r=froydnj
Attachment #8664086 - Attachment description: MozReview Request: Bug 1186745 part 3 - Make TracedRunnable accept an already_AddRefed instead of a raw pointer. → MozReview Request: Bug 1186745 part 4 - Make TracedRunnable accept an already_AddRefed instead of a raw pointer. r=froydnj
Attachment #8659627 - Flags: review?(nfroyd)
Comment on attachment 8659627 [details]
MozReview Request: Bug 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj

Bug 1186745 part 4 - Make nsThread::Dispatch() always leak the event if it fails.
Attachment #8659628 - Attachment description: MozReview Request: Bug 1186745 part 5 - Fix event leak when using NS_DispatchToCurrentThread. → MozReview Request: Bug 1186745 part 5 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj
Comment on attachment 8659628 [details]
MozReview Request: Bug 1186745 part 6 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj

Bug 1186745 part 5 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj
Assignee: rjesup → quanxunzhen
Comment on attachment 8664084 [details]
MozReview Request: Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default.

https://reviewboard.mozilla.org/r/19915/#review18355

::: xpcom/threads/LeakRefPtr.h:7
(Diff revision 2)
> +/* Smart pointer which leaks it's owning refcounted object by default. */

I'm sorry--and this is completely on me--but "its" is the right variant here.  I'm not sure what I was thinking with the previous review comment. :(
Attachment #8664084 - Flags: review?(nfroyd) → review+
Comment on attachment 8665376 [details]
MozReview Request: Bug 1186745 part 2 - Move nsThreadSyncDispatch class to its own header file. r=froydnj

https://reviewboard.mozilla.org/r/20211/#review18359

I don't see why this is a good change.  nsThreadSyncDispatch is tied to how we're doing sync dispatch internally; it's not likely to be useful in contexts outside of that, and it's not used in later patches in this series.
Attachment #8665376 - Flags: review?(nfroyd)
Attachment #8659627 - Flags: review?(nfroyd) → review+
Comment on attachment 8659627 [details]
MozReview Request: Bug 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj

https://reviewboard.mozilla.org/r/18989/#review18361

Thanks for following through and being patient on this series.  Sorry that it's taken so long to get to this point.
(In reply to Nathan Froyd [:froydnj] from comment #38)
> Comment on attachment 8665376 [details]
> MozReview Request: Bug 1186745 part 2 - Move nsThreadSyncDispatch class to
> its own header file.
> 
> https://reviewboard.mozilla.org/r/20211/#review18359
> 
> I don't see why this is a good change.  nsThreadSyncDispatch is tied to how
> we're doing sync dispatch internally; it's not likely to be useful in
> contexts outside of that, and it's not used in later patches in this series.

Because I use LeakRefPtr in nsThreadSyncDispatch in the next path. nsThread.h is an export header, while LeakRefPtr.h is not. If I keep nsThreadSyncDispatch in nsThread.h, we need to also export LeakRefPtr.h, otherwise nsThread.h won't compile outside the two directories.
Attachment #8665376 - Flags: review?(nfroyd)
Comment on attachment 8665376 [details]
MozReview Request: Bug 1186745 part 2 - Move nsThreadSyncDispatch class to its own header file. r=froydnj

https://reviewboard.mozilla.org/r/20211/#review18439

Thanks for the explanation.  I think the better solution would be to move it into nsThread.cpp...but bleh, I see that nsThreadPool.cpp uses it too (which is presumably why you didn't move it into nsThread.cpp in the first place), so that won't work.

OK, I think this is probably the best solution.  I wish the nsThread/nsThreadPool sync dispatching was unified, but I don't think it's fair to fix that in this bug.
Attachment #8665376 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e9783023fb2091856dbe5407c5c0a3fc3220386
Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce960a661987dacec560459797e720d442ccb912
Bug 1186745 part 2 - Move nsThreadSyncDispatch class to its own header file. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/383f8ac033ea6a9951779b06ee463c95d230a4ab
Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/edc0b56d81faf04f67462221aaaa566319df8c39
Bug 1186745 part 4 - Make TracedRunnable accept an already_AddRefed instead of a raw pointer. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8f740ef24304bb4088be28ba1c0204962aeadf1
Bug 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6142b815de04b37c1b798b79729dbdb4e827330
Bug 1186745 part 6 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj
I'm completely not able to reproduce the cpptest failure locally on Linux, even with the commit on m-i... Not sure what's wrong there...
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #45)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=839c394ca236

This result shows that the event is dispatched successfully, but the object is not released properly.
Comment on attachment 8664084 [details]
MozReview Request: Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default.

Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default.

This class can be used instead of raw pointer for a sound leaking-by-default
behavior. Also it could take advantage of move semantic check in the future.
Attachment #8665376 - Attachment description: MozReview Request: Bug 1186745 part 2 - Move nsThreadSyncDispatch class to its own header file. → MozReview Request: Bug 1186745 part 2 - Move nsThreadSyncDispatch class to its own header file. r=froydnj
Comment on attachment 8665376 [details]
MozReview Request: Bug 1186745 part 2 - Move nsThreadSyncDispatch class to its own header file. r=froydnj

Bug 1186745 part 2 - Move nsThreadSyncDispatch class to its own header file. r=froydnj
Attachment #8664085 - Attachment description: MozReview Request: Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called. r=froydnj → MozReview Request: Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called.
Comment on attachment 8664085 [details]
MozReview Request: Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called.

Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called.
Comment on attachment 8664086 [details]
MozReview Request: Bug 1186745 part 4 - Make TracedRunnable accept an already_AddRefed instead of a raw pointer. r=froydnj

Bug 1186745 part 4 - Make TracedRunnable accept an already_AddRefed instead of a raw pointer. r=froydnj
Comment on attachment 8659627 [details]
MozReview Request: Bug 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj

Bug 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj
Attachment #8659627 - Attachment description: MozReview Request: Bug 1186745 part 4 - Make nsThread::Dispatch() always leak the event if it fails. → MozReview Request: Bug 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj
Comment on attachment 8659628 [details]
MozReview Request: Bug 1186745 part 6 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj

Bug 1186745 part 6 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj
Attachment #8659628 - Attachment description: MozReview Request: Bug 1186745 part 5 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj → MozReview Request: Bug 1186745 part 6 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj
Comment on attachment 8664084 [details]
MozReview Request: Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default.

A small change: https://reviewboard.mozilla.org/r/19915/diff/2-3/
Attachment #8664084 - Flags: review+ → review?(nfroyd)
Comment on attachment 8664085 [details]
MozReview Request: Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called.

A small change: https://reviewboard.mozilla.org/r/19917/diff/2-3/
Attachment #8664085 - Flags: review+ → review?(nfroyd)
Comment on attachment 8664084 [details]
MozReview Request: Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default.

https://reviewboard.mozilla.org/r/19915/#review18971

::: xpcom/threads/LeakRefPtr.h:18
(Diff revisions 2 - 3)
> - * resource it's owning if not explicitly released.
> + * resource its owning if not explicitly released.

Nit: "it's" (short for "it is") is the correct usage here.  "its" (indicating possession of the refcounted object) is the correct usage on line 7.

::: xpcom/threads/LeakRefPtr.h:43
(Diff revisions 2 - 3)
> -  T* mRawPtr;
> +  T* MOZ_OWNING_REF mRawPtr;

Hooray for static analysis.
Attachment #8664084 - Flags: review?(nfroyd) → review+
Comment on attachment 8664085 [details]
MozReview Request: Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called.

https://reviewboard.mozilla.org/r/19917/#review18973
Attachment #8664085 - Flags: review?(nfroyd) → review+
Blocks: 1210749
https://hg.mozilla.org/integration/mozilla-inbound/rev/df9cb8f5f0a5eb1fa6bfe93d501f84fb0d79b077
Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a31df8787f0d80600ae8446b53eea44ab1f4375
Bug 1186745 part 2 - Move nsThreadSyncDispatch class to its own header file. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/e36909748ddf51e8d3a0d40f2fb5328b1db83f53
Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/73f73b531fc892ba8dae447528bba09ed2ee43ff
Bug 1186745 part 4 - Make TracedRunnable accept an already_AddRefed instead of a raw pointer. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b530871783ac74a6dcb8a3dbf46798a991bbc87
Bug 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/237a6acf0709d7392e7dc25c13e699f715b0dcf5
Bug 1186745 part 6 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj
Ahhhh..... Android cpp bustage?! On opt build only?! No stack info at all... How am I supposed to fix something like this...
OK I know why... I probably shouldn't have touched the nsThreadSyncDispatch at all, various concurrent issue happens there.

I should actually appreciate those cpp tests for that they help finding those issues and stopped me from landing problematic code.
Flags: needinfo?(quanxunzhen)
Comment on attachment 8664084 [details]
MozReview Request: Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default.

Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default.

This class can be used instead of raw pointer for a sound leaking-by-default
behavior. Also it could take advantage of move semantic check in the future.
Comment on attachment 8665376 [details]
MozReview Request: Bug 1186745 part 2 - Move nsThreadSyncDispatch class to its own header file. r=froydnj

Bug 1186745 part 2 - Move nsThreadSyncDispatch class to its own header file. r=froydnj
Comment on attachment 8664085 [details]
MozReview Request: Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called.

Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called.
Comment on attachment 8664086 [details]
MozReview Request: Bug 1186745 part 4 - Make TracedRunnable accept an already_AddRefed instead of a raw pointer. r=froydnj

Bug 1186745 part 4 - Make TracedRunnable accept an already_AddRefed instead of a raw pointer. r=froydnj
Comment on attachment 8659627 [details]
MozReview Request: Bug 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj

Bug 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj
Comment on attachment 8659628 [details]
MozReview Request: Bug 1186745 part 6 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj

Bug 1186745 part 6 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj
Comment on attachment 8664084 [details]
MozReview Request: Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default.

https://reviewboard.mozilla.org/r/19915/diff/3-4/

Add two methods to LeakRefPtr for usage in nsThreadSyncDispatch.
Attachment #8664084 - Flags: review+ → review?(nfroyd)
Comment on attachment 8664085 [details]
MozReview Request: Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called.

https://reviewboard.mozilla.org/r/19917/diff/3-4/

Because of bug 1210747, I still think it is better not to use nsCOMPtr for object across threads, and thus I keep using LeakRefPtr in nsThreadSyncDispatch. Now, it should behave identical like before except that it now leaks by default.
Attachment #8664085 - Flags: review+ → review?(nfroyd)
Comment on attachment 8664084 [details]
MozReview Request: Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default.

https://reviewboard.mozilla.org/r/19915/#review19173

This has turned out to be a lot of work to make things more consistent. :)
Attachment #8664084 - Flags: review?(nfroyd) → review+
Attachment #8664085 - Flags: review?(nfroyd) → review+
Comment on attachment 8664085 [details]
MozReview Request: Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called.

https://reviewboard.mozilla.org/r/19917/#review19175
https://hg.mozilla.org/integration/mozilla-inbound/rev/0de9cc9cb4059a867c921a7263f4f63e579351df
Bug 1186745 part 1 - Add LeakRefPtr for pointer leaking by default. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ba8d5b80bd08f0c339f97a239fdc25b3f6b9054
Bug 1186745 part 2 - Move nsThreadSyncDispatch class to its own header file. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed3daedff60095e9616569bcfcfe69e78359d90c
Bug 1186745 part 3 - Make nsThreadSyncDispatch leak the sync task by default when Run() is not called. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1be5e72cdc74ad5c75ecbf65ef424f9f119cded
Bug 1186745 part 4 - Make TracedRunnable accept an already_AddRefed instead of a raw pointer. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/83024a3a95863c73bddc8a2704cc13c1a6be13c1
Bug 1186745 part 5 - Make nsThread::Dispatch() always leak the event if it fails. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/9400b5b4f6da8054b131dbaa0fa8da7dfe78c032
Bug 1186745 part 6 - Fix event leak when using NS_DispatchToCurrentThread. r=froydnj
Regressions: 1360269
You need to log in before you can comment on or make changes to this bug.