Add SafeBrowsingNavigationObserver to listen to navigation events

chrome/browser/safe_browsing/safe_browsing_navigation_observer.h

+// Copyright 2016 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_SAFE_BROWSING_SAFE_BROWSING_NAVIGATION_OBSERVER_H_
+#define CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_NAVIGATION_OBSERVER_H_
+
+#include "base/supports_user_data.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "url/gurl.h"
+
+namespace content {
+class NavigationHandle;
+class NotificationRegistrar;

[Charlie Reis, 2016/09/23 23:14:14]

nit: You're including NavigationHandle and NotificationRegistrar above already,
so you don't need to also forward declare them here.  (Looking more closely,
maybe it makes sense to forward declare NavigationHandle and not include it, but
vice versa for NotificationRegistrar?)

[Jialiu Lin, 2016/09/27 17:51:00]

Done. 

+class RenderFrameHost;
+struct ResourceRedirectDetails;
+}
+
+namespace safe_browsing {
+
+// Struct to record the details of a navigation event for all frames.

[Charlie Reis, 2016/09/23 23:14:14]

nit: for any frame.
(A single struct won't record for all frames.)

[Jialiu Lin, 2016/09/27 17:51:00]

Done.

+// This information will be used to fill |url_chain| field in safe browsing
+// download pings.
+struct NavigationEvent {
+  NavigationEvent();
+  NavigationEvent(NavigationEvent&& nav_event);
+  NavigationEvent& operator=(NavigationEvent&& nav_event);
+  ~NavigationEvent();
+  GURL source_url;

[Charlie Reis, 2016/09/23 23:14:14]

nit: Add blank line before, and add a comment saying what this refers to. 
("Source URL" and "Source WebContents" aren't well defined concepts, and could
be confused with opener.  I think you're referring to the URL and WebContents
that caused this navigation to occur.  This could be the previous page in the
same frame, a different frame of the same page, or a different tab entirely.)

[Jialiu Lin, 2016/09/27 17:51:00]

Done.

+  int source_tab_id;  // Tab ID of the source WebContents returned by
+                      // SessionTabHelper. It is immutable for a given tab
+                      // and unique across Chrome within the current session.
+
+  GURL target_url;

[Charlie Reis, 2016/09/23 23:14:14]

Let's be clear about whether this is the original request URL (pre-redirects) or
the committed URL.  I'm not sure which is better for looking things up in the
map.

[Jialiu Lin, 2016/09/27 17:50:59]

This is the original request URL. But NavigationMap is keyed on the committed
URL. I'll add a comment near NavigationMap to make this point clearer. 

+  int target_tab_id;
+
+  int frame_id;         // Frame tree node id.
+  GURL main_frame_url;  // Main frame url of the source.
+
+  base::Time timestamp;  // Timestamp of last time this object is updated.
+
+  bool is_user_initiated;  // browser_initiated || has_user_gesture.
+  bool has_committed;
+  bool is_server_redirect;
+  GURL server_redirect_url;  // Last server redirect url in redirect chain.

[Charlie Reis, 2016/09/23 23:14:14]

Please clarify in the comment whether this is always present (i.e., if it's
equal to target_url if there are no redirects), or if it's only non-empty when
there's at least one server redirect.  (Redirect chains are typically handled
more like the former, but I'm guessing you mean the latter.)

[Jialiu Lin, 2016/09/27 17:51:00]

Yes, in case no server redirect, this field will be empty. Comment clarified.

+};
+
+// Manager class for SafeBrowsingNavigationObserver.

[Charlie Reis, 2016/09/23 23:14:14]

Can you say more about what this class manages?  This is mostly a recap of the
name.  :)

[Jialiu Lin, 2016/09/27 17:51:00]

Done.

+// TODO(jialiul): Remove base class content::NotificationObserver when
+// WebContentsObserver::DidOpenRequestedURL() covers all retargeting cases.
+class SBNavigationObserverManager

[Charlie Reis, 2016/09/23 23:14:14]

Is declaring a top-level class in the same file ok within chrome/?  I don't
think we usually allow it in content/, unless it's within an anonymous namespace
and not exposed to other code.

[Jialiu Lin, 2016/09/27 17:51:00]

Moved this manager class into a separate file. Done.

+    : public content::NotificationObserver,
+      public base::RefCountedThreadSafe<SBNavigationObserverManager> {
+ public:
+  struct GurlHash {
+    std::size_t operator()(const GURL& url) const {
+      return std::hash<std::string>()(url.spec());
+    }
+  };
+  typedef std::unordered_map<GURL, std::vector<NavigationEvent>, GurlHash>
+      NavigationMap;
+  SBNavigationObserverManager();

[Charlie Reis, 2016/09/23 23:14:14]

nit: Blank line before (after typedefs).

[Jialiu Lin, 2016/09/27 17:51:00]

Done.

+  void RecordNavigationEvent(const GURL& nav_event_key,
+                             NavigationEvent* nav_event);
+  void RecordUserGestureForWebContents(content::WebContents* web_contents,
+                                       const base::Time& timestamp);
+  void OnUserGestureConsumed(content::WebContents* web_contents,
+                             const base::Time& timestamp);
+  // Clean-ups need to be done when a WebContents gets destroyed.
+  void OnWebContentDestroyed(content::WebContents* web_contents);
+
+  // TODO(jialiul): more functions coming for managing navigation_map_.
+
+ private:
+  friend class base::RefCountedThreadSafe<SBNavigationObserverManager>;
+  friend class TestNavigationObserverManager;
+  friend class SBNavigationObserverBrowserTest;
+  friend class SBNavigationObserverTest;
+
+  typedef std::unordered_map<content::WebContents*, base::Time> UserGestureMap;
+
+  ~SBNavigationObserverManager() override;
+
+  // content::NotificationObserver:
+  void Observe(int type,
+               const content::NotificationSource& source,
+               const content::NotificationDetails& details) override;
+
+  void RecordRetargeting(const content::NotificationDetails& details);
+
+  NavigationMap* navigation_map() { return &navigation_map_; }
+
+  // navigation_map_ keeps track of all the observed navigations. This map is
+  // keyed on target url. Since the same url can be requested multiple times

[Charlie Reis, 2016/09/23 23:14:14]

Is it correct to key this on the pre-redirect URL?  (I haven't verified yet that
there's a bug here-- it depends on how we look up the key later.)

[Jialiu Lin, 2016/09/27 17:50:59]

Fixed this comments. Map actually keyed on post-redirect final resolved URL.
Thanks for catching this.

+  // across different tabs and frames, the value of this map is a vector of
+  // NavigationEvent ordered by navigation finish time. Entries in
+  // navigation_map_ will be removed if they are older than 2 minutes since
+  // their corresponding navigations finish.

[Charlie Reis, 2016/09/23 23:14:14]

Is this timer implemented yet?  I'm curious how it will work.

[Jialiu Lin, 2016/09/27 17:50:59]

Not yet. It will be in my next CL. 

+  NavigationMap navigation_map_;
+
+  // user_gesture_map_ keeps track of the timestamp of last user gesture in
+  // in each WebContents. This is used to determin if any retargeting navigation

[Charlie Reis, 2016/09/23 23:14:14]

nit: determine

This seems like it won't be accurate.  If there's a user gesture in one tab, and
later the tab automatically opens a popup with no user gesture, it will look
like there was one.  If you're ok with this for your use case, please document
it so that future code doesn't make the wrong assumption about this data.

[Jialiu Lin, 2016/09/27 17:51:00]

Safe browsing backend will aggregate download pings first, so we don't need to
get 100% correct for individual cases.  

+  // is user_initiated.
+  UserGestureMap user_gesture_map_;
+
+  content::NotificationRegistrar registrar_;
+  DISALLOW_COPY_AND_ASSIGN(SBNavigationObserverManager);
+};
+
+// Observes the navigation events for a single WebContents (both main-frame
+// and sub-frame navigations)
+class SafeBrowsingNavigationObserver : public base::SupportsUserData::Data,
+                                       public content::WebContentsObserver {
+ public:
+  static void MaybeCreateForWebContents(content::WebContents* web_contents);
+  static SafeBrowsingNavigationObserver* FromWebContents(
+      content::WebContents* web_contents);
+
+  SafeBrowsingNavigationObserver(
+      content::WebContents* contents,
+      const scoped_refptr<SBNavigationObserverManager>& manager);
+
+  ~SafeBrowsingNavigationObserver() override;
+
+ private:
+  static const char kWebContentsUserDataKey[];

[Charlie Reis, 2016/09/23 23:14:14]

nit: Let's just declare this in an anonymous namespace in the .cc file if
possible.

[Jialiu Lin, 2016/09/27 17:51:00]

Done.

+  void RecordNavigationPendingEntry(

[Charlie Reis, 2016/09/23 23:14:14]

I don't see this implemented anywhere.

[Jialiu Lin, 2016/09/27 17:50:59]

Oops, forgot to delete this one.

+      const content::NotificationSource& source,
+      const content::NotificationDetails& details);
+
+  // content::WebContentsObserver:
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override;
+  void DidRedirectNavigation(
+      content::NavigationHandle* navigation_handle) override;
+  void DidFinishNavigation(
+      content::NavigationHandle* navigation_handle) override;
+  void DidGetUserInteraction(const blink::WebInputEvent::Type type) override;
+  void WebContentsDestroyed() override;
+
+  // Map keyed on NavigationHandle* to keep track of all the ongoing navigation
+  // events. NavigationHandle pointers are owned by RenderFrameHost. Since a
+  // NavigationHandle object will be destructed after navigation is done,
+  // corresponding entry in this map will be removed from navigation_handle_map_
+  // and added to navigation_map_ at the end of DidFinishNavigation(...).
+  typedef std::unordered_map<content::NavigationHandle*, NavigationEvent>
+      NavigationHandleMap;

[Charlie Reis, 2016/09/23 23:14:14]

nit: Typedefs should be declared at the beginning of the section, before
methods.
(https://google.github.io/styleguide/cppguide.html#Declaration_Order)

[Jialiu Lin, 2016/09/27 17:50:59]

Done

+  NavigationHandleMap navigation_handle_map_;
+  scoped_refptr<SBNavigationObserverManager> manager_;
+  // If current on-going navigation triggered by a user gesture.
+  bool has_user_gesture_;

[Charlie Reis, 2016/09/23 23:14:14]

I don't think we can track this on a per-tab basis.  Several frames in the tab
can have navigations going at the same time, and each one might or might not
have a user gesture.

Why is it needed here, vs in NavigationEvent?

[Jialiu Lin, 2016/09/27 17:51:00]

Oh, sorry for the misleading comment. 
This field is set by WebContentsObservers::DidGetUserInteraction(), and
WebContentsObservers::DidGetUserInteraction() is actually called before
DidStartNavigation(). That is to say, we need a field to keep track of this
boolean before either NavigationHandle is available or NavigationEvent is
created.
And I assume that the navigation closely following DidGetUserInteraction() is
the one triggered by user gesture. (and similar to previous comment, this might
not be 100% true for individual cases, but should be right after aggregation.)

+  base::Time last_user_gesture_timestamp_;
+  DISALLOW_COPY_AND_ASSIGN(SafeBrowsingNavigationObserver);
+};
+
+}  // namespace safe_browsing
+
+#endif  // CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_NAVIGATION_OBSERVER_H_