Fixed the audio backgrounding bug

content/browser/renderer_host/media/audio_renderer_host.cc

Index: content/browser/renderer_host/media/audio_renderer_host.cc
diff --git a/content/browser/renderer_host/media/audio_renderer_host.cc b/content/browser/renderer_host/media/audio_renderer_host.cc
index dddeee21626a31f30aff5b4f749c396fcd5024b8..9805fc57b229a1f9d6457e892b5b3bfa550829fc 100644
--- a/content/browser/renderer_host/media/audio_renderer_host.cc
+++ b/content/browser/renderer_host/media/audio_renderer_host.cc
@@ -651,11 +651,32 @@ void AudioRendererHost::UpdateNumPlayingStreams(AudioEntry* entry,
         !RenderFrameHasActiveAudio(entry->render_frame_id());
     entry->set_playing(true);
     base::AtomicRefCountInc(&num_playing_streams_);
+
+    // If it's the first audio stream to start for this renderer.

[DaleCurtis, 2015/06/29 22:44:23]

Comment doesn't add much value, explain why instead of just transliterating the
conditional.

[sebsg, 2015/07/02 12:58:08]

Done.

+    if (base::AtomicRefCountIsOne(&num_playing_streams_)) {
+      RenderProcessHost* render_process_host =
+          RenderProcessHost::FromID(render_process_id_);
+
+      BrowserThread::PostTask(
+          BrowserThread::UI, FROM_HERE,
+          base::Bind(&RenderProcessHost::AudioStarted,
+                     base::Unretained(render_process_host)));

[gab, 2015/06/30 13:41:40]

base::Unretained() here is incorrect because there are no guarantees that
|render_process_host| will still be around by the time this task is run (which
could result in a use-after-free).

For educational purposes, here are a couple ways to go about passing the |this|
pointer:

1) Use Unretained if you have lifetime guarantees (you don't here).

2) Use refcounting (default if not using any bind helper): only an option if the
object implements RefCounted (RenderProcessHost doesn't) AND it makes sense for
your task to prevent the object from being deleted (in this case it doesn't
either).

3) Use a WeakPtr: only an option if the object vends WeakPtrs (e.g. implements
SupportsWeakPtr or has a WeakPtrFactory) -- not the case here either, but would
have been a nice option (WeakPtrs will check that they are still valid before
running the callback).

None of the above actually work great here so here's what I suggest, instead of
binding to the RenderProcessHost object right away, add a method in this task's
anonymous namespace that runs on the UI thread and takes the
|render_process_id_| as an argument, from it obtain the |render_process_host|
pointer and call the RenderProcessHost's AudioFoo() methods directly.

In fact, as I'm done writing this I had a thought that you probably shouldn't
call RenderProcessHost::FromID() outside the UI thread. And indeed you
shouldn't:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

So this is really the only option (and you should enable dcheck_always_on=1 in
your build ASAP!).

[DaleCurtis, 2015/06/30 16:34:35]

Oops, thanks for catching this. The last method is in fact what should be done
here. I had a brain fart and thought this was attaching to the browser process,
so if it went poof everything went with it.

[sebsg, 2015/07/02 12:58:08]

Done.

[sebsg, 2015/07/02 12:58:08]

Done.

+    }
   } else {
     entry->set_playing(false);
     should_alert_resource_scheduler =
         !RenderFrameHasActiveAudio(entry->render_frame_id());
-    base::AtomicRefCountDec(&num_playing_streams_);
+
+    // If it was the last audio stream playing for this renderer.

[DaleCurtis, 2015/06/29 22:44:23]

Ditto.

[sebsg, 2015/07/02 12:58:08]

Done.

+    if (!base::AtomicRefCountDec(&num_playing_streams_)) {
+      RenderProcessHost* render_process_host =
+          RenderProcessHost::FromID(render_process_id_);
+
+      BrowserThread::PostTask(
+          BrowserThread::UI, FROM_HERE,
+          base::Bind(&RenderProcessHost::AudioStopped,
+                     base::Unretained(render_process_host)));
+    }
   }
 
   if (should_alert_resource_scheduler && ResourceDispatcherHostImpl::Get()) {