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

Status: RESOLVED DUPLICATE of bug 1121672

(Core :: Networking: Cache, defect)

Description

[Nathan Froyd [:froydnj]]

Attachment: 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

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: 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.

Comment 2

[Honza Bambas (:mayhemer)]

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).

Comment 3

[Michal Novotny [:michal]]

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.

Comment 4

[Nathan Froyd [:froydnj]]

(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...

Comment 5

[Honza Bambas (:mayhemer)]

(Correct me if I'm wrong)