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 2193a457d7d48faac17ea61efab94d46ea73e986..e92a343db300a2fd0a5d63031828418a67e407df 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -303,16 +303,38 @@ 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) {
+// Return URLs that should override the default getter for this data source.

[Charlie Reis, 2015/11/20 21:30:12]

nit: Please rephrase.  This method returns a bool, not a URL.

[boliu, 2015/11/23 22:32:25]

Done.

+// Or an invalid GURL if there are no overrides.
+bool MaybeGetURLOverride(WebDataSource* ds, GURL* output) {
+  DocumentState* document_state = DocumentState::FromDataSource(ds);
+
+  // If load was from a data URL, then the saved data URL should be the
+  // URL of the data source.

[Charlie Reis, 2015/11/20 21:30:12]

nit: ..., not the history URL.

Sanity check: I know the history URL is stored in unreachableURL(), but what's
in ds->originalRequest().url()?  I thought that was the data URL at this point,
which means we wouldn't need to separately store it in DocumentState.  Maybe I'm
wrong?

[boliu, 2015/11/23 22:32:25]

Done
It's the base URL. Blink never sees the data part as an URL. The data URL is
parsed into a buffer in LoadDataURL and blink only ever sees that buffer.

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

Acknowledged.

+  if (document_state->was_load_data_url_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);
@@ -2605,6 +2627,10 @@ void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
       PopulateDocumentStateFromPending(document_state);
   }
 
+  // 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, if it exists.

[Charlie Reis, 2015/11/20 21:30:12]

nit: Update comment.

[boliu, 2015/11/23 22:32:25]

Done

   blink::WebView* webview = render_view_->webview();
   if (content_initiated && webview && webview->mainFrame() &&
@@ -2619,13 +2645,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_url_with_base_url_request(
+          old_document_state->was_load_data_url_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
@@ -4831,7 +4857,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,
@@ -5071,19 +5097,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 to not identify only changing history url as reload.

[Charlie Reis, 2015/11/20 21:30:12]

nit: Rephrase.

Needed so that history-url-only changes don't become reloads.

[boliu, 2015/11/23 22:32:25]

Done.

         params.history_url_for_data_url,
-        false);
+        replace);
   } else {
     CHECK(false) << "Invalid URL passed: "
                  << params.url.possibly_invalid_spec();
@@ -5147,8 +5177,11 @@ 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)) {

[Charlie Reis, 2015/11/20 21:30:12]

nit: No braces.

[boliu, 2015/11/23 22:32:25]

Done.

+    return overriden_url;
+  }
 
   const WebURLRequest& request = ds->request();
   return request.url();
@@ -5215,6 +5248,15 @@ 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_url_with_base_url_request(load_data);
+    document_state->set_data_url(common_params.url);

[Charlie Reis, 2015/11/20 21:30:12]

We should only do this if |load_data| is true.  It will be confusing for
non-load-data cases to have a non-data URL in data_url_.

[boliu, 2015/11/23 22:32:25]

Done

+
     pending_navigation_params_.reset();
   } else {
     document_state->set_navigation_state(