Add management related code to SafeBrowsingNavigationObserverManager

chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h

Index: chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h
diff --git a/chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h b/chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h
index f66172fe4eaf510de94361f484f9a1640cf77428..8218897ddb8962392583b23338565bce26fd2420 100644
--- a/chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h
+++ b/chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h
@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_NAVIGATION_OBSERVER_MANAGER_H_
 #define CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_NAVIGATION_OBSERVER_MANAGER_H_
 
+#include "chrome/common/safe_browsing/csd.pb.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -17,7 +18,7 @@ struct NavigationEvent;
 struct ResolvedIPAddress;
 
 // Manager class for SafeBrowsingNavigationObserver, which is in charge of
-// cleaning up stale navigation events, and identifing landing page/landing
+// cleaning up stale navigation events, and identifying landing page/landing
 // referrer for a specific download.
 // TODO(jialiul): For now, SafeBrowsingNavigationObserverManager also listens to
 // NOTIFICATION_RETARGETING as a way to detect cross frame/tab navigation.
@@ -27,6 +28,17 @@ class SafeBrowsingNavigationObserverManager
     : public content::NotificationObserver,
       public base::RefCountedThreadSafe<SafeBrowsingNavigationObserverManager> {
  public:
+  // For UMA histogram counting. Do NOT change order.
+  enum AttributionResult {

[Nathan Parker, 2016/12/05 22:21:24]

This can be private.

[Jialiu Lin, 2016/12/06 23:10:06]

Since I'm going to make AttributionResult IdentifyReferrerChain(...) public,
I'll probably keep this enum public too.(such that other classes e.g.
SafeBrowsingService can access this enum). 

+    SUCCESS,                   // Identified referrer chain is not empty.

[Nathan Parker, 2016/12/05 22:21:24]

Assign numbers to these so you can verify they match those in UMA.  I'd start
with 1 to detect when it's accidentally set to the default.

[Jialiu Lin, 2016/12/06 23:10:06]

Done.

+    SUCCESS_LANDING_PAGE,      // Successfully identified landing page.
+    SUCCESS_LANDING_REFERRER,  // Successfully identified landing referrer.
+    INVALID_URL,
+    NAVIGATION_EVENT_NOT_FOUND,
+
+    // Always at the end
+    ATTRIBUTION_FAILURE_TYPE_MAX
+  };
   // Helper function to check if user gesture is older than
   // kUserGestureTTLInSecond.
   static bool IsUserGestureExpired(const base::Time& timestamp);
@@ -49,8 +61,14 @@ class SafeBrowsingNavigationObserverManager
   void RecordHostToIpMapping(const std::string& host, const std::string& ip);
   // Clean-ups need to be done when a WebContents gets destroyed.
   void OnWebContentDestroyed(content::WebContents* web_contents);
-
-  // TODO(jialiul): more functions are coming for managing navigation_map_.
+  // Remove all the observed NavigationEvents, user gestures, and resolved IP
+  // addresses that are older than kNavigationFootPrintTTLInSecond.
+  void CleanUpStaleNavigationFootPrints();

[Nathan Parker, 2016/12/05 22:21:24]

nit: I'd say Footprint (lowercase P) since it's one word

[Jialiu Lin, 2016/12/06 23:10:06]

Oops. my spelling... Thanks for catching this.

+  // Identify and add referrer chain to ClientDownloadRequest proto.
+  void AddReferrerChainToClientDownloadRequest(
+      const GURL& download_url,
+      content::WebContents* source_contents,
+      ClientDownloadRequest* request);
 
  private:
   friend class base::RefCountedThreadSafe<
@@ -58,6 +76,12 @@ class SafeBrowsingNavigationObserverManager
   friend class TestNavigationObserverManager;
   friend class SBNavigationObserverBrowserTest;
   friend class SBNavigationObserverTest;
+  FRIEND_TEST_ALL_PREFIXES(SBNavigationObserverTest,
+                           TestCleanUpStaleNavigationEvents);
+  FRIEND_TEST_ALL_PREFIXES(SBNavigationObserverTest,
+                           TestCleanUpStaleUserGestures);
+  FRIEND_TEST_ALL_PREFIXES(SBNavigationObserverTest,
+                           TestCleanUpStaleIPAddresses);
 
   struct GurlHash {
     std::size_t operator()(const GURL& url) const {
@@ -84,6 +108,30 @@ class SafeBrowsingNavigationObserverManager
 
   HostToIpMap* host_to_ip_map() { return &host_to_ip_map_; }
 
+  void CleanUpNavigationEvents();

[Nathan Parker, 2016/12/05 22:21:24]

Add some comments as to what these do.

[Jialiu Lin, 2016/12/06 23:10:06]

Done.

+
+  void CleanUpUserGestures();
+
+  void CleanUpIpAddresses();
+
+  bool IsCleanUpScheduled() const;
+
+  void ScheduleNextCleanUpAfterInterval(base::TimeDelta interval);
+
+  NavigationEvent* FindNavigationEvent(const GURL& target_url,
+                                       int target_tab_id);
+  void AddToReferrerChain(

[Nathan Parker, 2016/12/05 22:21:24]

An idea: Would it be cleaner to have the code that fills in the
ClientDownloadRequest be external to this class, and have it just use public
methods?  That splits it apart into, a) code the tracks attributions (this
class) and, b) code that maps it to other structs.  We could then add more
functions for filling in ClientSafeBrowsingReqportRequest, without adding
complexity here.

[Jialiu Lin, 2016/12/06 23:10:06]

I'm thinking about move ReferrerChainEntry message out of ClientDownloadRequest
and directly put it under safe_browsing namespace, such that
ClientSafeBrowsingReportRequest can also reuse this message more easily later. 
Also make IdentifyReferrerChain() public, and add a todo to
AddReferrerChainToClientDownloadRequest(..) function. The later one will be
moved to download protection service when it is ready.  

[Nathan Parker, 2016/12/06 23:31:32]

SGTM

+      std::vector<safe_browsing::ClientDownloadRequest::ReferrerChainEntry>*
+          referrer_chain,
+      NavigationEvent* nav_event,
+      ClientDownloadRequest::ReferrerChainEntry::URLType type);
+
+  AttributionResult IdentifyReferrerChain(
+      const GURL& target_url,
+      int target_tab_id,  // -1 if tab id is not valid
+      int user_gesture_count_max,
+      std::vector<ClientDownloadRequest::ReferrerChainEntry>* referrer_chain);
+
   // navigation_map_ keeps track of all the observed navigations. This map is
   // keyed on the resolved request url. In other words, in case of server
   // redirects, its key is the last server redirect url, otherwise, it is the
@@ -108,6 +156,8 @@ class SafeBrowsingNavigationObserverManager
 
   content::NotificationRegistrar registrar_;
 
+  base::OneShotTimer cleanup_timer_;
+
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingNavigationObserverManager);
 };
 }  // namespace safe_browsing