If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Investigate if DISPATCH_SYNC dispatching is implemented correctly

RESOLVED FIXED in Firefox 53

Status

()

Core
XPCOM
RESOLVED FIXED
11 months ago
9 months ago

People

(Reporter: smaug, Assigned: Waldo)

Tracking

50 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
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
(Reporter)

Comment 1

11 months ago
'safe' meaning guaranteed to actually work.
Created attachment 8808033 [details] [diff] [review]
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.
Attachment #8808033 - Flags: review?(nfroyd)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED

Comment 3

11 months ago
The origin dispatch is what wakes up http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/xpcom/threads/nsThread.cpp#760 to unblock the caller.
Oh, sync dispatch can spin the current thread's event loop.  Bah, what a mess.
Treeherder results, after a typo-fix:

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

Comment 6

11 months ago
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

10 months ago
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.
Attachment #8808033 - Flags: review?(nfroyd) → review+

Comment 8

9 months ago
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ce718b9bae6
Make nsThreadSyncDispatch::IsPending properly respect the C++ memory model with respect to threads.  r=froydnj
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.
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.
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.
Thanks for the explanation. We need |task->Run()| to happen before |mIsPending = false;|. However the relaxed semantics doesn't guarantee that.

Comment 13

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0ce718b9bae6
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.