Update NetLog to be thread safe.

chrome/browser/net/chrome_net_log.cc

Index: chrome/browser/net/chrome_net_log.cc
===================================================================
--- chrome/browser/net/chrome_net_log.cc	(revision 65207)
+++ chrome/browser/net/chrome_net_log.cc	(working copy)
@@ -7,9 +7,10 @@
 #include <algorithm>
 
 #include "base/command_line.h"
+#include "base/lock.h"
 #include "base/logging.h"
 #include "base/string_util.h"
-#include "chrome/browser/browser_thread.h"
+#include "base/values.h"
 #include "chrome/browser/net/load_timing_observer.h"
 #include "chrome/browser/net/net_log_logger.h"
 #include "chrome/browser/net/passive_log_collector.h"
@@ -21,15 +22,34 @@
   return log_level_;
 }
 
-void ChromeNetLog::Observer::set_log_level(net::NetLog::LogLevel log_level) {
+void ChromeNetLog::Observer::SetLogLevel(net::NetLog::LogLevel log_level,
+                                         ChromeNetLog* net_log) {
+  DCHECK(net_log);
+  AutoLock lock(net_log->lock_);
   log_level_ = log_level;
+  net_log->UpdateLogLevel_();
 }
 
+ChromeNetLog::Entry::Entry(uint32 order,
+                           net::NetLog::EventType type,
+                           const base::TimeTicks& time,
+                           net::NetLog::Source source,
+                           net::NetLog::EventPhase phase,
+                           net::NetLog::EventParameters* params)
+    : order(order),
+      type(type),
+      time(time),
+      source(source),
+      phase(phase),
+      params(params) {
+}
+
+ChromeNetLog::Entry::~Entry() {}
+
 ChromeNetLog::ChromeNetLog()
-    : next_id_(1),
+    : last_id_(0),

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

Where is log_level_ initialized? (it may still be accessed even though there are
no observers).

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

It's actually initialized on the first call to AddObserver, a couple lines down.
 That having been said, initializing it explicitly seems a pretty reasonable
idea.

       passive_collector_(new PassiveLogCollector),
       load_timing_observer_(new LoadTimingObserver) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   AddObserver(passive_collector_.get());
   AddObserver(load_timing_observer_.get());
 
@@ -41,7 +61,6 @@
 }
 
 ChromeNetLog::~ChromeNetLog() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   RemoveObserver(passive_collector_.get());
   RemoveObserver(load_timing_observer_.get());
   if (net_log_logger_.get()) {
@@ -54,11 +73,7 @@
                             const Source& source,
                             EventPhase phase,
                             EventParameters* params) {
-  // This must be invoked when we're on the IO thread, or if the IO thread's
-  // message loop isn't valid. The later can happen if this is invoked when the
-  // IOThread is shuting down the MessageLoop.
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO) ||
-         !BrowserThread::IsMessageLoopValid(BrowserThread::IO));
+  AutoLock lock(lock_);
 
   // Notify all of the log observers.
   FOR_EACH_OBSERVER(Observer, observers_,
@@ -66,30 +81,46 @@
 }
 
 uint32 ChromeNetLog::NextID() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  return next_id_++;
+  return base::subtle::NoBarrier_AtomicIncrement(&last_id_, 1);
 }
 
 net::NetLog::LogLevel ChromeNetLog::GetLogLevel() const {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  base::subtle::Atomic32 log_level = base::subtle::NoBarrier_Load(&log_level_);

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

I need to buff up on my concurrency... what is the implication between doing it
with/without memory barrier?

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

I'm pretty weak here, too, but the barrier will make sure no reads/writes will
be reordered from before the instruction to after it by the compiler or the CPU.
 This applies across CPUs, and should handle issues both relating to caching
data in registers and cache coherency issues as well.

As we don't have any data that could have concurrency issues (Other than what
we're using the NoBarrier operations on), we shouldn't need barriers.

If we depended on data set before we last modified |log_level_|, for example, we
would need a barrier, if we weren't using a mutex (Mutexes always come with
their own implicit memory barriers, I believe).

Having a barrier is, of course, slower than not having one, but other than that,
using one when you don't need one shouldn't cause any issues, in general.

If you want to solicit a more informed opinion on the issue, I would not be
offended in the least.

+  return static_cast<net::NetLog::LogLevel>(log_level);
+}
 
+void ChromeNetLog::AddObserver(Observer* observer) {
+  AutoLock lock(lock_);
+  observers_.AddObserver(observer);
+  UpdateLogLevel_();
+}
+
+void ChromeNetLog::RemoveObserver(Observer* observer) {
+  AutoLock lock(lock_);
+  observers_.RemoveObserver(observer);
+  UpdateLogLevel_();
+}
+
+void ChromeNetLog::AddObserverAndGetAllCapturedEvents(
+    Observer* observer,
+    EntryList* passive_entries) {
+  AutoLock lock(lock_);
+  observers_.AddObserver(observer);
+  UpdateLogLevel_();
+  passive_collector_->GetAllCapturedEvents(passive_entries);
+}
+
+void ChromeNetLog::UpdateLogLevel_() {
+  lock_.AssertAcquired();
+
   // Look through all the observers and find the finest granularity
   // log level (higher values of the enum imply *lower* log levels).
-  LogLevel log_level = LOG_BASIC;
+  LogLevel new_log_level = LOG_BASIC;
   ObserverListBase<Observer>::Iterator it(observers_);
   Observer* observer;
   while ((observer = it.GetNext()) != NULL) {
-    log_level = std::min(log_level, observer->log_level());
+    new_log_level = std::min(new_log_level, observer->log_level());
   }
-  return log_level;
+  base::subtle::NoBarrier_Store(&log_level_, new_log_level);
 }
 
-void ChromeNetLog::AddObserver(Observer* observer) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  observers_.AddObserver(observer);
-}
-
-void ChromeNetLog::RemoveObserver(Observer* observer) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  observers_.RemoveObserver(observer);
-}