Add a NavigationThrottle to the public content/ interface

content/browser/frame_host/navigation_handle_impl.cc

 // Copyright 2015 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.
 
 #include "content/browser/frame_host/navigation_handle_impl.h"
 
 #include "content/browser/frame_host/navigator_delegate.h"
 #include "net/url_request/redirect_info.h"
 
 namespace content {
 
 // static
 scoped_ptr<NavigationHandleImpl> NavigationHandleImpl::Create(
     const GURL& url,
+    const GURL& validated_url,
     const bool is_main_frame,
     NavigatorDelegate* delegate) {
   return scoped_ptr<NavigationHandleImpl>(
-      new NavigationHandleImpl(url, is_main_frame, delegate));
+      new NavigationHandleImpl(url, validated_url, is_main_frame, delegate));
 }
 
 NavigationHandleImpl::NavigationHandleImpl(const GURL& url,
+                                           const GURL& validated_url,
                                            const bool is_main_frame,
                                            NavigatorDelegate* delegate)
     : url_(url),
+      validated_url_(validated_url),
+      is_main_frame_(is_main_frame),
+      is_post_(false),
+      has_user_gesture_(false),
+      transition_(ui::PAGE_TRANSITION_LINK),
+      is_external_protocol_(false),
       net_error_code_(net::OK),
       state_(DID_START),
-      is_main_frame_(is_main_frame),
       is_transferring_(false),
       delegate_(delegate) {
   delegate_->DidStartNavigation(this);
 }
 
 NavigationHandleImpl::~NavigationHandleImpl() {
   delegate_->DidFinishNavigation(this);
 }
 
 const GURL& NavigationHandleImpl::GetURL() const {
   return url_;
 }
 
+const GURL& NavigationHandleImpl::GetValidatedURL() const {
+  return validated_url_;
+}
+
+bool NavigationHandleImpl::IsInMainFrame() const {
+  return is_main_frame_;
+}
+
+bool NavigationHandleImpl::IsPost() const {
+  DCHECK(state_ != DID_START)

[nasko, 2015/08/31 23:25:15]

nit: CHECK? Also in the methods below.

[Avi (use Gerrit), 2015/09/01 16:38:47]

DCHECK is the usual Chromium style; why are you proposing CHECK?

[davidben, 2015/09/01 21:55:18]

I think this line doesn't have enough people commenting on it, so I'll add:

Nit: DCHECK_NE (ditto below)

[nasko, 2015/09/01 22:08:12]

This is not a codepath that the renderer can induce us to go down, so it is
purely bug in our implementation. As such, using a CHECK will allow us to get
crash reports with status of what we got wrong (stack trace and some registers).
DCHECKs should be used when a renderer process can cause us to hit it, otherwise
we will create a denial-of-service condition where malicious page can
continually crash the browser.

[clamy, 2015/09/03 15:30:51]

I think CHECKS are ok in that case, but apart from a codepath the renderer can
induce us to go down, there is another case where they should not be used. If a
codepath is critical to performance, converting a DCHECK in a CHECK can have an
impact on the user experience.

[nasko, 2015/09/04 23:36:49]

In general this is a great point. It must be a really hot codepath for a single
comparison statement to have an user visible impact.

+      << "This accessor should not be called before the request is started.";
+  return is_post_;
+}
+
+const Referrer& NavigationHandleImpl::GetSanitizedReferrer() const {
+  DCHECK(state_ != DID_START)
+      << "This accessor should not be called before the request is started.";
+  return sanitized_referrer_;
+}
+
+bool NavigationHandleImpl::HasUserGesture() const {
+  DCHECK(state_ != DID_START)
+      << "This accessor should not be called before the request is started.";
+  return has_user_gesture_;
+}
+
+ui::PageTransition NavigationHandleImpl::GetPageTransition() const {
+  DCHECK(state_ != DID_START)
+      << "This accessor should not be called before the request is started.";
+  return transition_;
+}
+
+bool NavigationHandleImpl::IsExternalProtocol() const {
+  DCHECK(state_ != DID_START)
+      << "This accessor should not be called before the request is started.";
+  return is_external_protocol_;
+}
+
 net::Error NavigationHandleImpl::GetNetErrorCode() const {
   return net_error_code_;
 }
 
-bool NavigationHandleImpl::IsInMainFrame() const {
-  return is_main_frame_;
-}
-
 bool NavigationHandleImpl::HasCommittedDocument() const {
   return state_ == DID_COMMIT;
 }
 
 bool NavigationHandleImpl::HasCommittedErrorPage() const {
   return state_ == DID_COMMIT_ERROR_PAGE;
 }
 
+void NavigationHandleImpl::RegisterThrottle(
+    scoped_ptr<NavigationThrottle> navigation_throttle) {
+  throttles_.push_back(navigation_throttle.Pass());
+}
+
+NavigationThrottle::ThrottleCheckResult NavigationHandleImpl::WillStartRequest(
+    bool is_post,
+    const Referrer& sanitized_referrer,
+    bool has_user_gesture,
+    ui::PageTransition transition,
+    bool is_external_protocol) {
+  // Update the navigation parameters.
+  is_post_ = is_post;
+  sanitized_referrer_ = sanitized_referrer;
+  has_user_gesture_ = has_user_gesture;
+  transition_ = transition;
+  is_external_protocol_ = is_external_protocol;
+
+  state_ = WILL_SEND_REQUEST;
+
+  // Register the navigation throttles.
+  delegate_->AddNavigationThrottles(this);
+
+  // Have each throttle be notified of the request.

[davidben, 2015/09/01 21:55:17]

Nit: Perhaps "Notify each throttle of the request."? Slightly shorter or
something.

[clamy, 2015/09/03 15:30:51]

Done.

+  for (auto it = throttles_.begin(); it != throttles_.end(); ++it) {

[davidben, 2015/09/01 21:55:18]

(Is it possible to use a range-based for loop here, or does it being a
ScopedVector cause you troubles?)

[clamy, 2015/09/03 15:30:51]

Done (it's been so long since I wrote a loop in Chromium code that I did not
remember we can now use this C++11 feature).

+    NavigationThrottle::ThrottleCheckResult result = (*it)->WillStartRequest();
+    if (result == NavigationThrottle::CANCEL_AND_IGNORE)
+      return NavigationThrottle::CANCEL_AND_IGNORE;
+  }
+  return NavigationThrottle::PROCEED;
+}
+
+NavigationThrottle::ThrottleCheckResult
+NavigationHandleImpl::WillRedirectRequest(const GURL& new_url,
+                                          const GURL& new_validated_url,
+                                          bool new_method_is_post,
+                                          const GURL& new_referrer_url,
+                                          bool new_is_external_protocol) {
+  // Update the navigation parameters.
+  url_ = new_url;
+  validated_url_ = new_validated_url;
+  is_post_ = new_method_is_post;
+  sanitized_referrer_.url = new_referrer_url;

[carlosk, 2015/08/28 16:40:24]

Why setting this member URL here when the next line replaces the whole value?

[clamy, 2015/09/03 15:30:51]

Because it needs to be updated before being sanitized (it will precisely look at
that url). The alternative is:
sanitized_referrer_ = Referrer::SanitizeForRequest(url_,
Referrer(new_referrer_url, sanitized_referrer_.policy)), which I don't find
tremendously clearer.

+  sanitized_referrer_ = Referrer::SanitizeForRequest(url_, sanitized_referrer_);

[davidben, 2015/09/01 21:55:18]

I'm a little puzzled by this line. Wouldn't the referrer come pre-sanitized by
the loader stack already? You're just trying to report on what the loader stack
decided to use. Or is the problem that this hasn't happened yet and you need to
repeat that logic? (I'm not that familiar with referrers.)

[clamy, 2015/09/03 15:30:51]

I based myself on the fact that the ResourceThrottle I was converting did
sanitize Referrers, so I ported that functionality over. Since I'm not familiar
with referrers, I did not want to miss a case where the Referrer would not be
properly sanitized.

+  is_external_protocol_ = new_is_external_protocol;
+
+  state_ = WILL_REDIRECT_REQUEST;

[davidben, 2015/09/01 21:55:18]

Is anything actually sensitive to this state? You don't have asynchronous
throttles, so you don't need to keep track of exactly where you are like that.

[clamy, 2015/09/03 15:30:51]

Done. Note that we may have to reintroduce it if throttles become asynchronous,
which may happen).

+
+  // Have each throttle be notified of the request.
+  for (auto it = throttles_.begin(); it != throttles_.end(); ++it) {
+    NavigationThrottle::ThrottleCheckResult result =
+        (*it)->WillRedirectRequest();
+    if (result == NavigationThrottle::CANCEL_AND_IGNORE)
+      return NavigationThrottle::CANCEL_AND_IGNORE;
+  }
+  return NavigationThrottle::PROCEED;
+}
+
 void NavigationHandleImpl::DidRedirectNavigation(const GURL& new_url) {
   url_ = new_url;
   delegate_->DidRedirectNavigation(this);
 }
 
 void NavigationHandleImpl::DidCommitNavigation() {
   state_ = net_error_code_ == net::OK ? DID_COMMIT : DID_COMMIT_ERROR_PAGE;
   delegate_->DidCommitNavigation(this);
 }
 
 }  // namespace content