OOPIF: Fix willSendRequest in A-B-A nested case.

content/renderer/render_frame_impl.cc

 // Copyright 2013 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/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 
 #include "base/auto_reset.h"
@@ skipping 3081 matching lines @@
       // TODO(nasko): When the top-level frame is remote, there is no document.
       // This is broken and should be fixed to propagate the first party.
       WebFrame* top = frame->top();
       if (top->isWebLocalFrame()) {
         request.setFirstPartyForCookies(
             frame->top()->document().firstPartyForCookies());
       }
     }
   }
 
-  WebFrame* top_frame = frame->top();
-  // TODO(nasko): Hack around asking about top-frame data source. This means
-  // for out-of-process iframes we are treating the current frame as the
-  // top-level frame, which is wrong.
-  if (!top_frame || top_frame->isWebRemoteFrame())
-    top_frame = frame;
-  WebDataSource* provisional_data_source = top_frame->provisionalDataSource();
-  WebDataSource* top_data_source = top_frame->dataSource();
+  WebDataSource* provisional_data_source = frame->provisionalDataSource();
   WebDataSource* data_source =
-      provisional_data_source ? provisional_data_source : top_data_source;
+      provisional_data_source ? provisional_data_source : frame->dataSource();
 
   DocumentState* document_state = DocumentState::FromDataSource(data_source);
   DCHECK(document_state);
   InternalDocumentStateData* internal_data =
       InternalDocumentStateData::FromDocumentState(document_state);
   NavigationStateImpl* navigation_state =
       static_cast<NavigationStateImpl*>(document_state->navigation_state());
   ui::PageTransition transition_type = navigation_state->GetTransitionType();
-  WebDataSource* frame_ds = frame->provisionalDataSource();

[Charlie Reis, 2015/07/09 17:40:11]

Noticed this was redundant now.

-  if (frame_ds && frame_ds->isClientRedirect()) {
+  if (provisional_data_source && provisional_data_source->isClientRedirect()) {
     transition_type = ui::PageTransitionFromInt(
         transition_type | ui::PAGE_TRANSITION_CLIENT_REDIRECT);
   }
 
   GURL request_url(request.url());
   GURL new_url;
   if (GetContentClient()->renderer()->WillSendRequest(
           frame,
           transition_type,
           request_url,
@@ skipping 47 matching lines @@
 
   // Add an empty HTTP origin header for non GET methods if none is currently
   // present.
   request.addHTTPOriginIfNeeded(WebString());
 
   // Attach |should_replace_current_entry| state to requests so that, should
   // this navigation later require a request transfer, all state is preserved
   // when it is re-created in the new process.
   bool should_replace_current_entry = false;
   if (navigation_state->IsContentInitiated()) {
     should_replace_current_entry = data_source->replacesCurrentHistoryItem();

[Charlie Reis, 2015/07/09 17:40:11]

This adds to my confidence in this change, because it was (unsurprisingly) wrong
when we got the navigation_state from the top frame.

I had to make one more change to NavigateInternal to get cross-process
location.replace in a subframe to work, but now it's behaving.

   } else {
     // If the navigation is browser-initiated, the NavigationState contains the
     // correct value instead of the WebDataSource.
     //
     // TODO(davidben): Avoid this awkward duplication of state. See comment on
     // NavigationState::should_replace_current_entry().
     should_replace_current_entry =
         navigation_state->start_params().should_replace_current_entry;
   }
 
@@ skipping 33 matching lines @@
   extra_data->set_is_main_frame(!parent);
   extra_data->set_frame_origin(
       GURL(frame->document().securityOrigin().toString()));
   extra_data->set_parent_is_main_frame(parent && !parent->parent());
   extra_data->set_parent_render_frame_id(parent_routing_id);
   extra_data->set_allow_download(
       navigation_state->common_params().allow_download);
   extra_data->set_transition_type(transition_type);
   extra_data->set_should_replace_current_entry(should_replace_current_entry);
   extra_data->set_transferred_request_child_id(
       navigation_state->start_params().transferred_request_child_id);

[Charlie Reis, 2015/07/09 17:40:11]

(This was what motivated this CL in the first place, fixing the A-B-A case.)

   extra_data->set_transferred_request_request_id(
       navigation_state->start_params().transferred_request_request_id);
   extra_data->set_service_worker_provider_id(provider_id);
   extra_data->set_stream_override(stream_override.Pass());
   request.setExtraData(extra_data);
 
-  DocumentState* top_document_state =
-      DocumentState::FromDataSource(top_data_source);
-  if (top_document_state) {
-    // TODO(gavinp): separate out prefetching and prerender field trials
-    // if the rel=prerender rel type is sticking around.
-    if (request.requestContext() == WebURLRequest::RequestContextPrefetch)
-      top_document_state->set_was_prefetcher(true);
+  // TODO(creis): Update prefetching to work with out-of-process iframes.
+  WebFrame* top_frame = frame->top();
+  if (top_frame && top_frame->isWebLocalFrame()) {
+    DocumentState* top_document_state =
+        DocumentState::FromDataSource(top_frame->dataSource());
+    if (top_document_state) {
+      // TODO(gavinp): separate out prefetching and prerender field trials
+      // if the rel=prerender rel type is sticking around.
+      if (request.requestContext() == WebURLRequest::RequestContextPrefetch)
+        top_document_state->set_was_prefetcher(true);
+    }
   }
 
   // This is an instance where we embed a copy of the routing id
   // into the data portion of the message. This can cause problems if we
   // don't register this id on the browser side, since the download manager
   // expects to find a RenderViewHost based off the id.
   request.setRequestorID(render_view_->GetRoutingID());
   request.setHasUserGesture(WebUserGestureIndicator::isProcessingUserGesture());
 
   if (!navigation_state->start_params().extra_headers.empty()) {
@@ skipping 1146 matching lines @@
   if (is_reload && !render_view_->history_controller()->GetCurrentEntry()) {
     // We cannot reload if we do not have any history state.  This happens, for
     // example, when recovering from a crash.
     is_reload = false;
     cache_policy = WebURLRequest::ReloadIgnoringCacheData;
   }
 
   pending_navigation_params_.reset(
       new NavigationParams(common_params, start_params, request_params));
 
-  // Create parameters for a standard navigation.
-  blink::WebFrameLoadType load_type = blink::WebFrameLoadType::Standard;
+  // Create parameters for a standard navigation, indicating whether it should
+  // replace the current NavigationEntry.
+  blink::WebFrameLoadType load_type =
+      start_params.should_replace_current_entry ?
+          blink::WebFrameLoadType::RedirectWithLockedBackForwardList :

[Charlie Reis, 2015/07/09 17:40:11]

This is the fix for https://crbug.com/317872, and it works for both main frames
and subframes.

+          blink::WebFrameLoadType::Standard;
   bool should_load_request = false;
   WebHistoryItem item_for_history_navigation;
   WebURLRequest request = CreateURLRequestForNavigation(
       common_params, stream_params.Pass(), frame_->isViewSourceModeEnabled());
 
   // PlzNavigate: Make sure that Blink's loader will not try to use browser side
   // navigation for this request (since it already went to the browser).
   if (browser_side_navigation)
     request.setCheckForBrowserSideNavigation(false);
 
@@ skipping 577 matching lines @@
 void RenderFrameImpl::RegisterMojoServices() {
   // Only main frame have ImageDownloader service.
   if (!frame_->parent()) {
     GetServiceRegistry()->AddService<image_downloader::ImageDownloader>(
         base::Bind(&ImageDownloaderImpl::CreateMojoService,
                    base::Unretained(this)));
   }
 }
 
 }  // namespace content