1157454 - TSan: data race dom/crypto/WebCryptoTask.cpp:307 WebCryptoTask::CalculateResult

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nathan Froyd [:froydnj]]

Attachment: webcryptotask-race.txt

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

* Specific information about this bug

TSan is complaining that we're reading and writing WebCryptoTask::mEarlyRv on different threads, but there's no synchronization between the threads--to ensure the write happens before the read, as the code expects.  (I guess you can argue that even if the write *doesn't* happen before the read, that's OK, because we wouldn't be doing the read off the main thread if the dispatch of the task didn't succeed?  Still seems sketchy to me.)

Running the dom/crypto/ tests triggers this race quite a number of times.

* 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

[Tim Taubert [:ttaubert] (inactive)]

I do actually think that the race in this case is benign.

mEarlyRv is a variable that indicates a failure only in the early phase (i.e. when constructing and/or initializing a WebCryptoTask). This is all done on the main thread and thus mEarlyRv is only set on the main thread. All WebCryptoTasks set mEarlyRv only from their constructor or their ::Init() function which is always called by the constructor.

After the task is created, we call |task->DispatchWithPromise(promise)| at which point we check mEarlyRv and if that doesn't indicate any failures we dispatch |task| to a background thread.

Now CalculateResult() is called on the background thread and runs the computations. Accessing mEarlyRv at this point doesn't make sense but is also benign. After the task was dispatched we don't actually read/write/need mEarlyRv anymore if I didn't miss anything.

TL;DR: While I think that this data race is benign we should be able to satisfy TSan by just removing the mEarlyRv check from CalculateResult() as it's pointless to check it at this stage in the task's lifetime.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1157454-Satisfy-TSan-by-removing-the-unneeded-mE.patch

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e712bae4880

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Nathan, can you please check if the data race reported by TSan is gone?

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e712bae4880

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Tim Taubert [:ttaubert] from comment #4)
> Nathan, can you please check if the data race reported by TSan is gone?

I no longer see this race.  Thanks for fixing it!