Update NetLog to be thread safe.

chrome/browser/net/passive_log_collector.h

Index: chrome/browser/net/passive_log_collector.h
===================================================================
--- chrome/browser/net/passive_log_collector.h	(revision 65207)
+++ chrome/browser/net/passive_log_collector.h	(working copy)
@@ -34,27 +34,6 @@
 // owned by a SourceTracker instance for the specific source type.
 class PassiveLogCollector : public ChromeNetLog::Observer {
  public:
-  // This structure encapsulates all of the parameters of a captured 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;
   typedef std::vector<net::NetLog::Source> SourceDependencyList;
 
   struct SourceInfo {
@@ -67,7 +46,7 @@
     std::string GetURL() const;
 
     uint32 source_id;
-    EntryList entries;
+    ChromeNetLog::EntryList entries;
     size_t num_entries_truncated;
 
     // List of other sources which contain information relevant to this
@@ -93,13 +72,13 @@
    public:
     virtual ~SourceTrackerInterface() {}
 
-    virtual void OnAddEntry(const Entry& entry) = 0;
+    virtual void OnAddEntry(const ChromeNetLog::Entry& entry) = 0;

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

See earlier comment about forward declaring. To ease refactor could also have a
typedef like:

typedef PassiveLogCollectorEntry Entry;

I dunno, starting to hallucinate right now I'll have to double check tomorow.

 
     // Clears all the passively logged data from this tracker.
     virtual void Clear() = 0;
 
     // Appends all the captured entries to |out|. The ordering is undefined.
-    virtual void AppendAllEntries(EntryList* out) const = 0;
+    virtual void AppendAllEntries(ChromeNetLog::EntryList* out) const = 0;
   };
 
   // This source tracker is intended for TYPE_NONE. All entries go into a
@@ -110,12 +89,12 @@
     ~GlobalSourceTracker();
 
     // SourceTrackerInterface implementation:
-    virtual void OnAddEntry(const Entry& entry);
+    virtual void OnAddEntry(const ChromeNetLog::Entry& entry);
     virtual void Clear();
-    virtual void AppendAllEntries(EntryList* out) const;
+    virtual void AppendAllEntries(ChromeNetLog::EntryList* out) const;
 
    private:
-    typedef std::deque<Entry> CircularEntryList;
+    typedef std::deque<ChromeNetLog::Entry> CircularEntryList;
     CircularEntryList entries_;
     DISALLOW_COPY_AND_ASSIGN(GlobalSourceTracker);
   };
@@ -136,9 +115,9 @@
     virtual ~SourceTracker();
 
     // SourceTrackerInterface implementation:
-    virtual void OnAddEntry(const Entry& entry);
+    virtual void OnAddEntry(const ChromeNetLog::Entry& entry);
     virtual void Clear();
-    virtual void AppendAllEntries(EntryList* out) const;
+    virtual void AppendAllEntries(ChromeNetLog::EntryList* out) const;
 
 #ifdef UNIT_TEST
     // Helper used to inspect the current state by unit-tests.
@@ -172,7 +151,8 @@
 
     // Updates |out_info| with the information from |entry|. Returns an action
     // to perform for this map entry on completion.
-    virtual Action DoAddEntry(const Entry& entry, SourceInfo* out_info) = 0;
+    virtual Action DoAddEntry(const ChromeNetLog::Entry& entry,
+                              SourceInfo* out_info) = 0;
 
     // Removes |source_id| from |sources_|. This also releases any references
     // to dependencies held by this source.
@@ -214,7 +194,8 @@
     explicit ConnectJobTracker(PassiveLogCollector* parent);
 
    protected:
-    virtual Action DoAddEntry(const Entry& entry, SourceInfo* out_info);
+    virtual Action DoAddEntry(const ChromeNetLog::Entry& entry,
+                              SourceInfo* out_info);
    private:
     DISALLOW_COPY_AND_ASSIGN(ConnectJobTracker);
   };
@@ -228,7 +209,8 @@
     SocketTracker();
 
    protected:
-    virtual Action DoAddEntry(const Entry& entry, SourceInfo* out_info);
+    virtual Action DoAddEntry(const ChromeNetLog::Entry& entry,
+                              SourceInfo* out_info);
 
    private:
     DISALLOW_COPY_AND_ASSIGN(SocketTracker);
@@ -243,7 +225,8 @@
     explicit RequestTracker(PassiveLogCollector* parent);
 
    protected:
-    virtual Action DoAddEntry(const Entry& entry, SourceInfo* out_info);
+    virtual Action DoAddEntry(const ChromeNetLog::Entry& entry,
+                              SourceInfo* out_info);
 
    private:
     DISALLOW_COPY_AND_ASSIGN(RequestTracker);
@@ -259,7 +242,8 @@
     InitProxyResolverTracker();
 
    protected:
-    virtual Action DoAddEntry(const Entry& entry, SourceInfo* out_info);
+    virtual Action DoAddEntry(const ChromeNetLog::Entry& entry,
+                              SourceInfo* out_info);
 
    private:
     DISALLOW_COPY_AND_ASSIGN(InitProxyResolverTracker);
@@ -274,7 +258,8 @@
     SpdySessionTracker();
 
    protected:
-    virtual Action DoAddEntry(const Entry& entry, SourceInfo* out_info);
+    virtual Action DoAddEntry(const ChromeNetLog::Entry& entry,
+                              SourceInfo* out_info);
 
    private:
     DISALLOW_COPY_AND_ASSIGN(SpdySessionTracker);
@@ -289,7 +274,8 @@
     DNSRequestTracker();
 
    protected:
-    virtual Action DoAddEntry(const Entry& entry, SourceInfo* out_info);
+    virtual Action DoAddEntry(const ChromeNetLog::Entry& entry,
+                              SourceInfo* out_info);
 
    private:
     DISALLOW_COPY_AND_ASSIGN(DNSRequestTracker);
@@ -304,7 +290,8 @@
     DNSJobTracker();
 
    protected:
-    virtual Action DoAddEntry(const Entry& entry, SourceInfo* out_info);
+    virtual Action DoAddEntry(const ChromeNetLog::Entry& entry,
+                              SourceInfo* out_info);
 
    private:
     DISALLOW_COPY_AND_ASSIGN(DNSJobTracker);
@@ -320,18 +307,18 @@
                           net::NetLog::EventPhase phase,
                           net::NetLog::EventParameters* params);
 
-  // Returns the tracker to use for sources of type |source_type|, or NULL.
-  SourceTrackerInterface* GetTrackerForSourceType(
-      net::NetLog::SourceType source_type);
-
   // Clears all of the passively logged data.
   void Clear();
 
   // Fills |out| with the full list of events that have been passively
   // captured. The list is ordered by capture time.
-  void GetAllCapturedEvents(EntryList* out) const;
+  void GetAllCapturedEvents(ChromeNetLog::EntryList* out) const;
 
  private:
+  // Returns the tracker to use for sources of type |source_type|, or NULL.
+  SourceTrackerInterface* GetTrackerForSourceType_(
+      net::NetLog::SourceType source_type);
+
   FRIEND_TEST_ALL_PREFIXES(PassiveLogCollectorTest,
                            HoldReferenceToDependentSource);
   FRIEND_TEST_ALL_PREFIXES(PassiveLogCollectorTest,
@@ -356,6 +343,8 @@
   // "order" field on captured events.
   uint32 num_events_seen_;
 
+  Lock lock_;

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

So I have been thinking about this, and I don't think we need this lock.

Basically what I am thinking is, any time we enter into PassiveLogCollector we
are already guaranteed that we hold ChromeNetLog::lock_ (since we hold it while
iterating observers).

As such regular accesses are protected.

So I think all we need is a way to get at that lock for things like "Clear()"
and we are in business.

It may seem like over-optimization, but in testing some other bug (where I had
to load 50 tabs at once), I watched as we dispatched 60,000 events in a short
period of time!!! So having each operation be as fast as possible is worthwhile.

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

This strikes me as a pretty reasonable idea, actually.  Think the simplest way
to enforce that is to remove the passive_log_collector() function from
ChromeNetLog(), and force all calls to go through it.

Could make the ChromeNetLog's lock_ acquireable by its observers instead, but
think it's just safest to let ChromeNetLog handle all the lock management
itself.

+
   DISALLOW_COPY_AND_ASSIGN(PassiveLogCollector);
 };