1219910 - TSan: data race netwerk/base/nsSocketTransportService2.cpp:1223 OnKeepaliveEnabledPrefChange (race on gSocketThread)

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Nathan Froyd [:froydnj]]

Attachment: TSan stack trace

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

* Specific information about this bug

The race here looks like:

- (main thread) We spawn the socket thread in nsSocketTransportService::Init
- (socket thread) The socket thread's runnable Run() (nsSocketTransportService::Run) starts executing
- (socket thread) gSocketThread is set
- (main thread) nsSocketTransportService::UpdatePrefs runs
- (main thread) nsSocketTransportService::OnKeepaliveEnabledPrefChange runs
- (main thread) gSocketThread is read, resulting in a race.

AFAICT, the race here is harmless: we will either see nullptr in OnKeepaliveEnabledPrefChange, resulting in a dispatch to the socket thread, or we will see the intended value of gSocketThread, which also results in a dispatch to the socket thread.  Either way, we get the intended result.

Using a relaxed atomic here should be enough to make TSan understand what's going on.
* 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

[Nathan Froyd [:froydnj]]

Attachment: make gSocketThread a relaxed atomic variable

All the places that (re-)declared gSocketThread already included
nsSocketTransportService2.h, so we can safely delete them.

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 8680859 [details] [diff] [review]
make gSocketThread a relaxed atomic variable

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

thanks

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a233dadfc49

Comment 4

[Carsten Book [:Tomcat]]

backed out for bustage -> https://treeherder.mozilla.org/logviewer.html#?job_id=16551758&repo=mozilla-inbound

Comment 5

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a49d8f9fc4d

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Carsten Book [:Tomcat] from comment #4)
> backed out for bustage ->
> https://treeherder.mozilla.org/logviewer.html#?job_id=16551758&repo=mozilla-
> inbound

Sigh, looks like a lot of places (re-)declare gSocketThread, but those declarations only get used in DEBUG, or something like that?  I wonder if they all pick up nsSocketTransportService2.h anyway...

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: make gSocketThread a relaxed atomic variable

Turns out a lot of places that redeclared gSocketThread only used it in DEBUG
builds, so my normal opt build wasn't catching things.

Comment 8

[Patrick McManus [:mcmanus]]

Comment on attachment 8684296 [details] [diff] [review]
make gSocketThread a relaxed atomic variable

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

even better

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82b29ff70513

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/82b29ff70513