1315560 - Investigate if DISPATCH_SYNC dispatching is implemented correctly

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

If I haven't missed anything, the implementation relies on mSyncTask usage.
That member variable is cleared on the target thread and read on the caller thread, but accessing isn't synchronized (no locks or such) or using atomics.
Why is that safe?

http://searchfox.org/mozilla-central/source/xpcom/threads/nsThreadSyncDispatch.h
http://searchfox.org/mozilla-central/source/xpcom/threads/nsThread.cpp#746-748,759

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

'safe' meaning guaranteed to actually work.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Untested, merely-eyeballed patch

(In reply to Olli Pettay [:smaug] from comment #0)
Yeah, that doesn't look like it respects the C++ memory model.

This is maybe unrelated, but I'm not sure what |mOrigin->Dispatch(this, NS_DISPATCH_NORMAL);| is exactly supposed to do there.  Dispatch an apparent no-op event back at the origin thread, but why?  The origin's just blocking on the target.  Why does it need to have an event dispatched at it?  Also worth noting that, because I'm not sure what the point of this is, I'm not entirely certain the atomic release here is in the right place with respect to this dispatch.  But this replicates existing behavior (sort of) so at least isn't a step backward from now, seems like.

Comment 3

[Benjamin Smedberg]

The origin dispatch is what wakes up http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/xpcom/threads/nsThread.cpp#760 to unblock the caller.

Comment 4

[Jeff Walden [:Waldo]]

Oh, sync dispatch can spin the current thread's event loop.  Bah, what a mess.

Comment 5

[Jeff Walden [:Waldo]]

Treeherder results, after a typo-fix:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74454f207fe32d077ad2e0376b1df75a538456c6

Comment 6

[Nathan Froyd [:froydnj]]

OK, so I can sort of believe this is necessary, but I'm also wondering why we've never seen this in TSan runs.  Perhaps we weren't running the right tests, or something?

It would be a bit subtle, but I think we could get by with Relaxed ordering here, since lock acquisitions and such would provide the visibility flushes we'd need...and honestly, that's basically what we're doing today...

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8808033 [details] [diff] [review]
Untested, merely-eyeballed patch

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

r=me with the Relaxed change below and removing the comments that talk about acquire/release.

::: xpcom/threads/nsThreadSyncDispatch.h
@@ +17,5 @@
>  {
>  public:
>    nsThreadSyncDispatch(nsIThread* aOrigin, already_AddRefed<nsIRunnable>&& aTask)
>      : mOrigin(aOrigin)
> +    , mSyncTask(mozilla::Move(aTask)),

I assume you've fixed the end-of-line comma here.

@@ +56,5 @@
>    nsCOMPtr<nsIThread> mOrigin;
>    // The task is leaked by default when Run() is not called, because
>    // otherwise we may release it in an incorrect thread.
>    mozilla::LeakRefPtr<nsIRunnable> mSyncTask;
> +  mozilla::Atomic<bool, mozilla::ReleaseAcquire> mIsPending;

We can safely use Relaxed consistency here; the Dispatch to unblock the origin thread will provide all the necessary memory barriers.  Please add a comment to that effect.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ce718b9bae6
Make nsThreadSyncDispatch::IsPending properly respect the C++ memory model with respect to threads.  r=froydnj

Comment 9

[Jeff Walden [:Waldo]]

Note that the landing *didn't* include the Relaxed suggestion from comment 7, 'cause it's not actually right, as I persuaded froydnj in face-to-face conversation.  :-)  With relaxed semantics, there's no guarantee the memory effects of running the task on the target thread (performed just before the |mIsPending = false;|) would be visible when |mIsPending| on the origin thread -- relaxed doesn't synchronize to set up the necessary happens-before relationship.

Synchronization yo atomics, hard are and.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

I don't quite get it.

|mIsPending = false;| performed on the target thread will eventually be visible on the original thread (guaranteed by |mOrigin->Dispatch(this, NS_DISPATCH_NORMAL);|), right? So the original thread will see |!mIsPending| someday in the future and stop spinning.

Comment 11

[Jeff Walden [:Waldo]]

It'll see !mIsPending eventually.  That's not at issue.  But when it sees !mIsPending, *it is not guaranteed to have seen the effects of running the task*.

A relaxed memory operation is atomic, but it doesn't order memory.  Because it doesn't order memory (as it would if release/acquire semantics are used, as in the final patch), the |task->Run()| before |mIsPending = false;| on the target thread, does not synchronize with an operation after |!wrapper->IsPending()| on the origin thread.  Because it doesn't synchronize with such an operation, that target-thread operation does not inter-thread happens before the origin-thread operation.

Thus *if* that origin-thread |!wrapper->IsPending()| is immediately followed by an operation that depends on the memory effects of |task->Run()| having occurred, it *would* be a data race, it *is* undefined behavior, and it *may* or may not go awry in practice.

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the explanation. We need |task->Run()| to happen before |mIsPending = false;|. However the relaxed semantics doesn't guarantee that.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0ce718b9bae6