Closed Bug 1120631 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Networking: Cache, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: froydnj, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tsan])

Attachments

(2 files, 1 obsolete file)

Attached file 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
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).
Assignee: nobody → honzab.moz
|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.
Status: NEW → ASSIGNED
Flags: needinfo?(nfroyd)
Not sure why this ni was added...
Flags: needinfo?(nfroyd)
Attached patch v1 (obsolete) — Splinter Review
Nathan, feel free to forward to e.g. :michal novotny.
Attachment #8561432 - Flags: review?(nfroyd)
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.
Attachment #8561432 - Flags: review?(nfroyd)
Attachment #8561432 - Flags: review?(michal.novotny)
Attachment #8561432 - Flags: review+
Attachment #8561432 - Flags: review?(michal.novotny) → review+
since this is the same try run as for bug 881830 - are this failures in the try run related to this patch ?
Flags: needinfo?(honzab.moz)
Keywords: checkin-needed
Attachment #8561432 - Attachment is obsolete: true
Attachment #8564176 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8ed4a862c8e8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.