Update NetLog to be thread safe.

chrome/browser/net/passive_log_collector.h

 // 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.
 
 #ifndef CHROME_BROWSER_NET_PASSIVE_LOG_COLLECTOR_H_
 #define CHROME_BROWSER_NET_PASSIVE_LOG_COLLECTOR_H_
 #pragma once
 
 #include <deque>
 #include <string>
@@ skipping 16 matching lines @@
 // (This is in contrast to the "active logging" which captures every single
 // network event, but requires capturing to have been enabled *prior* to
 // encountering the problem. Active capturing is enabled as long as
 // about:net-internals is open).
 //
 // The data captured by PassiveLogCollector is grouped by NetLog::Source, into
 // a SourceInfo structure. These in turn are grouped by NetLog::SourceType, and
 // 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 {
     SourceInfo();
     ~SourceInfo();
 
     // Returns the URL that corresponds with this source. This is
     // only meaningful for certain source types (URL_REQUEST, SOCKET_STREAM).
     // For the rest, it will return an empty string.
     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
     // source (for example, a url request might depend on the log items
     // for a connect job and for a socket that were bound to it.)
     SourceDependencyList dependencies;
 
     // Holds the count of how many other sources have added this as a
     // dependent source. When it is 0, it means noone has referenced it so it
     // can be deleted normally.
     int reference_count;
 
     // |is_alive| is set to false once the source has been added to the
     // tracker's graveyard (it may still be kept around due to a non-zero
     // reference_count, but it is still considered "dead").
     bool is_alive;
   };
 
   typedef std::vector<SourceInfo> SourceInfoList;
 
   // Interface for consuming a NetLog entry.
   class SourceTrackerInterface {
    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
   // circular buffer, and there is no concept of live/dead requests.
   class GlobalSourceTracker : public SourceTrackerInterface {
    public:
     GlobalSourceTracker();
     ~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);
   };
 
   // This class stores and manages the passively logged information for
   // URLRequests/SocketStreams/ConnectJobs.
   class SourceTracker : public SourceTrackerInterface {
    public:
     // Creates a SourceTracker that will track at most |max_num_sources|.
     // Up to |max_graveyard_size| unreferenced sources will be kept around
     // before deleting them for good. |parent| may be NULL, and points to
     // the owning PassiveLogCollector (it is used when adding references
     // to other sources).
     SourceTracker(size_t max_num_sources,
                   size_t max_graveyard_size,
                   PassiveLogCollector* parent);
 
     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.
     // Retuns a copy of the source infos held by the tracker.
     SourceInfoList GetAllDeadOrAliveSources(bool is_alive) const {
       SourceInfoList result;
       for (SourceIDToInfoMap::const_iterator it = sources_.begin();
            it != sources_.end(); ++it) {
         if (it->second.is_alive == is_alive)
           result.push_back(it->second);
@@ skipping 13 matching lines @@
     // kept alive at least as long as |info|.
     void AddReferenceToSourceDependency(const net::NetLog::Source& source,
                                         SourceInfo* info);
 
    private:
     typedef base::hash_map<uint32, SourceInfo> SourceIDToInfoMap;
     typedef std::deque<uint32> DeletionQueue;
 
     // 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.
     void DeleteSourceInfo(uint32 source_id);
 
     // Adds |source_id| to the FIFO queue (graveyard) for deletion.
     void AddToDeletionQueue(uint32 source_id);
 
     // Adds/Releases a reference from the source with ID |source_id|.
     // Use |offset=-1| to do a release, and |offset=1| for an addref.
@@ skipping 21 matching lines @@
 
   // Specialization of SourceTracker for handling ConnectJobs.
   class ConnectJobTracker : public SourceTracker {
    public:
     static const size_t kMaxNumSources;
     static const size_t kMaxGraveyardSize;
 
     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);
   };
 
   // Specialization of SourceTracker for handling Sockets.
   class SocketTracker : public SourceTracker {
    public:
     static const size_t kMaxNumSources;
     static const size_t kMaxGraveyardSize;
 
     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);
   };
 
   // Specialization of SourceTracker for handling URLRequest/SocketStream.
   class RequestTracker : public SourceTracker {
    public:
     static const size_t kMaxNumSources;
     static const size_t kMaxGraveyardSize;
 
     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);
   };
 
   // Specialization of SourceTracker for handling
   // SOURCE_INIT_PROXY_RESOLVER.
   class InitProxyResolverTracker : public SourceTracker {
    public:
     static const size_t kMaxNumSources;
     static const size_t kMaxGraveyardSize;
 
     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);
   };
 
   // Tracks the log entries for the last seen SOURCE_SPDY_SESSION.
   class SpdySessionTracker : public SourceTracker {
    public:
     static const size_t kMaxNumSources;
     static const size_t kMaxGraveyardSize;
 
     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);
   };
 
   // Tracks the log entries for the last seen SOURCE_HOST_RESOLVER_IMPL_REQUEST.
   class DNSRequestTracker : public SourceTracker {
    public:
     static const size_t kMaxNumSources;
     static const size_t kMaxGraveyardSize;
 
     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);
   };
 
   // Tracks the log entries for the last seen SOURCE_HOST_RESOLVER_IMPL_JOB.
   class DNSJobTracker : public SourceTracker {
    public:
     static const size_t kMaxNumSources;
     static const size_t kMaxGraveyardSize;
 
     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);
   };
 
   PassiveLogCollector();
   ~PassiveLogCollector();
 
   // Observer implementation:
   virtual void OnAddEntry(net::NetLog::EventType type,
                           const base::TimeTicks& time,
                           const net::NetLog::Source& source,
                           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,
                            HoldReferenceToDeletedSource);
 
   GlobalSourceTracker global_source_tracker_;
   ConnectJobTracker connect_job_tracker_;
   SocketTracker socket_tracker_;
   RequestTracker url_request_tracker_;
   RequestTracker socket_stream_tracker_;
   InitProxyResolverTracker init_proxy_resolver_tracker_;
   SpdySessionTracker spdy_session_tracker_;
   DNSRequestTracker dns_request_tracker_;
   DNSJobTracker dns_job_tracker_;
 
   // This array maps each NetLog::SourceType to one of the tracker instances
   // defined above. Use of this array avoid duplicating the list of trackers
   // elsewhere.
   SourceTrackerInterface* trackers_[net::NetLog::SOURCE_COUNT];
 
   // The count of how many events have flowed through this log. Used to set the
   // "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);
 };
 
 #endif  // CHROME_BROWSER_NET_PASSIVE_LOG_COLLECTOR_H_