1647378 - ThreadSanitizer: data race [@ mozilla::scache::StartupCache::MaybeWriteOffMainThread] vs. [@ malloc]

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Jason Kratzer [:jkratzer]]

The attached crash information was detected while running CI tests with ThreadSanitizer on mozilla-central revision 20200617-fa0afb432810.

For detailed crash information, see attachment.

General information about TSan reports

Why fix races?

Data races are undefined behavior and can cause crashes as well as correctness issues. Compiler optimizations can cause racy code to have unpredictable and hard-to-reproduce behavior.

Rating

If you think this race can cause crashes or correctness issues, it would be great to rate the bug appropriately as P1/P2 and/or indicating this in the bug. This makes it a lot easier for us to assess the actual impact that these reports make and if they are helpful to you.

False Positives / Benign Races

Typically, races reported by TSan are not false positives [1], but it is possible that the race is benign. Even in this case it would be nice to come up with a fix if it is easily doable and does not regress performance. Every race that we cannot fix will have to remain on the suppression list and slows down the overall TSan performance. Also note that seemingly benign races can possibly be harmful (also depending on the compiler, optimizations and the architecture) [2][3].

[1] One major exception is the involvement of uninstrumented code from third-party libraries.
[2] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
[3] How to miscompile programs with "benign" data races: https://www.usenix.org/legacy/events/hotpar11/tech/final_files/Boehm.pdf

Suppressing unfixable races

If the bug cannot be fixed, then a runtime suppression needs to be added in mozglue/build/TsanOptions.cpp. The suppressions match on the full stack, so it should be picked such that it is unique to this particular race. The bug number of this bug should also be included so we have some documentation on why this suppression was added.

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: Detailed Crash Information

Comment 2

[Andrew McCreight [:mccr8]]

Possibly related to bug 1614795?

Comment 3

[:Gijs (he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #2)

Possibly related to bug 1614795?

Yeah, this is related. We only read the member in one place - in bool StartupCache::StartupWriteComplete() -- which is only used in tests. My understanding is that the race's bad effects are going to be relating to overwriting "bad" values in the affected member, or mis-reading the value, both of which will only matter in the gtest. But the relevant gtest's set-up means that is very unlikely. So I don't think this is a problem in practice.

I'd be happy to fix this though, if that's easier than suppressing the error? Though it would not be desirable to add perf impact to this bool that we only care about for tests. I'd sooner use a different condition in the gtest somehow. Doug, thoughts?

---

For some background that I wrote up before I realized we don't care about the value:

We try to guard access here using a mutex, mTableLock. Specifically:

https://searchfox.org/mozilla-central/rev/3d39d3b7dd1b2be30692d4541ea681614e34c786/startupcache/StartupCache.cpp#750-751

gains the lock before we call WriteToDisk on the bg IO thread:

  nsCOMPtr<nsIRunnable> runnable =
      NS_NewRunnableFunction("StartupCache::Write", [self]() mutable {
        MutexAutoLock unlock(self->mTableLock);
        auto result = self->WriteToDisk();
        Unused << NS_WARN_IF(result.isErr());
      });
  NS_DispatchBackgroundTask(runnable.forget(), NS_DISPATCH_EVENT_MAY_BLOCK);

On the main thread, there are 5 other cases where we write to mStartupWriteInitiated:

• in the constructor;
• in ResetStartupTimer(), which has one callsite in PutBuffer, which is guarded by the lock;
• in EnsureShutdownWriteComplete() which is guarded directly by the lock in that method;
• in WriteToDisk() which is flagged up here as used off-main-thread, but can also be used on the main thread - we assert we own the lock just above it, and indeed both callers (EnsureShutdownWriteComplete and InvalidateCache) gain the lock before calling it.
• in MaybeWriteOffMainThread() which is the one we're talking about here.

The MaybeWriteOffMainThread() case should be benign because it overwrites the value with false. The only case that sets it to true is if we run through WriteToDisk. We can only hit the resetting code in MaybeWriteOffMainThread if mWrittenOnce is false, and if some other stuff about the internal state is also true. So the only time this can race (ie change the value of the member mStartupWriteInitiated) is if we either:

• completed calling WriteToDisk off-main-thread (but didn't set mWrittenOnce, e.g. due to the explicit early return in WriteToDisk or any of the other MOZ_TRY bits returning with an error.
• are still waiting for WriteToDisk to complete off-main-thread.

In both cases, we will now fire another task off to the background thread to do a write. But that task won't touch anything until it gains the relevant lock (see above). At which point it will once again set mStartupWriteInitiated to true. In theory, that pattern could repeat indefinitely (ie do a write, a timer fires that resets mStartupWriteInitiated, we do another write, etc.). In practice, mWrittenOnce will get set, and the timers are 60s timers and would require new data to come in to the startup cache to get written, so it seems exceedingly unlikely to happen in practice.

Comment 4

[Alex Thayer [:alexical] (she/her)]

Is the fix as trivial as moving mTableLock.TryLock() to before the read on mStartupWriteInitiated? I.e:

  if (!mTableLock.TryLock()) {
    return false;
  }
  if (!mStartupWriteInitiated || mDirty) {
    mTableLock.Unlock();
    return false;
  }

Edit: Oh, ignore me. No, because there would be the data race in writing it.

Comment 5

[Alex Thayer [:alexical] (she/her)]

Honestly just taking the trivial perf hit of making the bool an atomic seems fine to me. The performance scale we're dealing here does not reasonably care about the difference of an atomic vs vanilla value, no?

Comment 6

[:Gijs (he/him)]

(In reply to Doug Thayer [:dthayer] from comment #5)

Honestly just taking the trivial perf hit of making the bool an atomic seems fine to me. The performance scale we're dealing here does not reasonably care about the difference of an atomic vs vanilla value, no?

WFM!

Comment 7

[:Gijs (he/him)]

Attachment: Bug 1647378 - stop using mStartupWriteInitiated in the gtest for startupcache, r?dthayer

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7454b5ac3e9
stop using mStartupWriteInitiated in the gtest for startupcache, r=dthayer

Comment 9

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/a7454b5ac3e9