1671577 - ThreadSanitizer: data race [@ OnChannelConnected] vs. [@ mozilla::ipc::MessageChannel::AddProfilerMarker]

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Christian Holler (:decoder)]

The attached crash information was detected while running CI tests with ThreadSanitizer on try revision 1fdd4f52d08fd1fb01b6c77c4a86b03af275855d.

It looks like we are racing on setting mPeerPid vs. reading it. This could cause visibility issues (update the mPeerPid is never observed) or random behavior where the correct pid is sometimes observed and sometimes not.

General information about TSan reports

Why fix races?

Data races are undefined behavior and can cause crashes as well as correctness issues. Compiler optimizations can cause racy code to have unpredictable and hard-to-reproduce behavior.

Rating

If you think this race can cause crashes or correctness issues, it would be great to rate the bug appropriately as P1/P2 and/or indicating this in the bug. This makes it a lot easier for us to assess the actual impact that these reports make and if they are helpful to you.

False Positives / Benign Races

Typically, races reported by TSan are not false positives [1], but it is possible that the race is benign. Even in this case it would be nice to come up with a fix if it is easily doable and does not regress performance. Every race that we cannot fix will have to remain on the suppression list and slows down the overall TSan performance. Also note that seemingly benign races can possibly be harmful (also depending on the compiler, optimizations and the architecture) [2][3].

[1] One major exception is the involvement of uninstrumented code from third-party libraries.
[2] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
[3] How to miscompile programs with "benign" data races: https://www.usenix.org/legacy/events/hotpar11/tech/final_files/Boehm.pdf

Suppressing unfixable races

If the bug cannot be fixed, then a runtime suppression needs to be added in mozglue/build/TsanOptions.cpp. The suppressions match on the full stack, so it should be picked such that it is unique to this particular race. The bug number of this bug should also be included so we have some documentation on why this suppression was added.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Nika, do you know what kind of synchronization is expected on mPeerPid?

It would be nice if we could prioritize this fix because any temporary suppression would be highly generic (we need to suppress both stacks and the second stack would cause us to suppress all stacks with OnChannelConnected in them).

Comment 3

[Andrew McCreight [:mccr8]]

It looks like mPeerPid is otherwise only read or written to on the worker thread, but bug 1557447 added this read on the main thread. We're holding the monitor on the main thread, but not the worker thread. Maybe we could acquire the monitor in MessageChannel::OnChannelConnected()? Presumably we don't connect channels that much so hopefully it wouldn't affect performance.

Comment 4

[Andrew McCreight [:mccr8]]

I guess in comment 3 I mean "IO thread" and not "worker thread".

Comment 5

[Andrew McCreight [:mccr8]]

Maybe we could just compute the other pid via mListener->OtherPid(); and avoid mPeerPid altogether? I'm not sure if there's some specific reason not to do that.

Comment 6

[Andrew McCreight [:mccr8]]

It looks like an older version of the profiler patch had the comment "NOTE: mPeerPid has slightly different behaviour from IProtocol::OtherPid (e.g. is -1 for same-process messages)", but it got changed since then to make the behavior the same as OtherPid, so that shouldn't be an issue.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Bug 1671577 - Use OtherPid() in AddProfilerMarker().

Aside from its use in AddProfilerMarker(), after initialization mPeerPid
is only used on the IO thread, so the write to it does not hold the monitor.
This means that the read in AddProfilerMarker() can cause a race, even
though we hold the monitor. This method is only called when we hold
the monitor and everything is set up, so I think we can just use
mListener->OtherPid() to get the PID.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11c6d8f18687
Use OtherPid() in AddProfilerMarker(). r=jld

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/11c6d8f18687