1120631 - TSan: data race netwerk/cache2/CacheEntry.cpp:371 Load

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Nathan Froyd [:froydnj]]

Attachment: tsan-cache-race.txt

[cribbing from decoder's script, hopefully he won't mind]

The attached logfile shows a thread/data race 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 unacceptable 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

[Honza Bambas (:mayhemer)]

This needs a little bit more investigation.  More then just a race on an atomic happens here.  I smell a bug, but fix for that will not fix the warning (tho, that may become truly benign with the fix).

Comment 2

[Honza Bambas (:mayhemer)]

|enum class nsresult : uint32_t| declared class member is not atomically accessible on 64 bit builds.  mozilla::Atomic with just rel_acq ordering will do here.

Comment 3

[Honza Bambas (:mayhemer)]

Not sure why this ni was added...

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1

Nathan, feel free to forward to e.g. :michal novotny.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8561432 [details] [diff] [review]
v1

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

ReleaseAcquire is sort of odd to use in this context, but I think it makes sense, because you just want to ensure that you see all prior writes to the location.  I think making it Relaxed would work too; that's effectively what you have now, but the compiler would then know that you're explicitly specifying that the races (such as they are) are OK.

I'm going to forward the review to Michal because I don't know enough about the cache to say whether the hit you take from ReleaseAcquire vs. Relaxed (on ARM devices, anyway) is OK here.

Comment 6

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb4fb9d0b230

Comment 7

[Carsten Book [:Tomcat]]

since this is the same try run as for bug 881830 - are this failures in the try run related to this patch ?

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v1 (ci msg)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c34ac09f8042

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed4a862c8e8

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ed4a862c8e8