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,6 +19,10 @@
 // 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::Observer 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).
@@ -25,6 +31,28 @@
 //
 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 {

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

I see, you moved this out of PassiveLogCollector because of
AddObserverAndGetAllCapturedEvents() needing to reference it.

It feels a bit out of place here, since we don't use it as the parameter to
OnAddEntry() or anything. I might suggest forward declaring this as
"PassiveLogCollectorEntry", but maybe that is more trouble that it is worth.

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

Reason I didn't do that is because EntryList is an array of them (Not of
pointers to them, as that wouldn't be threadsafe), so couldn't create the
typedef EntryList if they were forward declared.  EntryList itself cannot be
forward declared either.

We also can't just declare it in passive_log_collector.h and include the file
here, as its a child class of ObserverThreadSafe.

I'm not sure there's a clean way to get Entry out of this file, other than
putting it in its own file and including it in this one.  If you think that's a
better solution, happy to go ahead and do it.

+    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 {

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

I suggest renaming this to "ThreadSafeObserver".

That will emphasize to future consumers that they must support being called from
multiple threads (since it is easy for tests to not cover that case).

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

Done.

    public:
@@ -35,10 +63,16 @@
     //
     // 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.
+    // and may cause PassiveLogCollector to use more memory than anticipated.

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

thanks for fixing the typo!

+    //
+    // Observers will be called on the same thread an entry is added on,
+    // and are responsible for ensuring their own thread safety.
     explicit Observer(LogLevel log_level);
 
     virtual ~Observer() {}
+
+    // It is illegal for an Observer to call any ChromeNetLog or
+    // ChromeNetLog::Observer functions in response to a call to OnAddEntry.

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

I suggest additionaly re-iterating the threading model here.

     virtual void OnAddEntry(EventType type,
                             const base::TimeTicks& time,
                             const Source& source,
@@ -46,7 +80,9 @@
                             EventParameters* params) = 0;
     LogLevel log_level() const;
    protected:
-    void set_log_level(LogLevel log_level);
+    // |net_log| is the ChromeNetLog the Observer is currently observing,
+    // and cannot be NULL.
+    void SetLogLevel(LogLevel log_level, ChromeNetLog* net_log);

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

Taking the ChromeNetLog* here isn't very intuitive.

What if Observer instead had a ChromeNetLog* member variable that tracked the
current NetLog it was observing?

ChromeNetLog could update it as part of the AddObserver/RemoveObserver methods.

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

Done.  Only thing I don't like about that is that it allows things to
theoretically be in an inconsistent state.

    private:
     LogLevel log_level_;
     DISALLOW_COPY_AND_ASSIGN(Observer);
@@ -67,6 +103,13 @@
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
+  // 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 AddObserverAndGetAllCapturedEvents(Observer* observer,

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

I have a reputation for writing incredibly long function names... in that
spirit, I suggest adding "Passive" to this name :-)

As in: AddObserverAndGetAllPassivelyCapturedEvents().

If you find that too unpleasant to bear, I will understand.

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

I'm fine with it, actually.  Only thing I have against long function names is
that they force their parameters onto the next line.

+                                          EntryList* passive_entries);
+
   PassiveLogCollector* passive_collector() {
     return passive_collector_.get();
   }
@@ -76,14 +119,20 @@
   }
 
  private:
-  uint32 next_id_;
+  // Called whenever an Observer is added or removed, or changes its log level.
+  // Must have acquired |lock_| prior to calling.
+  void UpdateLogLevel_();
+
+  Lock lock_;
+
+  base::subtle::Atomic32 last_id_;
+  base::subtle::Atomic32 log_level_;
+
   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_;
+  ObserverList<Observer, true> observers_;

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

Can you mention that |lock_| must be held whenever reading/writing this?

 
   DISALLOW_COPY_AND_ASSIGN(ChromeNetLog);
 };