bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

TSan: data race netwerk/cache2/CacheFile.cpp:1723 IsDoomed (race on mHandle)

RESOLVED DUPLICATE of bug 1121672

Status

()

Core
Networking: Cache
RESOLVED DUPLICATE of bug 1121672
3 years ago
3 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tsan])

Attachments

(2 attachments)

Created attachment 8659406 [details]
TSan stack trace

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
Created attachment 8659409 [details] [diff] [review]
hack around TSan's race detection in CacheFile::IsDoomed

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)
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 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-
(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)
(Correct me if I'm wrong)
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1121672
You need to log in before you can comment on or make changes to this bug.