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

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: froydnj, Assigned: mayhemer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla38
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [tsan])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8547773 [details]
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
(Assignee)

Comment 1

4 years ago
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
(Assignee)

Comment 2

4 years ago
|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)
(Assignee)

Comment 3

4 years ago
Not sure why this ni was added...
Flags: needinfo?(nfroyd)
(Assignee)

Comment 4

4 years ago
Created attachment 8561432 [details] [diff] [review]
v1

Nathan, feel free to forward to e.g. :michal novotny.
Attachment #8561432 - Flags: review?(nfroyd)
(Reporter)

Comment 5

4 years ago
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
(Assignee)

Updated

4 years ago
Attachment #8561432 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8564176 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8ed4a862c8e8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.