PlzNavigate: properly set the initiator of the navigation

content/browser/frame_host/navigation_request.cc

Index: content/browser/frame_host/navigation_request.cc
diff --git a/content/browser/frame_host/navigation_request.cc b/content/browser/frame_host/navigation_request.cc
index 16cb92373be9fb64b21773abf7a3ca6e8ae48daf..224bf85fb9886968ecdb95c9153869bd9aa6dd72 100644
--- a/content/browser/frame_host/navigation_request.cc
+++ b/content/browser/frame_host/navigation_request.cc
@@ -86,6 +86,10 @@ std::unique_ptr<NavigationRequest> NavigationRequest::CreateBrowserInitiated(
   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,
@@ -94,7 +98,7 @@ std::unique_ptr<NavigationRequest> NavigationRequest::CreateBrowserInitiated(
                             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(),
@@ -188,8 +192,8 @@ NavigationRequest::NavigationRequest(
       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() {