1601717 - ThreadSanitizer: data race [@ mozilla::dom::ProgressEvent_Binding::CreateInterfaceObjects] vs. [@ mozilla::dom::ProgressEvent_Binding::CreateInterfaceObjects]

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), defect, P3)

Description

[Christian Holler (:decoder)]

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

I looked into this a bit and the code in question looks like this:

  static bool sIdsInited = false;
  if (!sIdsInited && NS_IsMainThread()) {
    if (!InitIds(aCx, sNativeProperties.Upcast())) {
      return;
    }
    sIdsInited = true;
  }

The problem is that the outer if checks sIdsInited before checking that it is actually the main thread and not vice-versa. I think this is harmless, but the race is pretty frequent and this code is present in almost all generated IPC bindings. I can check if switching the predicates fixes the problem. Is there any concern in terms of performance to swap these?

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

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Boris Zbarsky [:bzbarsky]]

Is there any concern in terms of performance to swap these

Unfortunately, yes. The boolean check is much faster than NS_IsMainThread()...

Comment 3

[Christian Holler (:decoder)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)

Is there any concern in terms of performance to swap these

Unfortunately, yes. The boolean check is much faster than NS_IsMainThread()...

Yea, I feared it was this way. I guess we have multiple options here:

1. Make the bool atomic. I am not sure how this would affect performance, but it is well possible that it is negligible.

2. Rewrite the code under ifdef MOZ_TSAN to produce different (race-free) code just for ThreadSanitizer.

3. Suppress the race.

Normally I would simply go for 3) but this race is pretty frequent on tests already and might justify to go for 2) if 1) is noticeable in terms of performance. Each time a race is suppressed it has to be symbolized and matched against the suppression list, draining TSan performance.

What do you think Boris?

Comment 4

[Nathan Froyd [:froydnj]]

Making it Atomic<bool, Relaxed> should work just fine with minimal to no performance impact.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I expect making this an atomic bool, if it solves this problem, is just fine perf-wise. If we're landing in this code at all, we're about to start allocating a bunch of JS objects, which almost certainly dwarfs an atomic access.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1601717. Avoid Tsan warnings about unsychronized access to our "have we inited the Xray ids?" boolean. r=edgar

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9114238 [details]
Bug 1601717. Avoid Tsan warnings about unsychronized access to our "have we inited the Xray ids?" boolean. r=edgar

Christian, does this address the issue on your end?

Comment 8

[Christian Holler (:decoder)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #7)

Comment on attachment 9114238 [details]
Bug 1601717. Avoid Tsan warnings about unsychronized access to our "have we inited the Xray ids?" boolean. r=edgar

Christian, does this address the issue on your end?

Yes, this works just fine. Thanks for the quick response :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f94f996f9ff2
Avoid Tsan warnings about unsychronized access to our "have we inited the Xray ids?" boolean.  r=edgar

Comment 10

[Narcis Beleuzu [:NarcisB]]

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