Closed Bug 1219910 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tsan])

Attachments

(2 files, 1 obsolete file)

Attached file 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
All the places that (re-)declared gSocketThread already included
nsSocketTransportService2.h, so we can safely delete them.
Attachment #8680859 - Flags: review?(mcmanus)
Comment on attachment 8680859 [details] [diff] [review]
make gSocketThread a relaxed atomic variable

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

thanks
Attachment #8680859 - Flags: review?(mcmanus) → review+
(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...
Flags: needinfo?(nfroyd)
Turns out a lot of places that redeclared gSocketThread only used it in DEBUG
builds, so my normal opt build wasn't catching things.
Attachment #8684296 - Flags: review?(mcmanus)
Attachment #8680859 - Attachment is obsolete: true
Comment on attachment 8684296 [details] [diff] [review]
make gSocketThread a relaxed atomic variable

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

even better
Attachment #8684296 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/mozilla-central/rev/82b29ff70513
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.