1190951 - TSan: data race netwerk/cache2/CacheIndex.cpp:1397 CacheIndex::IsUpToDate

Status: RESOLVED FIXED

(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

This race is complaining about the initialization of CacheIndex::gInstance on one thread (the main thread) during startup and then accessing gInstance on the cache thread at some later time.

Is there a reason that we can't initialize the index on the actual cache thread?  Making the problem go away via relaxed atomics is certainly doable, but it seems like there ought to be a better solution.

* 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

[Julian Seward [:jseward]]

STR: apply patch from 1222043, then:

TSAN_OPTIONS=suppressions=/home/sewardj/MOZ/SUPPS/tsan-empty.txt DISPLAY=:1.0 \
   ./mach xpcshell-test --sequential --log-mach - \
   browser/components/dirprovider/tests/unit/test_bookmark_pref.js 2>&1 

This is currently the 3rd most common race when running the xpcshell tests,
accounting for 1281 out of a total of 12343 race summaries from TSan.

Comment 2

[Julian Seward [:jseward]]

The problem's root is in CacheObserver::Observe().  It has this:

  if (!strcmp(aTopic, "profile-do-change")) {
    AttachToPreferences();
    CacheFileIOManager::Init();
    CacheFileIOManager::OnProfile();
    return NS_OK;
  }

CacheFileIOManager::Init() creates the child thread, which goes on to read gInstance.
The parent thread continues with CacheFileIOManager::OnProfile(), which writes gInstance,
hence the race.

Comment 3

[Julian Seward [:jseward]]

I spent some time looking at whether it is possible to refactor the two
methods CacheFileIOManager::Init and CacheFileIOManager::OnProfile, so
that gInstance is initialised before the child thread is created, but with
zero knowledge of the code it's hard to figure out.

Any better suggestions?

Comment 4

[Michal Novotny [:michal]]

I'll have a look at it.

Comment 5

[Michal Novotny [:michal]]

(In reply to Julian Seward [:jseward] from comment #2)
> The problem's root is in CacheObserver::Observe().  It has this:
> 
>   if (!strcmp(aTopic, "profile-do-change")) {
>     AttachToPreferences();
>     CacheFileIOManager::Init();
>     CacheFileIOManager::OnProfile();
>     return NS_OK;
>   }
> 
> CacheFileIOManager::Init() creates the child thread, which goes on to read
> gInstance.
> The parent thread continues with CacheFileIOManager::OnProfile(), which
> writes gInstance,
> hence the race.

This comment belongs probably to some other bug. This bug is about CacheIndex::gInstance race and not CacheFileIOManager::gInstance.

Comment 6

[Michal Novotny [:michal]]

Attachment: fix

We used to check CacheIndex::gInstance to find out whether the service is initialized and can be used (including it's lock). Now the lock is static and protects the access to gInstance.

Something similar is probably needed in CacheFileIOManager too.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=baa8a3802f2f

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ab5b8056202

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6ab5b8056202