967936 - TSan: data race js/src/../../js/src/jsscript.h:423 hasSourceData

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Logfile with TSan trace

The attached logfile shows a thread/data race (mozilla-central revision 44ba69cacd7e) detected by TSan (ThreadSanitizer).

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 inacceptable performance issues. Also note that seemingly benign races can possibly be harmful (also depending on the compiler and the architecture) [1].

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

Comment 1

[Jan de Mooij [:jandem]]

This is the source compression thread racing with hasSourceData() on tbe main thread.

Comment 2

[:Benjamin Peterson]

Attachment: reorder.patch

I believe the race is benign, but this patch should ensure it doesn't happen again.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8370947 [details] [diff] [review]
reorder.patch

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

Grrr. Benjamin, why didn't we design this so that minimal data is shared by the two threads? I think all we need to share is the source to be compressed, which both threads treat as read-only, and the abort_ bit. The point I'm making here is: it could have been trivially correct.

The change looks good. Please also modify the comments in `class ScriptSource` to document which fields may be written by the source compression thread, and under what circumstances reading/writing is safe. r=me with that.

Comment 4

[:Benjamin Peterson]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> Comment on attachment 8370947 [details] [diff] [review]
> reorder.patch
> 
> Review of attachment 8370947 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Grrr. Benjamin, why didn't we design this so that minimal data is shared by
> the two threads? I think all we need to share is the source to be
> compressed, which both threads treat as read-only, and the abort_ bit. The
> point I'm making here is: it could have been trivially correct.

I think long ago it was more trivially correct but then coordination between the two threads became more sophisticated (and buggy!) over time. Not that excuses the current state of affairs, but I did try to make it dead simple initially.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a3cb44e8bdd

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3a3cb44e8bdd