Fix threading races on HRTFElevation::audioBusMap

Fix threading races on HRTFElevation::audioBusMap

According to the crash report (https://cluster-fuzz.appspot.com/testcase?key=6291334197411840),
there is a threading race in HRTFElevation::getConcatenatedImpulseResponsesForSubject.

static PassRefPtr<AudioBus> getConcatenatedImpulseResponsesForSubject(...) {
    typedef HashMap<String, RefPtr<AudioBus> > AudioBusMap;
    DEFINE_STATIC_LOCAL(AudioBusMap, audioBusMap, ());
    RefPtr<AudioBus> bus;
    AudioBusMap::iterator iterator = audioBusMap.find(subjectName); // (A)
    if (iterator == audioBusMap.end()) {
        ...;
        audioBusMap.set(subjectName, bus);  // (B)
    }
}

It's possible that:

(1) Thread 1 executes (A)
(2) Thread 2 executes (A)
(3) Thread 1 executes (B)
(4) Thread 2 executes (B) and crashes.

This CL protects accesses to the AudioBusMap with mutex.

BUG=270758
No tests because the crash depends on threading races and thus not reproducible.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157273

[haraken, 2013-09-04 00:49:07]

kbr: would you take a look at this one too? :)

[haraken, 2013-09-04 19:51:17]

kbr: ping?

[Ken Russell (switch to Gerrit), 2013-09-04 19:57:55]

https://codereview.chromium.org/23613007/diff/7001/Source/core/platform/audio...
File Source/core/platform/audio/HRTFElevation.cpp (right):

https://codereview.chromium.org/23613007/diff/7001/Source/core/platform/audio...
Source/core/platform/audio/HRTFElevation.cpp:68: DEFINE_STATIC_LOCAL(Mutex,
mutex, ());
This doesn't look any safer to me. If there is really a race then two threads
could plausibly call this function the first time and race for the
initialization of these static locals.

[haraken, 2013-09-04 20:05:24]

On 2013/09/04 19:57:55, Ken Russell wrote:
>
https://codereview.chromium.org/23613007/diff/7001/Source/core/platform/audio...
> File Source/core/platform/audio/HRTFElevation.cpp (right):
> 
>
https://codereview.chromium.org/23613007/diff/7001/Source/core/platform/audio...
> Source/core/platform/audio/HRTFElevation.cpp:68: DEFINE_STATIC_LOCAL(Mutex,
> mutex, ());
> This doesn't look any safer to me. If there is really a race then two threads
> could plausibly call this function the first time and race for the
> initialization of these static locals.

Yeah, right. The same problem is already happening for
DEFINE_STATIC_LOCAL(AudioBusMap, audioBusMap, ()). Do you have any idea for
correctly initializing the data?

[Ken Russell (switch to Gerrit), 2013-09-05 00:11:39]

On 2013/09/04 20:05:24, haraken wrote:
> On 2013/09/04 19:57:55, Ken Russell wrote:
> >
>
https://codereview.chromium.org/23613007/diff/7001/Source/core/platform/audio...
> > File Source/core/platform/audio/HRTFElevation.cpp (right):
> > 
> >
>
https://codereview.chromium.org/23613007/diff/7001/Source/core/platform/audio...
> > Source/core/platform/audio/HRTFElevation.cpp:68: DEFINE_STATIC_LOCAL(Mutex,
> > mutex, ());
> > This doesn't look any safer to me. If there is really a race then two
threads
> > could plausibly call this function the first time and race for the
> > initialization of these static locals.
> 
> Yeah, right. The same problem is already happening for
> DEFINE_STATIC_LOCAL(AudioBusMap, audioBusMap, ()). Do you have any idea for
> correctly initializing the data?

Looking elsewhere in the code base, it looks like defining a static local mutex
is a common idiom, and it looks like there's spec text at least in C++0x that
makes guarantees about concurrent initialization of function static variables.
It looks like my concern is unwarranted.

I'm a little concerned about putting a mutex around the expensive call to
AudioBus::loadPlatformResource, but it looks like this will only block the HRTF
database loader thread. LGTM therefore. Please get Raymond's review before
committing.

[haraken, 2013-09-05 00:17:09]

> Looking elsewhere in the code base, it looks like defining a static local
mutex
> is a common idiom, and it looks like there's spec text at least in C++0x that
> makes guarantees about concurrent initialization of function static variables.

Oh, that's good news. Thanks for the info!

> Please get Raymond's review before committing.

rtoy: would you take a look?

[Raymond Toy, 2013-09-05 02:13:20]

On 2013/09/05 00:17:09, haraken wrote:
> > Looking elsewhere in the code base, it looks like defining a static local
> mutex
> > is a common idiom, and it looks like there's spec text at least in C++0x
that
> > makes guarantees about concurrent initialization of function static
variables.
> 
> Oh, that's good news. Thanks for the info!
> 
> > Please get Raymond's review before committing.
> 
> rtoy: would you take a look?

lgtm.

I think it's ok to have a mutex here.  This code is only run at start up of an
AudioContext.  On an early version of WebAudio on Android, loading the database
took 5 sec or so.  This just meant that no audio could be produced for 5 sec
until the database was loaded.

[haraken, 2013-09-05 02:26:14]

Thanks for reviewing!

[commit-bot: I haz the power, 2013-09-05 02:26:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/23613007/7001

[commit-bot: I haz the power, 2013-09-05 04:59:21]

Change committed as 157273

[kcc1, 2013-09-05 06:43:50]

>> No tests because the crash depends on threading races and thus not
reproducible.

We have ThreadSanitizer, a data race detector. 
If you add a test that used to have a race, 
our ThreadSanitizer bots would find races if they appear again.

[haraken, 2013-09-05 06:48:15]

On 2013/09/05 06:43:50, kcc1 wrote:
> >> No tests because the crash depends on threading races and thus not
> reproducible.
> 
> We have ThreadSanitizer, a data race detector. 
> If you add a test that used to have a race, 
> our ThreadSanitizer bots would find races if they appear again.

The race happens just if the Audio thread calls
getConcatenatedImpulseResponsesForSubject(). I think it should be already
happening in some tests in webaudio/... Will take a look.