PlzNavigate: properly set the initiator of the navigation

content/browser/frame_host/navigation_request.cc

 // Copyright 2014 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_request.h"
 
 #include <utility>
 
 #include "content/browser/devtools/render_frame_devtools_agent_host.h"
 #include "content/browser/frame_host/frame_tree.h"
@@ skipping 68 matching lines @@
   net::HttpRequestHeaders headers;
   headers.AddHeadersFromString(entry.extra_headers());
   headers.SetHeaderIfMissing(net::HttpRequestHeaders::kUserAgent,
                              GetContentClient()->GetUserAgent());
 
   // Fill POST data in the request body.
   scoped_refptr<ResourceRequestBodyImpl> request_body;
   if (frame_entry.method() == "POST")
     request_body = frame_entry.GetPostData();
 
+  url::Origin initiator =
+      frame_tree_node->IsMainFrame()
+          ? url::Origin(dest_url)
+          : frame_tree_node->frame_tree()->root()->current_origin();

[Mike West, 2016/06/27 12:10:08]

I don't believe that this is correct, as it's always going to pull the main
frame's origin as opposed to the origin that actually initiated the navigation.
If A is nested in B, navigations that A initiates should have A as their
initiator. This code would give them B.

That said, I don't think the previous code (that I wrote, sorry!) was correct
either, as it picked the wrong initiator in the opposite case (where the parent
navigates the child). I didn't write enough tests, obviously.

I think we're actually going to have to do this when triggering the navigation
in Blink to have real assurance that we're doing the right thing. But I can do
that in a subsequent CL, that's not your problem.

[clamy, 2016/06/27 12:33:29]

This is only triggered by browser-initiated navigations (in which we have no
info on initiator). In practice, we would only reach this for subframes during a
history navigation. Unless we have the information in history, I don't really
have a better choice than using the top document origin. For renderer-initiated
navigations (ie all other subframe navigations), we rely on Blink's information
that is transmitted in the BeginNavigationparams.

If this is problematic for browser-initiated navigations, I think it is also the
case in the current architecture, as I couldn't find any info about initiator in
either OpenURl or NavigationEntry/FrameNavigationEntry.

[Mike West, 2016/06/27 12:55:13]

Ah, right. I missed the change in `content/renderer/render_frame_impl.cc` where
you're populating that data. I think there still might be some work to do there
to verify that things are set correctly on the Blink side, but that's work for a
separate CL.

[carlosk, 2016/06/28 12:03:17]

nit: should a TODO be added for that too?

   std::unique_ptr<NavigationRequest> navigation_request(new NavigationRequest(
       frame_tree_node, entry.ConstructCommonNavigationParams(
                            frame_entry, request_body, dest_url, dest_referrer,
                            navigation_type, lofi_state, navigation_start),
       BeginNavigationParams(headers.ToString(),
                             LoadFlagFromNavigationType(navigation_type),
                             false,  // has_user_gestures
                             false,  // skip_service_worker
-                            REQUEST_CONTEXT_TYPE_LOCATION),
+                            REQUEST_CONTEXT_TYPE_LOCATION, initiator),
       entry.ConstructRequestNavigationParams(
           frame_entry, is_same_document_history_load,
           frame_tree_node->has_committed_real_load(),
           controller->GetPendingEntryIndex() == -1,
           controller->GetIndexOfEntry(&entry),
           controller->GetLastCommittedEntryIndex(),
           controller->GetEntryCount()),
       true, &frame_entry, &entry));
   return navigation_request;
 }
@@ skipping 73 matching lines @@
   // and verifies that all origins are PSL-matches (and special-cases extension
   // URLs).
   const GURL& first_party_for_cookies =
       frame_tree_node->IsMainFrame()
           ? common_params.url
           : frame_tree_node->frame_tree()->root()->current_url();
   bool parent_is_main_frame = !frame_tree_node->parent() ?
       false : frame_tree_node->parent()->IsMainFrame();
   info_.reset(new NavigationRequestInfo(
       common_params, begin_params, first_party_for_cookies,
-      frame_tree_node->current_origin(), frame_tree_node->IsMainFrame(),
-      parent_is_main_frame, frame_tree_node->frame_tree_node_id()));
+      frame_tree_node->IsMainFrame(), parent_is_main_frame,
+      frame_tree_node->frame_tree_node_id()));
 }
 
 NavigationRequest::~NavigationRequest() {
 }
 
 void NavigationRequest::BeginNavigation() {
   DCHECK(!loader_);
   DCHECK(state_ == NOT_STARTED || state_ == WAITING_FOR_RENDERER_RESPONSE);
   state_ = STARTED;
   RenderFrameDevToolsAgentHost::OnBeforeNavigation(navigation_handle_.get());
@@ skipping 255 matching lines @@
       browser_context, navigating_frame_host->GetSiteInstance());
   DCHECK(partition);
 
   ServiceWorkerContextWrapper* service_worker_context =
       static_cast<ServiceWorkerContextWrapper*>(
           partition->GetServiceWorkerContext());
   navigation_handle_->InitServiceWorkerHandle(service_worker_context);
 }
 
 }  // namespace content