Closed
Bug 1203655
Opened 9 years ago
Closed 9 years ago
TSan: data race netwerk/cache2/CacheFile.cpp:1723 IsDoomed (race on mHandle)
Categories
(Core :: Networking: Cache, defect)
Tracking
()
RESOLVED
DUPLICATE
of bug 1121672
People
(Reporter: froydnj, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: [tsan])
Attachments
(2 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
michal
:
review-
|
Details | Diff | Splinter Review |
The attached logfile shows a thread/data race detected by TSan (ThreadSanitizer).
* Specific information about this bug
The writing side of this race comes from CacheFile::OnFileOpened on CacheFile::mHandle. We are writing mHandle under a lock, but when we read from it on the main thread, we're not using a lock, so TSan complains.
I have a truly gross hack for this that I will post shortly. This is a frequently reported race, and it'd be great to get rid of it somehow.
* 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
Reporter | ||
Comment 1•9 years ago
|
||
This patch is gross, and I won't try to pretend otherwise. Ideally the comment
will explain everything about why we're doing things this way. Honza and I
discussed adding an nsAtomicRefPtr or some such construct, but I'm not sure
that's worth the trouble...yet.
Another possibility is to have the body of the function read:
CacheFileHandle* handle;
handle = reinterpret_cast<CacheFileHandle**>(&mHandle);
handle = mHandle;
if (!handle)
return false;
return handle->IsDoomed();
which at least gets rid of the duplication of logic.
Attachment #8659409 -
Flags: review?(michal.novotny)
Comment 2•9 years ago
|
||
Nathan, isn't this already discussed in a different bug 1121672? We have already discussed possible true solution privately in "atomic refptr?" thread (I've responded on it last time on 8/14/2015, no answer from you).
Flags: needinfo?(nfroyd)
Comment 3•9 years ago
|
||
Comment on attachment 8659409 [details] [diff] [review]
hack around TSan's race detection in CacheFile::IsDoomed
AFAICS this bug is a dupe of 1121672. See my comment #9 in that bug.
Attachment #8659409 -
Flags: review?(michal.novotny) → review-
Reporter | ||
Comment 4•9 years ago
|
||
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #2)
> Nathan, isn't this already discussed in a different bug 1121672? We have
> already discussed possible true solution privately in "atomic refptr?"
> thread (I've responded on it last time on 8/14/2015, no answer from you).
An AtomicRefPtr for global-scope variables seems like an additional complication (why not use a raw pointer instead?) and so far would be used solely for a few places in the network cache, which doesn't seem like a reusable, general-purpose thing.
If you would like to have an AtomicRefPtr in the network cache, you can certainly do that...
Flags: needinfo?(nfroyd)
Comment 5•9 years ago
|
||
(Correct me if I'm wrong)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•