Fix potential thread safety issue on ScriptProcessNdoe during fire onaudioprocess eventhandler.

Fix potential thread safety issue on ScriptProcessNdoe during fire onaudioprocess eventhandler.

m_isRequestOutstanding can be set on main thread and audio thread both.
It might cause a issue unexpectly since it is not safe by thread operation.
Added mutex for it.

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

[KhNo, 2014-03-26 06:10:33]

Could you review this patch :)

[Raymond Toy, 2014-03-26 17:42:32]

On 2014/03/26 06:10:33, KhNo wrote:
> Could you review this patch :)

I don't think this is necessary.

m_isRequestOutstanding is a bool so any reads and writes will be atomic; you
can't get a partially formed value. 

Is there a scenario where we will do the wrong thing if we don't have a lock?

[KhNo, 2014-03-27 01:07:21]

On 2014/03/26 17:42:32, Raymond Toy wrote:
> On 2014/03/26 06:10:33, KhNo wrote:
> > Could you review this patch :)
> 
> I don't think this is necessary.
> 
> m_isRequestOutstanding is a bool so any reads and writes will be atomic; you
> can't get a partially formed value. 
> 
> Is there a scenario where we will do the wrong thing if we don't have a lock?

As you know, many thread safety issues are very corner cases. I do not have
specific sample and case.
But, potential scenario is existed.
When .buffersize is minimum and a main thread is doing something heavy,
inputbuffer is quickly ready to fire next event.
My worry is from middle of
"dispatchEvent(AudioProcessingEvent::create(inputBuffer, outputBuffer));",
m_isRequestOutstanding was set already to false.
In audio thread can fire next patch since buffer is full and
m_isRequestOutstanding is good to fire.

Actually, this bug can be fixed simply switching below two line.

m_isRequestOutstanding = false;
dispatchEvent(AudioProcessingEvent::create(inputBuffer, outputBuffer));

But, I thought It was more general way.

[Raymond Toy, 2014-03-27 23:27:53]

On 2014/03/27 01:07:21, KhNo wrote:
> On 2014/03/26 17:42:32, Raymond Toy wrote:
> > On 2014/03/26 06:10:33, KhNo wrote:
> > > Could you review this patch :)
> > 
> > I don't think this is necessary.
> > 
> > m_isRequestOutstanding is a bool so any reads and writes will be atomic; you
> > can't get a partially formed value. 
> > 
> > Is there a scenario where we will do the wrong thing if we don't have a
lock?
> 
> As you know, many thread safety issues are very corner cases. I do not have
> specific sample and case.
> But, potential scenario is existed.
> When .buffersize is minimum and a main thread is doing something heavy,
> inputbuffer is quickly ready to fire next event.
> My worry is from middle of
> "dispatchEvent(AudioProcessingEvent::create(inputBuffer, outputBuffer));",
> m_isRequestOutstanding was set already to false.
> In audio thread can fire next patch since buffer is full and
> m_isRequestOutstanding is good to fire.
> 
> Actually, this bug can be fixed simply switching below two line.
> 
> m_isRequestOutstanding = false;
> dispatchEvent(AudioProcessingEvent::create(inputBuffer, outputBuffer));
> 
> But, I thought It was more general way.

Yes. Reading and writing the same variable from two different threads without
synchronization is bad.  Keep the lock.

But please test this on as many different apps that you can find that use a
script processor node.

[KhNo, 2014-03-28 09:33:31]

On 2014/03/27 23:27:53, Raymond Toy wrote:
> On 2014/03/27 01:07:21, KhNo wrote:
> > On 2014/03/26 17:42:32, Raymond Toy wrote:
> > > On 2014/03/26 06:10:33, KhNo wrote:
> > > > Could you review this patch :)
> > > 
> > > I don't think this is necessary.
> > > 
> > > m_isRequestOutstanding is a bool so any reads and writes will be atomic;
you
> > > can't get a partially formed value. 
> > > 
> > > Is there a scenario where we will do the wrong thing if we don't have a
> lock?
> > 
> > As you know, many thread safety issues are very corner cases. I do not have
> > specific sample and case.
> > But, potential scenario is existed.
> > When .buffersize is minimum and a main thread is doing something heavy,
> > inputbuffer is quickly ready to fire next event.
> > My worry is from middle of
> > "dispatchEvent(AudioProcessingEvent::create(inputBuffer, outputBuffer));",
> > m_isRequestOutstanding was set already to false.
> > In audio thread can fire next patch since buffer is full and
> > m_isRequestOutstanding is good to fire.
> > 
> > Actually, this bug can be fixed simply switching below two line.
> > 
> > m_isRequestOutstanding = false;
> > dispatchEvent(AudioProcessingEvent::create(inputBuffer, outputBuffer));
> > 
> > But, I thought It was more general way.
> 
> Yes. Reading and writing the same variable from two different threads without
> synchronization is bad.  Keep the lock.
> 
> But please test this on as many different apps that you can find that use a
> script processor node.

I have tested layout test and local my simple testing with min and max
buffersize.
Also well-known demo has been tested. Nothing was bad fortunately. :)

https://chromium.googlecode.com/svn/trunk/samples/audio/javascript-processing...
http://srchea.com/apps/sound-visualizer-web-audio-api-webgl/
http://webaudioapi.com/samples/script-processor/ (I have to change mp3 to other)

[Raymond Toy, 2014-03-28 16:17:36]

On 2014/03/28 09:33:31, KhNo wrote:
> On 2014/03/27 23:27:53, Raymond Toy wrote:
> > On 2014/03/27 01:07:21, KhNo wrote:
> > > On 2014/03/26 17:42:32, Raymond Toy wrote:
> > > > On 2014/03/26 06:10:33, KhNo wrote:
> > > > > Could you review this patch :)
> > > > 
> > > > I don't think this is necessary.
> > > > 
> > > > m_isRequestOutstanding is a bool so any reads and writes will be atomic;
> you
> > > > can't get a partially formed value. 
> > > > 
> > > > Is there a scenario where we will do the wrong thing if we don't have a
> > lock?
> > > 
> > > As you know, many thread safety issues are very corner cases. I do not
have
> > > specific sample and case.
> > > But, potential scenario is existed.
> > > When .buffersize is minimum and a main thread is doing something heavy,
> > > inputbuffer is quickly ready to fire next event.
> > > My worry is from middle of
> > > "dispatchEvent(AudioProcessingEvent::create(inputBuffer, outputBuffer));",
> > > m_isRequestOutstanding was set already to false.
> > > In audio thread can fire next patch since buffer is full and
> > > m_isRequestOutstanding is good to fire.
> > > 
> > > Actually, this bug can be fixed simply switching below two line.
> > > 
> > > m_isRequestOutstanding = false;
> > > dispatchEvent(AudioProcessingEvent::create(inputBuffer, outputBuffer));
> > > 
> > > But, I thought It was more general way.
> > 
> > Yes. Reading and writing the same variable from two different threads
without
> > synchronization is bad.  Keep the lock.
> > 
> > But please test this on as many different apps that you can find that use a
> > script processor node.
> 
> I have tested layout test and local my simple testing with min and max
> buffersize.
> Also well-known demo has been tested. Nothing was bad fortunately. :)
> 
>
https://chromium.googlecode.com/svn/trunk/samples/audio/javascript-processing...
> http://srchea.com/apps/sound-visualizer-web-audio-api-webgl/
> http://webaudioapi.com/samples/script-processor/ (I have to change mp3 to
other)

lgtm.

I think my permissions for this are fixed today; if they're not try again
tomorrow.

[KhNo, 2014-03-29 00:07:04]

The CQ bit was checked by keonho07.kim@samsung.com

[commit-bot: I haz the power, 2014-03-29 00:07:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keonho07.kim@samsung.com/212183002/1

[KhNo, 2014-03-29 00:10:28]

On 2014/03/28 16:17:36, Raymond Toy wrote:
> On 2014/03/28 09:33:31, KhNo wrote:
> > On 2014/03/27 23:27:53, Raymond Toy wrote:
> > > On 2014/03/27 01:07:21, KhNo wrote:
> > > > On 2014/03/26 17:42:32, Raymond Toy wrote:
> > > > > On 2014/03/26 06:10:33, KhNo wrote:
> > > > > > Could you review this patch :)
> > > > > 
> > > > > I don't think this is necessary.
> > > > > 
> > > > > m_isRequestOutstanding is a bool so any reads and writes will be
atomic;
> > you
> > > > > can't get a partially formed value. 
> > > > > 
> > > > > Is there a scenario where we will do the wrong thing if we don't have
a
> > > lock?
> > > > 
> > > > As you know, many thread safety issues are very corner cases. I do not
> have
> > > > specific sample and case.
> > > > But, potential scenario is existed.
> > > > When .buffersize is minimum and a main thread is doing something heavy,
> > > > inputbuffer is quickly ready to fire next event.
> > > > My worry is from middle of
> > > > "dispatchEvent(AudioProcessingEvent::create(inputBuffer,
outputBuffer));",
> > > > m_isRequestOutstanding was set already to false.
> > > > In audio thread can fire next patch since buffer is full and
> > > > m_isRequestOutstanding is good to fire.
> > > > 
> > > > Actually, this bug can be fixed simply switching below two line.
> > > > 
> > > > m_isRequestOutstanding = false;
> > > > dispatchEvent(AudioProcessingEvent::create(inputBuffer, outputBuffer));
> > > > 
> > > > But, I thought It was more general way.
> > > 
> > > Yes. Reading and writing the same variable from two different threads
> without
> > > synchronization is bad.  Keep the lock.
> > > 
> > > But please test this on as many different apps that you can find that use
a
> > > script processor node.
> > 
> > I have tested layout test and local my simple testing with min and max
> > buffersize.
> > Also well-known demo has been tested. Nothing was bad fortunately. :)
> > 
> >
>
https://chromium.googlecode.com/svn/trunk/samples/audio/javascript-processing...
> > http://srchea.com/apps/sound-visualizer-web-audio-api-webgl/
> > http://webaudioapi.com/samples/script-processor/ (I have to change mp3 to
> other)
> 
> lgtm.
> 
> I think my permissions for this are fixed today; if they're not try again
> tomorrow.

Oh, nice! Thanks for review.

[commit-bot: I haz the power, 2014-03-29 01:19:53]

Change committed as 170395