Set user_gesture bit at NavigationHandle creation time.

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 <utility>
 
 #include "base/debug/dump_without_crashing.h"
 #include "base/logging.h"
@@ skipping 33 matching lines @@
 
 // static
 std::unique_ptr<NavigationHandleImpl> NavigationHandleImpl::Create(
     const GURL& url,
     FrameTreeNode* frame_tree_node,
     bool is_renderer_initiated,
     bool is_same_page,
     bool is_srcdoc,
     const base::TimeTicks& navigation_start,
     int pending_nav_entry_id,
+    bool has_user_gesture,
     bool started_from_context_menu) {
   return std::unique_ptr<NavigationHandleImpl>(new NavigationHandleImpl(
       url, frame_tree_node, is_renderer_initiated, is_same_page, is_srcdoc,
-      navigation_start, pending_nav_entry_id, started_from_context_menu));
+      navigation_start, pending_nav_entry_id, has_user_gesture,
+      started_from_context_menu));
 }
 
 NavigationHandleImpl::NavigationHandleImpl(
     const GURL& url,
     FrameTreeNode* frame_tree_node,
     bool is_renderer_initiated,
     bool is_same_page,
     bool is_srcdoc,
     const base::TimeTicks& navigation_start,
     int pending_nav_entry_id,
+    bool has_user_gesture,
     bool started_from_context_menu)
     : url_(url),
-      has_user_gesture_(false),
+      has_user_gesture_(has_user_gesture),
       transition_(ui::PAGE_TRANSITION_LINK),
       is_external_protocol_(false),
       net_error_code_(net::OK),
       render_frame_host_(nullptr),
       is_renderer_initiated_(is_renderer_initiated),
       is_same_page_(is_same_page),
       is_srcdoc_(is_srcdoc),
       was_redirected_(false),
       connection_info_(net::HttpResponseInfo::CONNECTION_INFO_UNKNOWN),
       original_url_(url),
@@ skipping 102 matching lines @@
   return method_ == "POST";
 }
 
 const Referrer& NavigationHandleImpl::GetReferrer() {
   CHECK_NE(INITIAL, state_)
       << "This accessor should not be called before the request is started.";
   return sanitized_referrer_;
 }
 
 bool NavigationHandleImpl::HasUserGesture() {
-  CHECK_NE(INITIAL, state_)
-      << "This accessor should not be called before the request is started.";
   return has_user_gesture_;
 }
 
 ui::PageTransition NavigationHandleImpl::GetPageTransition() {
   CHECK_NE(INITIAL, state_)
       << "This accessor should not be called before the request is started.";
   return transition_;
 }
 
 bool NavigationHandleImpl::IsExternalProtocol() {
@@ skipping 76 matching lines @@
 
 void NavigationHandleImpl::RegisterThrottleForTesting(
     std::unique_ptr<NavigationThrottle> navigation_throttle) {
   throttles_.push_back(std::move(navigation_throttle));
 }
 
 NavigationThrottle::ThrottleCheckResult
 NavigationHandleImpl::CallWillStartRequestForTesting(
     bool is_post,
     const Referrer& sanitized_referrer,
-    bool has_user_gesture,
     ui::PageTransition transition,
     bool is_external_protocol) {
   NavigationThrottle::ThrottleCheckResult result = NavigationThrottle::DEFER;
 
   scoped_refptr<ResourceRequestBodyImpl> resource_request_body;
   std::string method = "GET";
   if (is_post) {
     method = "POST";
 
     std::string body = "test=body";
     resource_request_body = new ResourceRequestBodyImpl();
     resource_request_body->AppendBytes(body.data(), body.size());
   }
 
   WillStartRequest(method, resource_request_body, sanitized_referrer,
-                   has_user_gesture, transition, is_external_protocol,
+                   transition, is_external_protocol,
                    REQUEST_CONTEXT_TYPE_LOCATION,
                    base::Bind(&UpdateThrottleCheckResult, &result));
 
   // Reset the callback to ensure it will not be called later.
   complete_callback_.Reset();
   return result;
 }
 
 NavigationThrottle::ThrottleCheckResult
 NavigationHandleImpl::CallWillRedirectRequestForTesting(
@@ skipping 72 matching lines @@
     ServiceWorkerContextWrapper* service_worker_context) {
   DCHECK(IsBrowserSideNavigationEnabled());
   service_worker_handle_.reset(
       new ServiceWorkerNavigationHandle(service_worker_context));
 }
 
 void NavigationHandleImpl::WillStartRequest(
     const std::string& method,
     scoped_refptr<content::ResourceRequestBodyImpl> resource_request_body,
     const Referrer& sanitized_referrer,
-    bool has_user_gesture,
     ui::PageTransition transition,
     bool is_external_protocol,
     RequestContextType request_context_type,
     const ThrottleChecksFinishedCallback& callback) {
   // |method != "POST"| should imply absence of |resource_request_body|.
   if (method != "POST" && resource_request_body) {
     NOTREACHED();
     resource_request_body = nullptr;
   }
 
   // Update the navigation parameters.
   method_ = method;
   if (method_ == "POST")
     resource_request_body_ = resource_request_body;
   sanitized_referrer_ = sanitized_referrer;
-  has_user_gesture_ = has_user_gesture;
   transition_ = transition;
   is_external_protocol_ = is_external_protocol;
   request_context_type_ = request_context_type;
   state_ = WILL_SEND_REQUEST;
   complete_callback_ = callback;
 
   RegisterNavigationThrottles();
 
   if (IsBrowserSideNavigationEnabled())
     navigation_ui_data_ = GetDelegate()->GetNavigationUIData(this);
@@ skipping 89 matching lines @@
 }
 
 void NavigationHandleImpl::DidCommitNavigation(
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params,
     bool same_page,
     RenderFrameHostImpl* render_frame_host) {
   DCHECK(!render_frame_host_ || render_frame_host_ == render_frame_host);
   DCHECK_EQ(frame_tree_node_, render_frame_host->frame_tree_node());
   CHECK_EQ(url_, params.url);
 
   method_ = params.method;

[clamy, 2016/11/17 14:30:04]

I think it'd be nice to DCHECK here that the gesture we got from the commit
message is the same that we set at creation time. This way we can catch
unexpected bugs.

[Bryan McQuade, 2016/11/19 20:21:38]

I agree - this seems like something we should do. I added this, but the DCHECK
unfortunately fires in many tests, as well as at runtime. It seems that the
gesture tracking logic is already a bit buggy. I debugged one such issue in bug
667070. I encountered other cases as well but did not debug those.

I created a minimal patch that just adds this DCHECK for same page navs for the
existing gesture logic (without the rest of this patch) and you can see that
many tests fail as a result: https://codereview.chromium.org/2512043003.

It seems like we may need to invest a bit more time in fixing the existing
gesture tracking logic across blink and content first. I unfortunately don't
have cycles to do that - it looks like a larger project.

I'm also open to moving this patch forward w/o adding the DCHECK, but I am very
happy to put it on hold if you prefer to get the existing gesture logic sorted
out first.

I've removed the DCHECK from this patch for now. Let me know what you think
makes sense as a next step.

[clamy, 2016/11/21 16:50:46]

Acknowledged. Could you add a TODO(clamy) to check that it remains the same, and
link to a bug number? Thanks!

[Bryan McQuade, 2016/11/22 14:13:03]

Done.

-  has_user_gesture_ = (params.gesture == NavigationGestureUser);
   transition_ = params.transition;
   render_frame_host_ = render_frame_host;
 
   // If an error page reloads, net_error_code might be 200 but we still want to
   // count it as an error page.
   if (params.base_url.spec() == kUnreachableWebDataURL ||
       net_error_code_ != net::OK) {
     state_ = DID_COMMIT_ERROR_PAGE;
   } else {
     state_ = DID_COMMIT;
@@ skipping 229 matching lines @@
     throttles_to_register.push_back(std::move(clear_site_data_throttle));
 
   if (throttles_to_register.size() > 0) {
     throttles_.insert(throttles_.begin(), throttles_to_register.begin(),
                       throttles_to_register.end());
     throttles_to_register.weak_clear();
   }
 }
 
 }  // namespace content