845741 - TSan: Thread data race in mozilla::MediaStreamGraphImpl::ExtractPendingInput() vs. mozilla::SourceMediaStream::FinishWithLockHeld()

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Christoph Diehl [:posidron]]

Attachment: trace

During initial fuzz tests with ThreadSanitizer (LLVM version), we get a data race reported as described in the attached log. Trace was created on mozilla-central with changeset 122820:c233837cce08.

According to the TSan devs, most of the reported traces should be real data races, even though they can be "benign". We need to determine if the race can/should be fixed, or put on the ignore list. Even for benign races, TSan devs suggest to fix them (second priority), as they can also cause problems [1].

[1] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong

Comment 1

[Christoph Diehl [:posidron]]

This happened during fuzzing the JSEP in two separate windows.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

This is almost certainly a data race on mUpdateFinished.  It's set on the caller side (Mainthread) and read in MSG without any locking.  This could be resolved by having Finish operate on the MSG thread by inserting a message instead of just setting a flag.  (I'm not sure if that would cause any other issues, however)

I haven't checked all the versions, but I think this affects all the way back to FF19, probably before.

I suspect the downside might be MSG thread taking longer to notice that Finish() or EndAllTracksAndFinish() was called.  I don't know if that would cause any real problems so long as they eventually synchronize; I haven't thought it through. (Tim?)

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Set mUpdateFinished under lock

Looked at this several times before seeing my mistake: when I switched to FinishWithLockHeld(), I didn't remove the level of braces around the AutoLock

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 734235 [details] [diff] [review]
Set mUpdateFinished under lock

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

thanks for taking this!!

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59e93bff76d4

We'll want this for Aurora/22, and it's a small-enough/safe-enough bug to consider for Beta/21

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59e93bff76d4

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 734235 [details] [diff] [review]
Set mUpdateFinished under lock

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 803976

User impact if declined: TSAN bugs are inherently risky and compiler/optimizer and runtime timing dependent.  Nuclear missiles could be launched (to borrow a phrase from the "there are no such things as benign data races" article.

Testing completed (on m-c, etc.): on m-c

Risk to taking this patch (and alternatives if risky): No appreciable risk

String or IDL/UUID changes made by this patch: none

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/76f7efd3e9c1

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Roc - should we nominate this for beta?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it's probably not worth it.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Thanks!