Browser test for LoadDataWithBaseURL reload

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 8ab011d7f3a8acb0145c0cd6233d7404def82219..1ac25e120f877651cb6a5f5037715d71001ec32a 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -307,16 +307,37 @@ void GetRedirectChain(WebDataSource* ds, std::vector<GURL>* result) {
   }
 }
 
-// Returns the original request url. If there is no redirect, the original
-// url is the same as ds->request()->url(). If the WebDataSource belongs to a
-// frame was loaded by loadData, the original url will be ds->unreachableURL()
-GURL GetOriginalRequestURL(WebDataSource* ds) {
+// Gets URL that should override the default getter for this data source.

[Charlie Reis, 2015/11/24 00:22:59]

nit: Add: "(if any), storing it in |output|."

[boliu, 2015/11/24 19:03:25]

Done.

+// Return true if there is an override URL.

[Charlie Reis, 2015/11/24 00:22:59]

nit: Returns
(to be consistent with "Gets")

[boliu, 2015/11/24 19:03:25]

Done.

+bool MaybeGetURLOverride(WebDataSource* ds, GURL* output) {

[Charlie Reis, 2015/11/24 00:22:59]

nit: MaybeGetOverriddenURL

[boliu, 2015/11/24 19:03:25]

Done.

+  DocumentState* document_state = DocumentState::FromDataSource(ds);
+
+  // If load was from a data URL, then the saved data URL, not the history
+  // URL, should be the URL of the data source.
+  if (document_state->was_load_data_with_base_url_request()) {
+    *output = document_state->data_url();
+    return true;
+  }
+
   // WebDataSource has unreachable URL means that the frame is loaded through
   // blink::WebFrame::loadData(), and the base URL will be in the redirect
   // chain. However, we never visited the baseURL. So in this case, we should
   // use the unreachable URL as the original URL.
-  if (ds->hasUnreachableURL())
-    return ds->unreachableURL();
+  if (ds->hasUnreachableURL()) {
+    *output = ds->unreachableURL();
+    return true;
+  }
+
+  return false;
+}
+
+// Returns the original request url. If there is no redirect, the original
+// url is the same as ds->request()->url(). If the WebDataSource belongs to a
+// frame was loaded by loadData, the original url will be ds->unreachableURL()
+GURL GetOriginalRequestURL(WebDataSource* ds) {
+  GURL overriden_url;
+  if (MaybeGetURLOverride(ds, &overriden_url))
+    return overriden_url;
 
   std::vector<GURL> redirects;
   GetRedirectChain(ds, &redirects);
@@ -2614,7 +2635,11 @@ void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
       PopulateDocumentStateFromPending(document_state);
   }
 
-  // Carry over the user agent override flag, if it exists.
+  // The rest of RenderView assumes that a WebDataSource will always have a
+  // non-null NavigationState.
+  UpdateNavigationState(document_state);
+
+  // Carry over the user agent override flag and data URL if necessary.
   blink::WebView* webview = render_view_->webview();
   if (content_initiated && webview && webview->mainFrame() &&
       webview->mainFrame()->isWebLocalFrame() &&
@@ -2628,13 +2653,13 @@ void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
           InternalDocumentStateData::FromDocumentState(old_document_state);
       internal_data->set_is_overriding_user_agent(
           old_internal_data->is_overriding_user_agent());
+
+      document_state->set_was_load_data_with_base_url_request(

[Charlie Reis, 2015/11/24 00:22:59]

Looking closer, I'm skeptical about this call.  Won't this apply to link clicks
to different (non-data) pages?  It makes sense to carry over the user agent
override in that case, but we would no longer be on a data URL.

What's the case this was needed for?  We should set it correctly for
browser-initiated navigations in UpdateNavigationState (given your change
below).  Is this for the in-page renderer-initiated case?

If so, maybe we should copy the URL over in didNavigateWithinPage instead? 
(That might also clear the way for using NavigateState instead of
DocumentState?)

[boliu, 2015/11/24 19:03:25]

Err, this is not needed afaict. in-page navigations don't create new
DocumentState.

I think I was copying originally copying the set_is_overriding_user_agent
pattern, and didn't really know what I was doing :p

[Charlie Reis, 2015/11/24 19:32:38]

Great, I'm glad it's not needed.

+          old_document_state->was_load_data_with_base_url_request());
+      document_state->set_data_url(old_document_state->data_url());
     }
   }
 
-  // The rest of RenderView assumes that a WebDataSource will always have a
-  // non-null NavigationState.
-  UpdateNavigationState(document_state);
-
   // DocumentState::referred_by_prefetcher_ is true if we are
   // navigating from a page that used prefetching using a link on that
   // page.  We are early enough in the request process here that we
@@ -4840,7 +4865,7 @@ void RenderFrameImpl::NavigateInternal(
     if (!common_params.base_url_for_data_url.is_empty() ||
         (browser_side_navigation &&
          common_params.url.SchemeIs(url::kDataScheme))) {
-      LoadDataURL(common_params, frame_);
+      LoadDataURL(common_params, frame_, load_type);
     } else {
       // Load the request.
       frame_->toWebLocalFrame()->load(request, load_type,
@@ -5080,19 +5105,23 @@ void RenderFrameImpl::BeginNavigation(blink::WebURLRequest* request) {
 }
 
 void RenderFrameImpl::LoadDataURL(const CommonNavigationParams& params,
-                                  WebFrame* frame) {
+                                  WebFrame* frame,
+                                  blink::WebFrameLoadType load_type) {
   // A loadData request with a specified base URL.
   std::string mime_type, charset, data;
   if (net::DataURL::Parse(params.url, &mime_type, &charset, &data)) {
     const GURL base_url = params.base_url_for_data_url.is_empty() ?
         params.url : params.base_url_for_data_url;
+    bool replace = load_type == blink::WebFrameLoadType::ReloadFromOrigin ||
+                   load_type == blink::WebFrameLoadType::Reload;
     frame->loadData(
         WebData(data.c_str(), data.length()),
         WebString::fromUTF8(mime_type),
         WebString::fromUTF8(charset),
         base_url,
+        // Needed so that history-url-only changes don't become reloads.
         params.history_url_for_data_url,
-        false);
+        replace);
   } else {
     CHECK(false) << "Invalid URL passed: "
                  << params.url.possibly_invalid_spec();
@@ -5156,8 +5185,10 @@ bool RenderFrameImpl::ShouldDisplayErrorPageForFailedLoad(
 
 GURL RenderFrameImpl::GetLoadingUrl() const {
   WebDataSource* ds = frame_->dataSource();
-  if (ds->hasUnreachableURL())
-    return ds->unreachableURL();
+
+  GURL overriden_url;
+  if (MaybeGetURLOverride(ds, &overriden_url))
+    return overriden_url;
 
   const WebURLRequest& request = ds->request();
   return request.url();
@@ -5224,6 +5255,16 @@ void RenderFrameImpl::UpdateNavigationState(DocumentState* document_state) {
           base::TimeTicks::Now();
     }
     document_state->set_navigation_state(CreateNavigationStateFromPending());
+
+    const CommonNavigationParams& common_params =
+        pending_navigation_params_->common_params;
+    bool load_data = !common_params.base_url_for_data_url.is_empty() &&
+                     !common_params.history_url_for_data_url.is_empty() &&
+                     common_params.url.SchemeIs(url::kDataScheme);
+    document_state->set_was_load_data_with_base_url_request(load_data);
+    if (load_data)
+      document_state->set_data_url(common_params.url);
+
     pending_navigation_params_.reset();
   } else {
     document_state->set_navigation_state(