Fix heap-use-after-free issue with WebAudioCapturerSource.

Fix heap-use-after-free issue with WebAudioCapturerSource.

WebAudioCapturerSource registers with a blink WebMediaStreamSource.
When the audio track was stopped, the WebAudioCapturerSource was
destroyed and the WebMediaStreamSource was left with a dangling
pointer, which it tried to use, resulting in access to freed
memory and usually a crashed tab.

This CL makes WebAudioCapturerSource aware of the WebMediaStreamSource
with which it is registered, so that it can be deregistered when the
audio track is stopped.

BUG=473253
TEST=See testcase.html in crbug.com/473253

Committed: https://crrev.com/228cd9447121ede4d32ab48c8dfe066736cfdae2
Cr-Commit-Position: refs/heads/master@{#324622}

[Guido Urdaneta, 2015-04-09 11:28:48]

guidou@chromium.org changed reviewers:
+ henrika@chromium.org, perkj@chromium.org

[henrika (OOO until Aug 14), 2015-04-09 11:48:22]

henrika@chromium.org changed reviewers:
+ rtoy@chromium.org

[henrika (OOO until Aug 14), 2015-04-09 11:53:40]

Nice work Guido. Some initial comments.

https://codereview.chromium.org/1071063005/diff/1/content/renderer/media/weba...
File content/renderer/media/webaudio_capturer_source.cc (right):

https://codereview.chromium.org/1071063005/diff/1/content/renderer/media/weba...
content/renderer/media/webaudio_capturer_source.cc:123: void
WebAudioCapturerSource::removeFromBlinkSource() {
Can you add some comments about what happens here exactly. And why it is needed.

https://codereview.chromium.org/1071063005/diff/1/content/renderer/media/weba...
File content/renderer/media/webaudio_capturer_source.h (right):

https://codereview.chromium.org/1071063005/diff/1/content/renderer/media/weba...
content/renderer/media/webaudio_capturer_source.h:58: // This method removes
this object from a blink::WebMediaStreamSource with
Nit, Removes this object...

[Guido Urdaneta, 2015-04-09 13:10:54]

Note that I added a private method to WebAudioCapturerSource to deregister the
WebAudioCapturerSource from blink, but I didn't add any method to register it
with the blink object.
I changed the constructor so that it receives a reference to the blink object,
but I left the registration exactly as it was in
PeerConnectionDependencyFactory::CreateWebAudioSource().
I did it that way in order to alter the existing code as little as possible. 

Do you think guys think this approach is OK, or would it be better to approach
it differently? For example, having a public method addToBlinkSource() that
takes the blink source and adds the WebAudioCapturerSource to it instead of
doing the registration in
PeerConnectionDependencyFactory::CreateWebAudioSource().

[henrika (OOO until Aug 14), 2015-04-09 13:16:34]

I think perkj@ is more suitable to answer your question. If your version solves
the problem I am fine as is.

LGTM

[perkj_chrome, 2015-04-09 14:55:49]

https://codereview.chromium.org/1071063005/diff/60001/content/renderer/media/...
File content/renderer/media/webaudio_capturer_source.cc (right):

https://codereview.chromium.org/1071063005/diff/60001/content/renderer/media/...
content/renderer/media/webaudio_capturer_source.cc:30: removeFromBlinkSource();
On what thread is this object destroyed? I don't think you can call
removeAudioConsumer(this) from other threads than the main thread.

If it is only ever just one thread add  
DCHECK(thread_checker_.CalledOnValidThread());

[Guido Urdaneta, 2015-04-10 10:56:38]

On 2015/04/09 14:55:49, perkj wrote:
>
https://codereview.chromium.org/1071063005/diff/60001/content/renderer/media/...
> File content/renderer/media/webaudio_capturer_source.cc (right):
> 
>
https://codereview.chromium.org/1071063005/diff/60001/content/renderer/media/...
> content/renderer/media/webaudio_capturer_source.cc:30:
removeFromBlinkSource();
> On what thread is this object destroyed? I don't think you can call
> removeAudioConsumer(this) from other threads than the main thread.
> 
> If it is only ever just one thread add  
> DCHECK(thread_checker_.CalledOnValidThread());

I checked destruction via the JavaScript GC and it occurs on the main thread
(just like explicitly calling the stop() JavaScript method) so the DCHECK works.
Moreover, in the GC case, the object is destroyed anyway by a call to the Stop()
method. Stop() in this case is invoked by the destructor of
WebRtcLocalAudioTrack, which owns the WebAudioCapturerSource. That destructor
also has a DCHECK to ensure execution in the main thread.

In a separate patch I also made WebAudioCapturerSource a scoped_ptr instead of a
scoped_refptr. It seems to work, but it's not really necessary to fix this bug.
Should I integrate it in this patch?

[perkj_chrome, 2015-04-10 12:31:55]

thanks
lgtm
I think you should keep the cls separate.

[Guido Urdaneta, 2015-04-10 12:57:22]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2015-04-10 12:57:22]

The patchset sent to the CQ was uploaded after l-g-t-m from henrika@chromium.org
Link to the patchset: https://codereview.chromium.org/1071063005/#ps80001
(title: "Add thread check to WebAudioCapturerSourcer destructor")

[commit-bot: I haz the power, 2015-04-10 12:57:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1071063005/80001

[commit-bot: I haz the power, 2015-04-10 13:00:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-10 13:01:39]

Patchset 5 (id:??) landed as
https://crrev.com/228cd9447121ede4d32ab48c8dfe066736cfdae2
Cr-Commit-Position: refs/heads/master@{#324622}