swap the renderer_ reference to local before calling Stop()

swap the renderer_ reference to local before calling Stop(), otherwise we will invalidate the pointer during the Stop() call.

The problem that this CL is trying to fix:
In WebRtcAudioDeviceImpl::Terminate(), when the renderer_ is valid and we call renderer_->Stop(), this Stop() function will trigger a callback till WebRtcAudioDeviceImpl::RemoveAudioRenderer() where we set the renderer_ to NULL. This will crash chrome since renderer_ is being used at the same time.


BUG=241430
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202420

[henrika (OOO until Aug 14), 2013-05-27 10:06:00]

https://codereview.chromium.org/15732021/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/15732021/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_impl.cc:262: //
WebRtcAudioRenderer::Stop() will trigger RemoveAudioRenderer(), so we
I am stupid, but the example says:

{
//     scoped_refptr<MyFoo> a = new MyFoo();
//     scoped_refptr<MyFoo> b;
//
//     b.swap(a);
//     // now, |b| references the MyFoo object, and |a| references NULL.
//   }

[henrika (OOO until Aug 14), 2013-05-27 10:11:21]

https://codereview.chromium.org/15732021/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/15732021/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_impl.cc:262: //
WebRtcAudioRenderer::Stop() will trigger RemoveAudioRenderer(), so we
OK, checking the swap implementation it looks like:

renderer_ will be NULL, and
renderer will be set to valid renderer_

but it is still not clear what that solves.

[henrika (OOO until Aug 14), 2013-05-27 10:14:45]

OK, see the bug now but any idea why we see this now?

[henrika (OOO until Aug 14), 2013-05-27 10:29:47]

LGTM if you improve the comment in the code explaining why you have to do this.

Also, I would chat with Tommi off-line to see if he has alternative ideas. The
whole design feels a bit shaky IMO.

[no longer working on chromium, 2013-05-27 10:34:38]

+ Tommi since Henrik is chatting with him about it.

Tommi, could you please take a look at the CL as well.

Thanks
SX

[henrika (OOO until Aug 14), 2013-05-27 10:36:18]

I meant, please check with Tommi just in case.


On Mon, May 27, 2013 at 12:34 PM, <xians@chromium.org> wrote:

> + Tommi since Henrik is chatting with him about it.
>
> Tommi, could you please take a look at the CL as well.
>
> Thanks
> SX
>
>
https://codereview.chromium.**org/15732021/<https://codereview.chromium.org/1...
>

[tommi (sloooow) - chröme, 2013-05-27 10:48:13]

https://codereview.chromium.org/15732021/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/15732021/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_impl.cc:265:
renderer_.swap(renderer);
I don't think swap() is the right thing here because it explicitly sets
renderer_ to NULL even before RemoveAudioRenderer() is called.  If you run the
code (and assuming that RemoveAudioRenderer() is called as you say), then you
should hit a DCHECK.

Instead, what about doing this instead:

if (renderer_) {
  // Grab a local reference while we call Stop().
  scoped_refptr<WebRtcAudioRenderer> local_renderer(renderer_);
  local_renderer->Stop();
  // The above call to Stop() should have triggered a call to
RemoveAudioRenderer
  // which clears our reference to the audio renderer.
  DCHECK(!renderer_.get());
}

[no longer working on chromium, 2013-05-27 11:15:41]

Good catch, I missed the DCHECK in the RemoveAudioRenderer().

Tommi, ptal.

Thanks,
SX

https://codereview.chromium.org/15732021/diff/2001/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://codereview.chromium.org/15732021/diff/2001/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_impl.cc:265:
renderer_.swap(renderer);
On 2013/05/27 10:48:13, tommi wrote:
> I don't think swap() is the right thing here because it explicitly sets
> renderer_ to NULL even before RemoveAudioRenderer() is called.  If you run the
> code (and assuming that RemoveAudioRenderer() is called as you say), then you
> should hit a DCHECK.
> 
> Instead, what about doing this instead:
> 
> if (renderer_) {
>   // Grab a local reference while we call Stop().
>   scoped_refptr<WebRtcAudioRenderer> local_renderer(renderer_);
>   local_renderer->Stop();
>   // The above call to Stop() should have triggered a call to
> RemoveAudioRenderer
>   // which clears our reference to the audio renderer.
>   DCHECK(!renderer_.get());
> }

Done.

[tommi (sloooow) - chröme, 2013-05-27 11:22:35]

lgtm

[commit-bot: I haz the power, 2013-05-27 11:23:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/15732021/10001

[commit-bot: I haz the power, 2013-05-27 14:02:08]

Change committed as 202420