Update NetLog to be thread safe.

chrome/browser/net/chrome_net_log.h

Index: chrome/browser/net/chrome_net_log.h
===================================================================
--- chrome/browser/net/chrome_net_log.h	(revision 65207)
+++ chrome/browser/net/chrome_net_log.h	(working copy)
@@ -6,8 +6,10 @@
 #define CHROME_BROWSER_NET_CHROME_NET_LOG_H_
 #pragma once
 
+#include "base/atomicops.h"
 #include "base/observer_list.h"
 #include "base/scoped_ptr.h"
+#include "chrome/browser/browser_thread.h"
 #include "net/base/net_log.h"
 
 class LoadTimingObserver;
@@ -17,39 +19,84 @@
 // ChromeNetLog is an implementation of NetLog that dispatches network log
 // messages to a list of observers.
 //
+// All methods are threadsafe, with the exception that no ChromeNetLog or
+// ChromeNetLog::ThreadSafeObserver functions may be called by an observer's
+// OnAddEntry() method.
+//
 // By default, ChromeNetLog will attach the observer PassiveLogCollector which
 // will keep track of recent request information (which used when displaying
 // the about:net-internals page).
 //
-// TODO(eroman): Move this default observer out of ChromeNetLog.
-//
 class ChromeNetLog : public net::NetLog {
  public:
+  // This structure encapsulates all of the parameters of an event,
+  // including an "order" field that identifies when it was captured relative
+  // to other events.
+  struct Entry {
+    Entry(uint32 order,
+          net::NetLog::EventType type,
+          const base::TimeTicks& time,
+          net::NetLog::Source source,
+          net::NetLog::EventPhase phase,
+          net::NetLog::EventParameters* params);
+    ~Entry();
+
+    uint32 order;
+    net::NetLog::EventType type;
+    base::TimeTicks time;
+    net::NetLog::Source source;
+    net::NetLog::EventPhase phase;
+    scoped_refptr<net::NetLog::EventParameters> params;
+  };
+
+  typedef std::vector<Entry> EntryList;
+
   // Interface for observing the events logged by the network stack.
-  class Observer {
+  class ThreadSafeObserver {
    public:
     // Constructs an observer that wants to see network events, with
-    // the specified minimum event granularity.
+    // the specified minimum event granularity.  A ThreadSafeObserver can only
+    // observe a single ChromeNetLog at a time.
     //
     // Typical observers should specify LOG_BASIC.
     //
     // Observers that need to see the full granularity of events can
     // specify LOG_ALL. However doing so will have performance consequences,
-    // and may cause PassiveLogCollector to use more memory than anticiapted.
-    explicit Observer(LogLevel log_level);
+    // and may cause PassiveLogCollector to use more memory than anticipated.
+    //
+    // Observers will be called on the same thread an entry is added on,
+    // and are responsible for ensuring their own thread safety.
+    explicit ThreadSafeObserver(LogLevel log_level);
 
-    virtual ~Observer() {}
+    virtual ~ThreadSafeObserver();
+
+    // This method will be called on the thread that the event occurs on.  It
+    // is the responsibility of the Observer to handle it in a threadsafe
+    // manner.
+    //
+    // It is illegal for an Observer to call any ChromeNetLog or

[eroman, 2010/11/18 18:04:03]

nit: "an Observer" --> either:
   "an observer" or "a ThreadsafeObserver"

[mmenke, 2010/11/23 16:48:45]

Thought I caught all of those.

+    // ChromeNetLog::ThreadSafeObserver functions in response to a call to
+    // OnAddEntry.
     virtual void OnAddEntry(EventType type,
                             const base::TimeTicks& time,
                             const Source& source,
                             EventPhase phase,
                             EventParameters* params) = 0;
     LogLevel log_level() const;
-   protected:
-    void set_log_level(LogLevel log_level);
+
+    // This must only be called by ChromeNetLog.
+    void set_net_log(ChromeNetLog* net_log);

[eroman, 2010/11/18 18:04:03]

Can you make it private? (outter classes have direct access to private members
of their inner classes)

[mmenke, 2010/11/23 16:48:45]

I tried that initially, but was getting a compile error...  Also tried accessing
it directly.  Same is true if I make it protected instead of private.

It looks like inner classes *can* access private members of their outer class,
however.  Only problem with switching to something like
ObserverThreadSafe::StartObserving(ChromeNetLog*) is
AddObserverAndGetAllPassivelyCapturedEvents().

I've made ChromeNetLog a friend of ObserverThreadSafe.  friend doesn't seem to
be used too heavily in the Chrome code base, but it is used, and seems the
simplest workaround.

+    ChromeNetLog* net_log();
+  protected:
+    // Can only be called when actively observing a ChromeNetLog.
+    void SetLogLevel(LogLevel log_level);
    private:
+    // ChromeNetLog currently being observed, if any.  Set by ChromeNetLog's
+    // AddObserver and RemoveObserver methods.
+    ChromeNetLog* net_log_;
+
     LogLevel log_level_;
-    DISALLOW_COPY_AND_ASSIGN(Observer);
+    DISALLOW_COPY_AND_ASSIGN(ThreadSafeObserver);
   };
 
   ChromeNetLog();
@@ -64,26 +111,40 @@
   virtual uint32 NextID();
   virtual LogLevel GetLogLevel() const;
 
-  void AddObserver(Observer* observer);
-  void RemoveObserver(Observer* observer);
+  void AddObserver(ThreadSafeObserver* observer);
+  void RemoveObserver(ThreadSafeObserver* observer);
 
-  PassiveLogCollector* passive_collector() {
-    return passive_collector_.get();
-  }
+  // Adds |observer| and writes all passively captured events to
+  // |passive_entries|. Guarantees that no events in |passive_entries| will be
+  // sent to |observer| and all future events that have yet been sent to the
+  // PassiveLogCollector will be sent to |observer|.
+  void AddObserverAndGetAllPassivelyCapturedEvents(ThreadSafeObserver* observer,
+                                                   EntryList* passive_entries);
 
+  void ClearAllPassivelyCapturedEvents();
+
   LoadTimingObserver* load_timing_observer() {
     return load_timing_observer_.get();
   }
 
  private:
-  uint32 next_id_;
+  // Called whenever an Observer is added or removed, or changes its log level.

[eroman, 2010/11/18 18:04:03]

same comment regarding the capitlized "Observer".

+  // Must have acquired |lock_| prior to calling.
+  void UpdateLogLevel_();
+
+  Lock lock_;
+
+  base::subtle::Atomic32 last_id_;

[eroman, 2010/11/18 18:04:03]

Could you add a brief comment for this? (I realize it didn't have one before, my
bad).

Perhaps:

// The ID number of the most recently added NetLog::Source

+  base::subtle::Atomic32 log_level_;
+
+  // Not threadsafe.  Must only be used when |lock_| is acquired.
   scoped_ptr<PassiveLogCollector> passive_collector_;
+
   scoped_ptr<LoadTimingObserver> load_timing_observer_;
   scoped_ptr<NetLogLogger> net_log_logger_;
 
-  // Note that this needs to be "mutable" so we can iterate over the observer
-  // list in GetLogLevel().
-  mutable ObserverList<Observer, true> observers_;
+  // |lock_| must be acquired whenever reading or writing to this.
+  ObserverList<ThreadSafeObserver, true> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeNetLog);
 };