Change WebRTC log callback registration in browser process.

chrome/browser/media/webrtc_logging_handler_host.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/media/webrtc_logging_handler_host.h"
 
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 130 matching lines @@
 void WebRtcLogBuffer::SetComplete() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!read_only_) << "Already set? (programmer error)";
   read_only_ = true;
   // Detach from the current thread so that we can check reads on a different
   // thread.  This is to make sure that Read()s still happen on one thread only.
   thread_checker_.DetachFromThread();
 }
 
 WebRtcLoggingHandlerHost::WebRtcLoggingHandlerHost(
+    int render_process_id,
     Profile* profile,
     WebRtcLogUploader* log_uploader)
     : BrowserMessageFilter(WebRtcLoggingMsgStart),
       profile_(profile),
       logging_state_(CLOSED),
       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_);
 }
 
 WebRtcLoggingHandlerHost::~WebRtcLoggingHandlerHost() {
   // If we hit this, then we might be leaking a log reference count (see
   // ApplyForStartLogging).
   DCHECK_EQ(CLOSED, logging_state_);
 }
 
@@ skipping 57 matching lines @@
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(!callback.is_null());
 
   if (logging_state_ != STARTED) {
     FireGenericDoneCallback(callback, false, "Logging not started.");
     return;
   }
 
   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) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(!callback.is_null());
 
   if (logging_state_ != STOPPED) {
     if (!callback.is_null()) {
       content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
           base::Bind(callback, false, "", kLogNotStoppedOrNoLogOpen));
@@ skipping 377 matching lines @@
   LogToCircularBuffer("Discovered " + base::SizeTToString(network_list.size()) +
                       " network interfaces:");
   for (net::NetworkInterfaceList::const_iterator it = network_list.begin();
        it != network_list.end(); ++it) {
     LogToCircularBuffer(
         "Name: " + it->friendly_name + ", Address: " +
         IPAddressToSensitiveString(it->address) + ", Type: " +
         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);
   log_buffer_->Log(message);
 }
 
 base::FilePath WebRtcLoggingHandlerHost::GetLogDirectoryAndEnsureExists() {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   base::FilePath log_dir_path =
       WebRtcLogList::GetWebRtcLogDirectoryForProfile(profile_->GetPath());
@@ skipping 212 matching lines @@
 
   if (!is_audio_debug_recordings_in_progress_) {
     error_callback.Run("No audio debug recording in progress");
     return;
   }
 
   host->DisableAudioDebugRecordings();
   is_audio_debug_recordings_in_progress_ = false;
   callback.Run(prefix_path.AsUTF8Unsafe(), true /* stopped */, is_manual_stop);
 }