RenderFrameImpl: simplify CommitNavigation path

- Remove ResetSourceLocation. We can do it locally in
  DocumentLoader::StartLoading instead.
- Move commit call out of LoadDataURL to CommitNavigation.
- Remove some code duplication in CommitNavigation.
- Add weak_this check which was missing in LoadDataURL path.

Bug: 855189
Change-Id: I1d97a6f28ff574d47b0eb2f592705dfcf8daf026
Reviewed-on: https://chromium-review.googlesource.com/c/1352527
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614170}

content/renderer/render_frame_impl.cc

@@ -3254,71 +3254,84 @@ void RenderFrameImpl::CommitNavigation(
         &item_for_history_navigation, &load_type);
   }
 
-  base::OnceClosure continue_navigation;
-  if (commit_status == blink::mojom::CommitResult::Ok) {
-    base::WeakPtr<RenderFrameImpl> weak_this = weak_factory_.GetWeakPtr();
-    // Check if the navigation being committed originated as a client redirect.
-    bool is_client_redirect =
-        !!(common_params.transition & ui::PAGE_TRANSITION_CLIENT_REDIRECT);
-
-    // Perform a navigation for loadDataWithBaseURL if needed (for main frames).
-    // Note: the base URL might be invalid, so also check the data URL string.
-    bool should_load_data_url = !common_params.base_url_for_data_url.is_empty();
-#if defined(OS_ANDROID)
-    should_load_data_url |= !request_params.data_url_as_string.empty();
-#endif
-    if (is_main_frame_ && should_load_data_url) {
-      LoadDataURL(common_params, request_params, load_type,
-                  item_for_history_navigation, is_client_redirect,
-                  std::move(document_state));
-    } else {
-      WebURLRequest request = CreateURLRequestForCommit(
-          common_params, request_params, std::move(url_loader_client_endpoints),
-          head);
-      // Note: we cannot use base::AutoReset here, since |this| can be deleted
-      // in the next call and auto reset will introduce use-after-free bug.
-      committing_main_request_ = true;
-      frame_->CommitNavigation(
-          request, load_type, item_for_history_navigation, is_client_redirect,
-          devtools_navigation_token,
-          BuildNavigationParams(
-              common_params, request_params,
-              BuildServiceWorkerNetworkProviderForNavigation(
-                  &request_params, std::move(controller_service_worker_info))),
-          std::move(document_state));
-      // The commit can result in this frame being removed. Use a
-      // WeakPtr as an easy way to detect whether this has occured. If so, this
-      // method should return immediately and not touch any part of the object,
-      // otherwise it will result in a use-after-free bug.
-      if (!weak_this)
-        return;
-      committing_main_request_ = false;
-      RequestExtraData* extra_data =
-          static_cast<RequestExtraData*>(request.GetExtraData());
-      continue_navigation =
-          extra_data->TakeContinueNavigationFunctionOwnerShip();
-    }
-  } else {
+  if (commit_status != blink::mojom::CommitResult::Ok) {
     // The browser expects the frame to be loading this navigation. Inform it
     // that the load stopped if needed.
     if (frame_ && !frame_->IsLoading())
       Send(new FrameHostMsg_DidStopLoading(routing_id_));
+    return;
   }
 
-  // Reset the source location now that the commit checks have been processed.
-  frame_->GetDocumentLoader()->ResetSourceLocation();
-  if (frame_->GetProvisionalDocumentLoader())
-    frame_->GetProvisionalDocumentLoader()->ResetSourceLocation();
+  base::WeakPtr<RenderFrameImpl> weak_this = weak_factory_.GetWeakPtr();
+  // Check if the navigation being committed originated as a client redirect.
+  bool is_client_redirect =
+      !!(common_params.transition & ui::PAGE_TRANSITION_CLIENT_REDIRECT);
+
+  std::unique_ptr<blink::WebNavigationParams> navigation_params =
+      BuildNavigationParams(
+          common_params, request_params,
+          BuildServiceWorkerNetworkProviderForNavigation(
+              &request_params, std::move(controller_service_worker_info)));
 
-  // Continue the navigation.
-  // TODO(arthursonzogni): Pass the data needed to continue the navigation to
-  // this function instead of storing it in the
-  // NavigationResponseOverrideParameters. The architecture of committing the
-  // navigation in the renderer process should be simplified and avoid going
-  // through the ResourceFetcher for the main resource.
-  if (continue_navigation) {
-    base::AutoReset<bool> replaying(&replaying_main_response_, true);
-    std::move(continue_navigation).Run();
+  // Perform a navigation to a data url if needed (for main frames).
+  // Note: the base URL might be invalid, so also check the data URL string.
+  bool should_load_data_url = !common_params.base_url_for_data_url.is_empty();
+#if defined(OS_ANDROID)
+  should_load_data_url |= !request_params.data_url_as_string.empty();
+#endif
+  if (is_main_frame_ && should_load_data_url) {
+    std::string mime_type;
+    std::string charset;
+    std::string data;
+    GURL base_url;
+    DecodeDataURL(common_params, request_params, &mime_type, &charset, &data,
+                  &base_url);
+    frame_->CommitDataNavigation(
+        WebURLRequest(base_url), WebData(data.c_str(), data.length()),
+        WebString::FromUTF8(mime_type), WebString::FromUTF8(charset),
+        // Needed so that history-url-only changes don't become reloads.
+        common_params.history_url_for_data_url, load_type,
+        item_for_history_navigation, is_client_redirect,
+        std::move(navigation_params), std::move(document_state));
+    // The commit can result in this frame being removed. Use a
+    // WeakPtr as an easy way to detect whether this has occured. If so, this
+    // method should return immediately and not touch any part of the object,
+    // otherwise it will result in a use-after-free bug.
+    if (!weak_this)
+      return;
+  } else {
+    WebURLRequest request =
+        CreateURLRequestForCommit(common_params, request_params,
+                                  std::move(url_loader_client_endpoints), head);
+    // Note: we cannot use base::AutoReset here, since |this| can be deleted
+    // in the next call and auto reset will introduce use-after-free bug.
+    committing_main_request_ = true;
+    frame_->CommitNavigation(request, load_type, item_for_history_navigation,
+                             is_client_redirect, devtools_navigation_token,
+                             std::move(navigation_params),
+                             std::move(document_state));
+    // The commit can result in this frame being removed. Use a
+    // WeakPtr as an easy way to detect whether this has occured. If so, this
+    // method should return immediately and not touch any part of the object,
+    // otherwise it will result in a use-after-free bug.
+    if (!weak_this)
+      return;
+    committing_main_request_ = false;
+
+    // Continue the navigation.
+    // TODO(arthursonzogni): Pass the data needed to continue the navigation to
+    // this function instead of storing it in the
+    // NavigationResponseOverrideParameters. The architecture of committing the
+    // navigation in the renderer process should be simplified and avoid going
+    // through the ResourceFetcher for the main resource.
+    RequestExtraData* extra_data =
+        static_cast<RequestExtraData*>(request.GetExtraData());
+    base::OnceClosure continue_navigation =
+        extra_data->TakeContinueNavigationFunctionOwnerShip();
+    if (continue_navigation) {
+      base::AutoReset<bool> replaying(&replaying_main_response_, true);
+      std::move(continue_navigation).Run();
+    }
   }
 }
 
@@ -6835,22 +6848,23 @@ void RenderFrameImpl::BeginNavigationInternal(
   }
 }
 
-void RenderFrameImpl::LoadDataURL(
+void RenderFrameImpl::DecodeDataURL(
     const CommonNavigationParams& common_params,
     const RequestNavigationParams& request_params,
-    blink::WebFrameLoadType load_type,
-    blink::WebHistoryItem item_for_history_navigation,
-    bool is_client_redirect,
-    std::unique_ptr<blink::WebDocumentLoader::ExtraData> navigation_data) {
+    std::string* mime_type,
+    std::string* charset,
+    std::string* data,
+    GURL* base_url) {
   // A loadData request with a specified base URL.
   GURL data_url = common_params.url;
 #if defined(OS_ANDROID)
   if (!request_params.data_url_as_string.empty()) {
 #if DCHECK_IS_ON()
     {
-      std::string mime_type, charset, data;
-      DCHECK(net::DataURL::Parse(data_url, &mime_type, &charset, &data));
-      DCHECK(data.empty());
+      std::string mime_type_tmp, charset_tmp, data_tmp;
+      DCHECK(net::DataURL::Parse(data_url, &mime_type_tmp, &charset_tmp,
+                                 &data_tmp));
+      DCHECK(data_tmp.empty());
     }
 #endif
     data_url = GURL(request_params.data_url_as_string);
@@ -6859,23 +6873,10 @@ void RenderFrameImpl::LoadDataURL(
     }
   }
 #endif
-  std::string mime_type, charset, data;
-  if (net::DataURL::Parse(data_url, &mime_type, &charset, &data)) {
-    const GURL base_url = common_params.base_url_for_data_url.is_empty()
-                              ? common_params.url
-                              : common_params.base_url_for_data_url;
-
-    frame_->CommitDataNavigation(
-        WebURLRequest(base_url), WebData(data.c_str(), data.length()),
-        WebString::FromUTF8(mime_type), WebString::FromUTF8(charset),
-        // Needed so that history-url-only changes don't become reloads.
-        common_params.history_url_for_data_url, load_type,
-        item_for_history_navigation, is_client_redirect,
-        BuildNavigationParams(
-            common_params, request_params,
-            BuildServiceWorkerNetworkProviderForNavigation(
-                &request_params, nullptr /* controller_service_worker_info */)),
-        std::move(navigation_data));
+  if (net::DataURL::Parse(data_url, mime_type, charset, data)) {
+    *base_url = common_params.base_url_for_data_url.is_empty()
+                    ? common_params.url
+                    : common_params.base_url_for_data_url;
   } else {
     CHECK(false) << "Invalid URL passed: "
                  << common_params.url.possibly_invalid_spec();

content/renderer/render_frame_impl.h

@@ -1151,14 +1151,13 @@ class CONTENT_EXPORT RenderFrameImpl
   // Sends a FrameHostMsg_BeginNavigation to the browser
   void BeginNavigationInternal(std::unique_ptr<blink::WebNavigationInfo> info);
 
-  // Loads a data url.
-  void LoadDataURL(
-      const CommonNavigationParams& common_params,
-      const RequestNavigationParams& request_params,
-      blink::WebFrameLoadType load_type,
-      blink::WebHistoryItem item_for_history_navigation,
-      bool is_client_redirect,
-      std::unique_ptr<blink::WebDocumentLoader::ExtraData> navigation_data);
+  // Decodes a data url for navigation commit.
+  void DecodeDataURL(const CommonNavigationParams& common_params,
+                     const RequestNavigationParams& request_params,
+                     std::string* mime_type,
+                     std::string* charset,
+                     std::string* data,
+                     GURL* base_url);
 
   // Sends a proper FrameHostMsg_DidFailProvisionalLoadWithError_Params IPC for
   // the failed request |request|.

third_party/blink/public/web/web_document_loader.h

@@ -119,8 +119,6 @@ class BLINK_EXPORT WebDocumentLoader {
   virtual WebServiceWorkerNetworkProvider*
   GetServiceWorkerNetworkProvider() = 0;
 
-  virtual void ResetSourceLocation() = 0;
-
   // Can be used to temporarily suspend feeding the parser with new data. The
   // parser will be allowed to read new data when ResumeParser() is called the
   // same number of time than BlockParser().

third_party/blink/renderer/core/exported/web_document_loader_impl.cc

@@ -144,10 +144,6 @@ WebDocumentLoaderImpl::GetServiceWorkerNetworkProvider() {
   return DocumentLoader::GetServiceWorkerNetworkProvider();
 }
 
-void WebDocumentLoaderImpl::ResetSourceLocation() {
-  DocumentLoader::ResetSourceLocation();
-}
-
 void WebDocumentLoaderImpl::BlockParser() {
   DocumentLoader::BlockParser();
 }

third_party/blink/renderer/core/exported/web_document_loader_impl.h

@@ -79,7 +79,6 @@ class CORE_EXPORT WebDocumentLoaderImpl final : public DocumentLoader,
   void SetServiceWorkerNetworkProvider(
       std::unique_ptr<WebServiceWorkerNetworkProvider>) override;
   WebServiceWorkerNetworkProvider* GetServiceWorkerNetworkProvider() override;
-  void ResetSourceLocation() override;
   void BlockParser() override;
   void ResumeParser() override;
   bool IsArchive() const override;

third_party/blink/renderer/core/loader/document_loader.cc

@@ -300,10 +300,6 @@ void DocumentLoader::SetServiceWorkerNetworkProvider(
   service_worker_network_provider_ = std::move(provider);
 }
 
-void DocumentLoader::ResetSourceLocation() {
-  source_location_ = nullptr;
-}
-
 std::unique_ptr<SourceLocation> DocumentLoader::CopySourceLocation() const {
   return source_location_ ? source_location_->Clone() : nullptr;
 }
@@ -966,8 +962,10 @@ void DocumentLoader::StartLoading() {
   DCHECK_EQ(state_, kNotStarted);
   state_ = kProvisional;
 
-  if (MaybeLoadEmpty())
+  if (MaybeLoadEmpty()) {
+    source_location_ = nullptr;
     return;
+  }
 
   DCHECK(!GetTiming().NavigationStart().is_null());
   // The fetch has already started in the browser,
@@ -984,6 +982,7 @@ void DocumentLoader::StartLoading() {
   // make some modification to the request, e.g. adding the referer header.
   request_ = GetResource()->IsLoading() ? GetResource()->GetResourceRequest()
                                         : fetch_params.GetResourceRequest();
+  source_location_ = nullptr;
 }
 
 void DocumentLoader::DidInstallNewDocument(Document* document) {

third_party/blink/renderer/core/loader/document_loader.h

@@ -224,8 +224,7 @@ class CORE_EXPORT DocumentLoader
     return service_worker_network_provider_.get();
   }
 
-  // Allows to specify the SourceLocation that triggered the navigation.
-  void ResetSourceLocation();
+  // Returns a SourceLocation that triggered the navigation if any.
   std::unique_ptr<SourceLocation> CopySourceLocation() const;
 
   void LoadFailed(const ResourceError&);