Implement browser-side logging to WebRtc log

content/browser/renderer_host/media/media_stream_manager.cc

Index: content/browser/renderer_host/media/media_stream_manager.cc
diff --git a/content/browser/renderer_host/media/media_stream_manager.cc b/content/browser/renderer_host/media/media_stream_manager.cc
index 6ed73ca48f62b6973c1cdf275b9c355ac32fce38..a93ce4c48056eb368294a09de65a22512756ebc6 100644
--- a/content/browser/renderer_host/media/media_stream_manager.cc
+++ b/content/browser/renderer_host/media/media_stream_manager.cc
@@ -14,17 +14,20 @@
 #include "base/rand_util.h"
 #include "base/run_loop.h"
 #include "base/threading/thread.h"
+#include "content/browser/browser_main_loop.h"
 #include "content/browser/renderer_host/media/audio_input_device_manager.h"
 #include "content/browser/renderer_host/media/device_request_message_filter.h"
 #include "content/browser/renderer_host/media/media_stream_requester.h"
 #include "content/browser/renderer_host/media/media_stream_ui_proxy.h"
 #include "content/browser/renderer_host/media/video_capture_manager.h"
 #include "content/browser/renderer_host/media/web_contents_capture_util.h"
+#include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/media_device_id.h"
 #include "content/public/browser/media_observer.h"
 #include "content/public/browser/media_request_state.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/media_stream_request.h"
 #include "media/audio/audio_manager_base.h"
@@ -36,6 +39,19 @@
 #include "base/win/scoped_com_initializer.h"
 #endif
 
+static void DoAddLogMessage(const std::string& message) {

[tommi (sloooow) - chröme, 2014/01/24 12:34:54]

I think //content prefers anonymous namespaces (even inside the content
namespace).  //media is a bit special in this way and that's the style you're
following here I think.

[vrk (LEFT CHROMIUM), 2014/01/25 00:50:21]

Done.

+  // Must be on the UI thread to access BrowserMainLoop.
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  // May be null in tests.
+  // TODO(vrk): Handle this more elegantly by having native log messages become
+  // no-ops until MediaStreamManager is aware that a renderer process has
+  // started logging. crbug.com/333894
+  if (content::BrowserMainLoop::GetInstance()) {
+    content::BrowserMainLoop::GetInstance()->
+        media_stream_manager()->AddLogMessageOnUIThread(message);
+  }
+}
+
 namespace content {
 
 namespace {
@@ -846,6 +862,24 @@ bool MediaStreamManager::TranslateSourceIdToDeviceId(
   return false;
 }
 
+void MediaStreamManager::LogDevicesToNativeLogs(
+    MediaStreamType stream_type,
+    const StreamDeviceInfoArray& devices) {
+  std::stringstream devices_msg;
+  devices_msg << "Devices found for stream type " << stream_type << ":"
+              << std::endl;
+  for (StreamDeviceInfoArray::const_iterator it = devices.begin();
+       it != devices.end(); ++it) {
+    devices_msg << "  " << it->device.name << " ("
+                << it->device.id << ")" << std::endl;

[no longer working on chromium, 2014/01/24 09:54:38]

btw, are you supposed to log the device_id? has the private concern been
addressed here?

[tommi (sloooow) - chröme, 2014/01/24 12:34:54]

Agreed. To be safe we should either skip logging the ID or log only the ID as
javascript will see it (origin specific).

[vrk (LEFT CHROMIUM), 2014/01/25 00:50:21]

Removed. (Fwiw I also sent a mail to kaichou@ with sample logs to get the
privacy OK.)

[vrk (LEFT CHROMIUM), 2014/01/25 00:50:21]

Done, just omitted.

+  }
+  if (devices.empty())

[no longer working on chromium, 2014/01/24 09:54:38]

nit, I think it is cleaner to do:
if (devices.empty()) {
  devices_msg << "No devices found.";
} else {
  for (StreamDeviceInfoArray::const_iterator it = devices.begin();
       it != devices.end(); ++it) {
    devices_msg << "  " << it->device.name << " (" << it->device.id << ")" <<
std::endl;
}

[vrk (LEFT CHROMIUM), 2014/01/25 00:50:21]

Done.

+    devices_msg << "No devices found.";
+
+  DVLOG(1) << devices_msg;
+  SendMessageToNativeLog(devices_msg.str());
+}
+
 void MediaStreamManager::ClearEnumerationCache(EnumerationCache* cache) {
   DCHECK_EQ(base::MessageLoop::current(), io_loop_);
   cache->valid = false;
@@ -1014,6 +1048,11 @@ void MediaStreamManager::SetupRequest(const std::string& label) {
       // Enumerate the devices if there is no valid device lists to be used.
       StartEnumeration(request);
       return;
+    } else {
+      // Cache is valid, so log the cached devices.

[no longer working on chromium, 2014/01/24 09:54:38]

are you sure you want to log the cache for each SetupRequest?
Note that SetupRequest is used by all the clients, including pepper, and I know
that they call OpenDevice() once a while to poll the device. If things are still
the case, you will log a lot here.

[no longer working on chromium, 2014/01/24 10:36:17]

Per pointed out that only media stream requests were logged in
AddLogMessageOnUIThread. But what I understand from the code is that you might
log the same request multiple times. Right?

[vrk (LEFT CHROMIUM), 2014/01/25 00:50:21]

Thanks for letting me know. I don't think there will be many situations in which
people are running native logging and Pepper at the same time, but added an if
statement to limit to MediaStream requests. This isn't a great solution, but I
think the real problem is that MediaStreamManager needs to be refactored. At the
very least, I don't think pepper code should be in this class. I added
crbug.com/337982 for this cleanup.

+      SendMessageToNativeLog("Using cached devices for request.");

[tommi (sloooow) - chröme, 2014/01/24 12:34:54]

Would it make sense to aggregate all these messages into a single message so
that they're guaranteed to stay intact inside the log?

+      LogDevicesToNativeLogs(audio_type, audio_enumeration_cache_.devices);
+      LogDevicesToNativeLogs(video_type, video_enumeration_cache_.devices);

[no longer working on chromium, 2014/01/24 09:54:38]

what do you log if the type is MEDIA_NO_SERVICE ?

[vrk (LEFT CHROMIUM), 2014/01/25 00:50:21]

See LogDevicesToNativeLogs - it would log the MEDIA_NO_SERVICE enum value and
"No devices found." I don't think that's the worst thing in the world, but I
added some logic to make it clearer.

     }
 
     if (!SetupDeviceCaptureRequest(request)) {
@@ -1403,7 +1442,10 @@ void MediaStreamManager::DevicesEnumerated(
     MediaStreamType stream_type, const StreamDeviceInfoArray& devices) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DVLOG(1) << "DevicesEnumerated("
-           << ", {stream_type = " << stream_type <<  "})";
+           << "{stream_type = " << stream_type << "})" << std::endl;
+
+  SendMessageToNativeLog("New device enumeration result:");
+  LogDevicesToNativeLogs(stream_type, devices);
 
   // Only cache the device list when the device list has been changed.
   bool need_update_clients = false;
@@ -1476,6 +1518,35 @@ void MediaStreamManager::DevicesEnumerated(
   DCHECK_GE(active_enumeration_ref_count_[stream_type], 0);
 }
 
+// static
+void MediaStreamManager::SendMessageToNativeLog(const std::string& message) {
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,

[no longer working on chromium, 2014/01/24 09:54:38]

I have some concern on the thread safety here. Previously MediaStreamManager is
listening to a WillDestroyCurrentMessageLoop() message to clean up its resource
when the IO thread is going away, then MediaStreamManager will be deleted on UI
thread.
But this method will post a task on UI thread to MediaStreamManager itself, can
it happen that MediaStreamManager gets deleted before the task is executed? I
think this is at least theoretically possible and depending on how this method
is used.

I believe you need to use a weak pointer to invalidate all the tasks when IO
thread is going away.

[tommi (sloooow) - chröme, 2014/01/24 12:34:54]

Actually, this method is static and the task that gets posted isn't bound to a
MediaStreamManager pointer.  DoAddLogMessage will actually do the checking of
whether the manager instance is dead or alive, so I think this is OK from that
standpoint.

[vrk (LEFT CHROMIUM), 2014/01/25 00:50:21]

I believe so as well, so leaving as is.

+      base::Bind(::DoAddLogMessage, message));
+}
+
+void MediaStreamManager::AddLogMessageOnUIThread(const std::string& message) {
+  // Must be on the UI thread to access RenderProcessHost from process ID.
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // Add logs for each pending request.
+  for (DeviceRequests::iterator it = requests_.begin(); it != requests_.end();
+       ++it) {
+    DeviceRequest* request = it->second;
+    // Log the message to all renderers that are requesting a MediaStream or

[perkj_chrome, 2014/01/24 09:15:10]

Can you expand the comment : that are requesting a MediaStream or have a
MediaStream running

[vrk (LEFT CHROMIUM), 2014/01/25 00:50:21]

Done.

+    // device enumeration.
+    if (request->request_type == MEDIA_GENERATE_STREAM ||
+        request->request_type == MEDIA_ENUMERATE_DEVICES) {

[perkj_chrome, 2014/01/24 09:15:10]

Fyi- MEDIA_ENUMERATE_DEVICES is called by flash for some reason every 10s or so.


I would recommend that you run flash in one tab and webrtc in another and look
at the logs to see that you are not spammed.

[vrk (LEFT CHROMIUM), 2014/01/25 00:50:21]

Pepper flash? I don't understand why MediaStreamManager deals with Pepper
requests at all. I created crbug.com/337982 in response to this.
 
I just removed the MEDIA_ENUMERATE_DEVICES if statement. We really only care
about MEDIA_GENERATE_STREAM.

+      content::RenderProcessHostImpl* render_process_host_impl =
+          static_cast<content::RenderProcessHostImpl*>(
+              content::RenderProcessHost::FromID(
+                  request->requesting_process_id));
+      if (render_process_host_impl)
+        render_process_host_impl->WebRtcLogMessage(message);
+    }
+  }
+}
+
 void MediaStreamManager::HandleAccessRequestResponse(
     const std::string& label,
     const MediaStreamDevices& devices) {