845357 - TSan: Thread data race in ccapp_get_state vs. CCAppInit

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: trace

During initial tests with ThreadSanitizer (LLVM version), we get a data race reported as described in the attached log. Trace was created on mozilla-central with changeset 122820:c233837cce08.

According to the TSan devs, most of the reported traces should be real data races, even though they can be "benign". We need to determine if the race can/should be fixed, or put on the ignore list. Even for benign races, TSan devs suggest to fix them (second priority), as they can also cause problems [1].

[1] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Looks like a simple date race where a memory barrier is needed after updating the state variable, or depending on if it's set to other values elsewhere it may need mutex locking.

In practice this is highly unlikely to cause problems, but that's hard to prove.

Comment 2

[Suhas]

Attachment: Patch to order reads/writes of CCApp state between threads

Took a stab at what might order the reads/writes of CCApp state races.

Comment 3

[Ethan Hugg [:ehugg]]

Comment on attachment 731077 [details] [diff] [review]
Patch to order reads/writes of CCApp state between threads


Suhas, this patch looks a bit odd to me it doesn't have the usual header with name and date like this:
# HG changeset patch
# User Ethan Hugg <[e-mail]>
# Date 1353004707 28800

And it also has Windows-style CRLFs throughout which means folks will get patching errors.  Did you use bzexport to upload this patch?

Comment 4

[Suhas]

Hi Ethan .. My bzexport is giving trouble .Hence i had to upload it manually. Let me give it another try.

Comment 5

[Suhas]

Attachment: Patch to order writes/reads of CCApp state between threads

Uploading hg export way this time untill my bzexport issue gets fixed.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 731219 [details]
Patch to order writes/reads of CCApp state between threads

Review of attachment 731219 [details]:
-----------------------------------------------------------------

r+; I understand ethan has some nits to address.  I can land if needed over the weekend once they're addressed

Comment 7

[Suhas]

thanks jesup .. Once i hear from Ethan, I shall upload a new version based for you to check in.

Comment 8

[Ethan Hugg [:ehugg]]

Comment on attachment 731219 [details]
Patch to order writes/reads of CCApp state between threads

Review of attachment 731219 [details]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c
@@ +179,5 @@
> +// Sets up mutex needed for protecting state variables.
> +static cc_int32_t InitInternal();
> +
> +// Handy macro for executing memory barrier on CCApp State updates.
> +#define ENTER_MEMORY_BARRIER \

I think this macro name is a bit too generic.  I would suggest getting CCAPPSTATE in there like ENTER_CCAPPSTATE_PROTECT for example.

@@ +314,3 @@
>  cc_reg_state_t ccapp_get_state() {
> +   // wait till the ongoing modification is
> +   // completed

I believe we have been putting /* */ in C code and // in C++ code.

@@ +344,5 @@
> +  }
> +  // Bug 845357
> +  ENTER_MEMORY_BARRIER;
> +  gCCApp.state = CC_CREATED_IDLE;
> +  canReadCCAppState = 1;

Is this needed? This is also set to 1 in EXIT_MEMORY_BARRIER

@@ +1241,2 @@
>          gCCApp.state = CC_OOS_FAILOVER;
> +        EXIT_MEMORY_BARRIER;

This pattern repeats a lot, how about a static function ccapp_set_state()?

Comment 9

[Suhas]

Attachment: Patch to order writes/reads of CCApp state between threads

Take r+ forward from Ethan and Randell with comments addressed.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c4acb0bd10d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c4acb0bd10d