Update NetLog to be thread safe.

chrome/browser/dom_ui/net_internals_ui.cc

 // Copyright (c) 2010 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/dom_ui/net_internals_ui.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 134 matching lines @@
  private:
   class IOThreadImpl;
 
   // This is the "real" message handler, which lives on the IO thread.
   scoped_refptr<IOThreadImpl> proxy_;
 
   DISALLOW_COPY_AND_ASSIGN(NetInternalsMessageHandler);
 };
 
 // This class is the "real" message handler. With the exception of being
 // allocated and destroyed on the UI thread, its methods are expected to be

[eroman, 2010/11/17 05:59:02]

Could you updating this language to mention the OnEntryAdded?

 // called from the IO thread.
 class NetInternalsMessageHandler::IOThreadImpl
     : public base::RefCountedThreadSafe<
           NetInternalsMessageHandler::IOThreadImpl,
           BrowserThread::DeleteOnUIThread>,
       public ChromeNetLog::Observer,
       public ConnectionTester::Delegate {
  public:
   // Type for methods that can be used as MessageHandler callbacks.
   typedef void (IOThreadImpl::*MessageHandler)(const ListValue*);
@@ skipping 13 matching lines @@
   // Creates a callback that will run |method| on the IO thread.
   //
   // This can be used with DOMUI::RegisterMessageCallback() to bind to a method
   // on the IO thread.
   DOMUI::MessageCallback* CreateCallback(MessageHandler method);
 
   // Called once the DOMUI has been deleted (i.e. renderer went away), on the
   // IO thread.
   void Detach();
 
+  // Sends all passive log entries in |entries| to Javascript handler.

[eroman, 2010/11/17 05:59:02]

nit: "to the javascript handler" (added "the").

+  void SendPassiveLogEntries(const ChromeNetLog::EntryList& passive_entries);
+
   //--------------------------------
   // Javascript message handlers:
   //--------------------------------
 
-  // This message is called after the webpage's onloaded handler has fired.
-  // it indicates that the renderer is ready to start receiving captured data.
   void OnRendererReady(const ListValue* list);
 
   void OnGetProxySettings(const ListValue* list);
   void OnReloadProxySettings(const ListValue* list);
   void OnGetBadProxies(const ListValue* list);
   void OnClearBadProxies(const ListValue* list);
   void OnGetHostResolverInfo(const ListValue* list);
   void OnClearHostResolverCache(const ListValue* list);
   void OnEnableIPv6(const ListValue* list);
-  void OnGetPassiveLogEntries(const ListValue* list);
   void OnStartConnectionTests(const ListValue* list);
   void OnGetHttpCacheInfo(const ListValue* list);
   void OnGetSocketPoolInfo(const ListValue* list);
   void OnGetSpdySessionInfo(const ListValue* list);
 #ifdef OS_WIN
   void OnGetServiceProviders(const ListValue* list);
 #endif
 
   void OnSetLogLevel(const ListValue* list);
 
@@ skipping 164 matching lines @@
   dom_ui_->RegisterMessageCallback(
       "getHostResolverInfo",
       proxy_->CreateCallback(&IOThreadImpl::OnGetHostResolverInfo));
   dom_ui_->RegisterMessageCallback(
       "clearHostResolverCache",
       proxy_->CreateCallback(&IOThreadImpl::OnClearHostResolverCache));
   dom_ui_->RegisterMessageCallback(
       "enableIPv6",
       proxy_->CreateCallback(&IOThreadImpl::OnEnableIPv6));
   dom_ui_->RegisterMessageCallback(
-      "getPassiveLogEntries",
-      proxy_->CreateCallback(&IOThreadImpl::OnGetPassiveLogEntries));
-  dom_ui_->RegisterMessageCallback(
       "startConnectionTests",
       proxy_->CreateCallback(&IOThreadImpl::OnStartConnectionTests));
   dom_ui_->RegisterMessageCallback(
       "getHttpCacheInfo",
       proxy_->CreateCallback(&IOThreadImpl::OnGetHttpCacheInfo));
   dom_ui_->RegisterMessageCallback(
       "getSocketPoolInfo",
       proxy_->CreateCallback(&IOThreadImpl::OnGetSocketPoolInfo));
   dom_ui_->RegisterMessageCallback(
       "getSpdySessionInfo",
@@ skipping 46 matching lines @@
 NetInternalsMessageHandler::IOThreadImpl::CreateCallback(
     MessageHandler method) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   return new CallbackHelper(this, method);
 }
 
 void NetInternalsMessageHandler::IOThreadImpl::Detach() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   // Unregister with network stack to observe events.
   if (is_observing_log_)
-    io_thread_->globals()->net_log->RemoveObserver(this);
+    io_thread_->net_log()->RemoveObserver(this);
 
   // Cancel any in-progress connection tests.
   connection_tester_.reset();
 }
 
 void NetInternalsMessageHandler::IOThreadImpl::OnRendererReady(
     const ListValue* list) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(!is_observing_log_) << "notifyReady called twice";
 
-  // Register with network stack to observe events.
-  is_observing_log_ = true;
-  io_thread_->globals()->net_log->AddObserver(this);
-
   // Tell the javascript about the relationship between event type enums and
   // their symbolic name.
   {
     std::vector<net::NetLog::EventType> event_types =
         net::NetLog::GetAllEventTypes();
 
     DictionaryValue* dict = new DictionaryValue();
 
     for (size_t i = 0; i < event_types.size(); ++i) {
       const char* name = net::NetLog::EventTypeToString(event_types[i]);
@@ skipping 127 matching lines @@
     //               be part of the time library.
     const int64 kUnixEpochMs = 11644473600000LL;
     int64 tick_to_unix_time_ms = tick_to_time_ms - kUnixEpochMs;
 
     // Pass it as a string, since it may be too large to fit in an integer.
     CallJavascriptFunction(L"g_browser.receivedTimeTickOffset",
                            Value::CreateStringValue(
                                base::Int64ToString(tick_to_unix_time_ms)));
   }
 
-  OnGetPassiveLogEntries(NULL);
+  // Register with network stack to observe events.
+  is_observing_log_ = true;
+  ChromeNetLog::EntryList entries;
+  io_thread_->net_log()->AddObserverAndGetAllCapturedEvents(this, &entries);

[eroman, 2010/11/17 05:59:02]

The impact of threading here is fairly subtle.

I don't remember, can you check whether the javascript cares if it receives
active events BEFORE receiving the passively captured events?

I don't think it will matter, but it is worth checking and additionally noting
here that it can happen (although unlikely).

Basically I am thinking that right after we attach the observer and grab the
passively logged entries, the observer might be called on some other thread, and
in turn queue up a task in the UI thread. Then we reach the next line
(SendPassiveLogEntries) which also posts a task to the UI thread, this time to
send the renderer the passively logged entries.

[mmenke, 2010/11/17 21:42:14]

Thanks for catching that.  I hadn't thought about that case.  It looks to me
like there is actually an issue: All events within a source are assumed to be
received in the order they occurred.  I've updated the JS code to hold onto
actively logged events until passively logged ones are received.

Could try to fix it C++-side, but the Javascript solution seems a lot simpler.

+  SendPassiveLogEntries(entries);
 }
 
 void NetInternalsMessageHandler::IOThreadImpl::OnGetProxySettings(
     const ListValue* list) {
   URLRequestContext* context = context_getter_->GetURLRequestContext();
   net::ProxyService* proxy_service = context->proxy_service();
 
   DictionaryValue* dict = new DictionaryValue();
   if (proxy_service->fetched_config().is_valid())
     dict->Set("original", proxy_service->fetched_config().ToValue());
@@ skipping 136 matching lines @@
 
   if (host_resolver_impl) {
     host_resolver_impl->SetDefaultAddressFamily(
         net::ADDRESS_FAMILY_UNSPECIFIED);
   }
 
   // Cause the renderer to be notified of the new value.
   OnGetHostResolverInfo(NULL);
 }
 
-void NetInternalsMessageHandler::IOThreadImpl::OnGetPassiveLogEntries(
-    const ListValue* list) {
-  ChromeNetLog* net_log = io_thread_->globals()->net_log.get();
-
-  PassiveLogCollector::EntryList passive_entries;
-  net_log->passive_collector()->GetAllCapturedEvents(&passive_entries);
-
+void NetInternalsMessageHandler::IOThreadImpl::SendPassiveLogEntries(
+    const ChromeNetLog::EntryList& passive_entries) {
   ListValue* dict_list = new ListValue();
   for (size_t i = 0; i < passive_entries.size(); ++i) {
-    const PassiveLogCollector::Entry& e = passive_entries[i];
+    const ChromeNetLog::Entry& e = passive_entries[i];
     dict_list->Append(net::NetLog::EntryToDictionaryValue(e.type,
                                                           e.time,
                                                           e.source,
                                                           e.phase,
                                                           e.params,
                                                           false));
   }
 
   CallJavascriptFunction(L"g_browser.receivedPassiveLogEntries", dict_list);
 }
@@ skipping 107 matching lines @@
   int log_level;
   std::string log_level_string;
   if (!list->GetString(0, &log_level_string) ||
       !base::StringToInt(log_level_string, &log_level)) {
     NOTREACHED();
     return;
   }
 
   DCHECK_GE(log_level, net::NetLog::LOG_ALL);
   DCHECK_LE(log_level, net::NetLog::LOG_BASIC);
-  set_log_level(static_cast<net::NetLog::LogLevel>(log_level));
+  SetLogLevel(static_cast<net::NetLog::LogLevel>(log_level),
+               io_thread_->net_log());

[eroman, 2010/11/17 05:59:02]

indentation is off by one.

 }
 
 void NetInternalsMessageHandler::IOThreadImpl::OnAddEntry(
     net::NetLog::EventType type,
     const base::TimeTicks& time,
     const net::NetLog::Source& source,
     net::NetLog::EventPhase phase,
     net::NetLog::EventParameters* params) {

[eroman, 2010/11/17 05:59:02]

Could you add a big comment in this function noting that we could be called from
any thread?

   DCHECK(is_observing_log_);

[eroman, 2010/11/17 05:59:02]

I wander if we need a memory barrier or something to read the bool (since it
could have been just written on a different thread).

I suggest just deleting this DCHECK to be safe.

[mmenke, 2010/11/17 21:42:14]

I don't think we do, since we set it before adding ourselves as an Observer
(Which requires we acquire a lock before we're actually added), and we never set
it to false.  Still seems like it's safer to remove the DCHECK, though.  Done.

 
   CallJavascriptFunction(
       L"g_browser.receivedLogEntry",
       net::NetLog::EntryToDictionaryValue(type, time, source, phase, params,
                                           false));
 }
 
 void NetInternalsMessageHandler::IOThreadImpl::OnStartConnectionTestSuite() {
   CallJavascriptFunction(L"g_browser.receivedStartConnectionTestSuite", NULL);
 }
@@ skipping 39 matching lines @@
   if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     if (handler_) {
       // We check |handler_| in case it was deleted on the UI thread earlier
       // while we were running on the IO thread.
       handler_->CallJavascriptFunction(function_name, arg);
     }
     delete arg;
     return;
   }
 
-  // Otherwise if we were called from the IO thread, bridge the request over to
-  // the UI thread.
-
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (!BrowserThread::PostTask(
            BrowserThread::UI, FROM_HERE,
            NewRunnableMethod(
                this,
                &IOThreadImpl::CallJavascriptFunction,
                function_name, arg))) {
     // Failed posting the task, avoid leaking.
     delete arg;
   }
 }
@@ skipping 13 matching lines @@
   NetInternalsHTMLSource* html_source = new NetInternalsHTMLSource();
 
   // Set up the chrome://net-internals/ source.
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       NewRunnableMethod(
           Singleton<ChromeURLDataManager>::get(),
           &ChromeURLDataManager::AddDataSource,
           make_scoped_refptr(html_source)));
 }