1190985 - TSan: data race netwerk/base/nsSocketTransportService2.cpp:70 DebugMutexAutoLock

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Nathan Froyd [:froydnj]]

The attached logfile shows a thread/data race detected by TSan (ThreadSanitizer).

* Specific information about this bug

TSan is unhappy about DebugMutexAutoLock::sDebugOwningThread because it gets read outside of a lock on the main thread while being written (inside a lock) on a non-main thread.

I think this is actually safe, but it'd be reasonable to make sDebugOwningThread an Atomic<PRThread*, Relaxed> to convince TSan we really know what we're doing here.
* General information about TSan, data races, etc.

Typically, races reported by TSan are not false positives, but it is possible that the race is benign. Even in this case though, we should try to come up with a fix unless this would cause unacceptable performance issues. Also note that seemingly benign races can possibly be harmful (also depending on the compiler and the architecture) [1][2].

If the bug cannot be fixed, then this bug should be used to either make a compile-time annotation for blacklisting or add an entry to the runtime blacklist.

[1] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
[2] _How to miscompile programs with "benign" data races_: https://www.usenix.org/legacy/events/hotpar11/tech/final_files/Boehm.pdf

Comment 1

[u408661]

Attachment: Fix data race in DebugMutexAutoLock

This should do the trick.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=418c431c5d44

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8643816 [details] [diff] [review]
Fix data race in DebugMutexAutoLock

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

Looks great, thank you!

::: netwerk/base/nsSocketTransportService2.cpp
@@ +54,5 @@
>  {
>  public:
>      explicit DebugMutexAutoLock(Mutex& mutex);
>      ~DebugMutexAutoLock();
> +    static void Init() { sDebugOwningThread = nullptr; }

FWIW, this is unnecessary because...

@@ +63,3 @@
>  };
>  
> +Atomic<PRThread *, Relaxed> DebugMutexAutoLock::sDebugOwningThread;

...this is automatically initialized to null.

Comment 3

[u408661]

Attachment: Fix data race in DebugMutexAutoLock

Update with :froydnj's comments (thanks!)

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8643825 [details] [diff] [review]
Fix data race in DebugMutexAutoLock

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

thanks nathan, thanks nick

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b893803cc40

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b893803cc40

Comment 7

[Nicholas Nethercote [inactive]]

Nice work, all y'all.