Change WebRTC log callback registration in browser process.

chrome/browser/media/webrtc_logging_handler_host.cc

Index: chrome/browser/media/webrtc_logging_handler_host.cc
diff --git a/chrome/browser/media/webrtc_logging_handler_host.cc b/chrome/browser/media/webrtc_logging_handler_host.cc
index 0106cb34718e02866df51f1d5ad2a1d94a7ac90b..832558d8eff9bfa15f3ce72f608d500743ce6077 100644
--- a/chrome/browser/media/webrtc_logging_handler_host.cc
+++ b/chrome/browser/media/webrtc_logging_handler_host.cc
@@ -148,6 +148,7 @@ void WebRtcLogBuffer::SetComplete() {
 }
 
 WebRtcLoggingHandlerHost::WebRtcLoggingHandlerHost(
+    int render_process_id,
     Profile* profile,
     WebRtcLogUploader* log_uploader)
     : BrowserMessageFilter(WebRtcLoggingMsgStart),
@@ -156,7 +157,8 @@ WebRtcLoggingHandlerHost::WebRtcLoggingHandlerHost(
       upload_log_on_render_close_(false),
       log_uploader_(log_uploader),
       is_audio_debug_recordings_in_progress_(false),
-      current_audio_debug_recordings_id_(0) {
+      current_audio_debug_recordings_id_(0),
+      render_process_id_(render_process_id) {
   DCHECK(profile_);
   DCHECK(log_uploader_);
 }
@@ -234,7 +236,14 @@ void WebRtcLoggingHandlerHost::StopLogging(
 
   stop_callback_ = callback;
   logging_state_ = STOPPING;
+
   Send(new WebRtcLoggingMsg_StopLogging());
+
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(
+          &WebRtcLoggingHandlerHost::DisableBrowserProcessLoggingOnUIThread,
+          this));
 }
 
 void WebRtcLoggingHandlerHost::UploadLog(const UploadDoneCallback& callback) {
@@ -632,12 +641,36 @@ void WebRtcLoggingHandlerHost::LogInitialInfoOnIOThread(
         net::NetworkChangeNotifier::ConnectionTypeToString(it->type));
   }
 
+  BrowserThread::PostTask(

[o1ka, 2016/04/07 13:00:11]

Probably move it to the top to have less chances to miss something in the
beginning?

[tommi (sloooow) - chröme, 2016/04/07 13:52:19]

Won't that introduce a potential race with writing the log header?

[Henrik Grunell, 2016/04/07 14:32:59]

It won't race, since this function and LogMessage() are both on the IO thread.

It's not a big thing, but doesn't hurt to do it early. Moved.

+      BrowserThread::UI, FROM_HERE,
+      base::Bind(
+          &WebRtcLoggingHandlerHost::EnableBrowserProcessLoggingOnUIThread,
+          this));
+
   Send(new WebRtcLoggingMsg_StartLogging());
   logging_started_time_ = base::Time::Now();
   logging_state_ = STARTED;
   FireGenericDoneCallback(callback, true, "");
 }
 
+void WebRtcLoggingHandlerHost::EnableBrowserProcessLoggingOnUIThread() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  content::RenderProcessHost* host =
+      content::RenderProcessHost::FromID(render_process_id_);
+  if (host) {
+    host->SetWebRtcLogMessageCallback(

[o1ka, 2016/04/07 13:00:11]

What guarantees that |host| is still alive here (i.e. the pointer is valid)? A
comment would be nice.

[tommi (sloooow) - chröme, 2016/04/07 13:52:19]

FromID() must be called on the UI thread, it returns a pointer that can only be
cleared on the UI thread, so we're safe in this scope as long as we don't do any
message processing (which we don't)

[Henrik Grunell, 2016/04/07 14:32:59]

Yes. Actually, I don't think we should spam with a comment for this. In that
case everyone who uses FromID() should have that.

+        base::Bind(&WebRtcLoggingHandlerHost::LogMessage, this));
+  }
+}
+
+void WebRtcLoggingHandlerHost::DisableBrowserProcessLoggingOnUIThread() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  content::RenderProcessHost* host =
+      content::RenderProcessHost::FromID(render_process_id_);
+  if (host)
+    host->UnsetWebRtcLogMessageCallback();

[tommi (sloooow) - chröme, 2016/04/07 12:51:44]

could this possibly 'unset' a callback that was not set via
EnableBrowserProcessLoggingOnUIThread?

E.g. a call sequence like this:

1. A call is made to EnableBrowserProcessLoggingOnUIThread()
  -> host is null
2. A second call is made to EnableBrowserProcessLoggingOnUIThread()
  -> is not null, so the callback is set.
3. The call to Disable that corresponds to step #1 happens
  -> This causes the #2 Enable scope, to be prematurely closed.
4. Second call to Disable is effectively a no-op since a callback isn't
registered.

So, should Enable return a bool?

Should we have a Scoped type class that makes sure we automatically always call
Disable if Enable has returned true?

Should we DCHECK that a callback is in fact registered when we call
UnsetWebRtcLogMessageCallback?

[o1ka, 2016/04/07 13:00:11]

What guarantees that |host| is still alive here (i.e. the pointer is valid)

[Henrik Grunell, 2016/04/07 13:34:53]

This can't happen. These two functions are only called when logging is started
and stopped respectively, and we have a state so it's not possible to start
twice.
Hmm, there is still a disable when the render process is going away, but that's
in RenderProcessHostImpl:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

It would be nice to move that to here, but I don't think we can rely on the host
still being alive which is needed to be able to remove the callback. If we
continue to rely on the RPHI to remove it, some commenting around this is good.

Where you thinking of some weak ptr style type?
If we keep the current approach, we can't.

[tommi (sloooow) - chröme, 2016/04/07 13:52:19]

From the code, it looks like it's at least theoratically possible, right?  If
it's not supposed to be possible, can we have a check to that effect?

[tommi (sloooow) - chröme, 2016/04/07 13:52:19]

On 2016/04/07 13:34:53, Henrik Grunell wrote:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
Depends on the use, but I was thinking of something like:

class Foo {
 public:
  ...
  class ScopedEnableLogging {
   public:
    ScopedEnableLogging(Foo* foo) : foo_(foo), enabled_(foo->EnableLogging()) {}
    ~ScopedEnableLogging() {
      if (enabled_)
        foo_->DisableLogging();
    }

   private:  
    const bool enabled_;
  };

 protected:
  friend class ScopedEnableLogging;
  void EnableLogging();
  void DisableLogging();
  ...
};

Maybe that's not applicable in this situation.

[Henrik Grunell, 2016/04/07 14:32:59]

No it's no applicable. In fact, the callback will hold a ref to the logging
handler, so we won't even ever destroy it as long as the callback is registered.
We have to clear it in RPHI as done now.

[Henrik Grunell, 2016/04/07 14:32:59]

Hmm, in MSM it's OK to call several times, as a comment says. Tests do this.
Here, it should not be. I'd need to add book keeping (a flag) here to check it.
I'd prefer not to add more. You have the last say. :)

+}
+
 void WebRtcLoggingHandlerHost::LogToCircularBuffer(const std::string& message) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK_NE(logging_state_, CLOSED);