DocumentLoader::CommitNavigation cleanup

Split most of the window initialization logic out into a helper.

Also, it's not clear that GlobalObjectReusePolicy needs to be an enum.
Return a boolean for whether we should reuse the dom window, and use
the bool |should_reset_browser_interface_broker| for all of the layers
of load commit callbacks, rather than switching from enum to bool
part way.

Change-Id: I8ff3c514115ff6b94f7992af8a73c5cc06e77623
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2341346
Commit-Queue: Nate Chapin <japhet@chromium.org>
Auto-Submit: Nate Chapin <japhet@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796055}

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

@@ -540,7 +540,7 @@ void LocalFrameClientImpl::DispatchDidReceiveTitle(const String& title) {
 void LocalFrameClientImpl::DispatchDidCommitLoad(
     HistoryItem* item,
     WebHistoryCommitType commit_type,
-    GlobalObjectReusePolicy global_object_reuse_policy) {
+    bool should_reset_browser_interface_broker) {
   if (!web_frame_->Parent()) {
     web_frame_->ViewImpl()->DidCommitLoad(commit_type == kWebStandardCommit,
                                           false);
@@ -549,7 +549,7 @@ void LocalFrameClientImpl::DispatchDidCommitLoad(
   if (web_frame_->Client()) {
     web_frame_->Client()->DidCommitNavigation(
         WebHistoryItem(item), commit_type,
-        global_object_reuse_policy == GlobalObjectReusePolicy::kCreateNew);
+        should_reset_browser_interface_broker);
     if (web_frame_->GetFrame()->IsLocalRoot()) {
       // This update should be sent as soon as loading the new document begins
       // so that the browser and compositor could reset their states. However,

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

@@ -52,7 +52,6 @@ class BrowserInterfaceBrokerProxy;
 class WebDevToolsAgentImpl;
 class WebLocalFrameImpl;
 class WebSpellCheckPanelHostClient;
-enum class GlobalObjectReusePolicy;
 
 class CORE_EXPORT LocalFrameClientImpl final : public LocalFrameClient {
  public:
@@ -101,9 +100,10 @@ class CORE_EXPORT LocalFrameClientImpl final : public LocalFrameClient {
                                        WebHistoryCommitType,
                                        bool content_initiated) override;
   void DispatchDidReceiveTitle(const String&) override;
-  void DispatchDidCommitLoad(HistoryItem*,
+  void DispatchDidCommitLoad(
+      HistoryItem*,
       WebHistoryCommitType,
-                             GlobalObjectReusePolicy) override;
+      bool should_reset_browser_interface_broker) override;
   void DispatchDidFailLoad(const ResourceError&, WebHistoryCommitType) override;
   void DispatchDidFinishDocumentLoad() override;
   void DispatchDidFinishLoad() override;

third_party/blink/renderer/core/frame/local_frame_client.h

@@ -131,9 +131,10 @@ class CORE_EXPORT LocalFrameClient : public FrameClient {
                                                WebHistoryCommitType,
                                                bool content_initiated) {}
   virtual void DispatchDidReceiveTitle(const String&) = 0;
-  virtual void DispatchDidCommitLoad(HistoryItem*,
+  virtual void DispatchDidCommitLoad(
+      HistoryItem*,
       WebHistoryCommitType,
-                                     GlobalObjectReusePolicy) = 0;
+      bool should_reset_browser_interface_broker) = 0;
   virtual void DispatchDidFailLoad(const ResourceError&,
                                    WebHistoryCommitType) = 0;
   virtual void DispatchDidFinishDocumentLoad() = 0;

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

@@ -1602,15 +1602,12 @@ scoped_refptr<SecurityOrigin> DocumentLoader::CalculateOrigin(
   return origin;
 }
 
-GlobalObjectReusePolicy DocumentLoader::CalculateGlobalObjectReusePolicy(
-    SecurityOrigin* security_origin) {
+bool ShouldReuseDOMWindow(LocalFrame* frame, SecurityOrigin* security_origin) {
   // Secure transitions can only happen when navigating from the initial empty
   // document.
-  if (!GetFrameLoader().StateMachine()->IsDisplayingInitialEmptyDocument())
-    return GlobalObjectReusePolicy::kCreateNew;
-  if (!frame_->DomWindow()->GetSecurityOrigin()->CanAccess(security_origin))
-    return GlobalObjectReusePolicy::kCreateNew;
-  return GlobalObjectReusePolicy::kUseExisting;
+  if (!frame->Loader().StateMachine()->IsDisplayingInitialEmptyDocument())
+    return false;
+  return frame->DomWindow()->GetSecurityOrigin()->CanAccess(security_origin);
 }
 
 WindowAgent* GetWindowAgentForOrigin(LocalFrame* frame,
@@ -1625,53 +1622,10 @@ WindowAgent* GetWindowAgentForOrigin(LocalFrame* frame,
       V8PerIsolateData::MainThreadIsolate(), origin);
 }
 
-void DocumentLoader::CommitNavigation() {
-  CHECK_GE(state_, kCommitted);
-  DCHECK(frame_->GetPage());
-  DCHECK(!frame_->GetDocument() || !frame_->GetDocument()->IsActive());
-  DCHECK_EQ(frame_->Tree().ChildCount(), 0u);
-
-  // Prepare a DocumentInit before clearing the frame, because it may need to
-  // inherit an aliased security context.
-  Document* owner_document = nullptr;
-
-  // TODO(dcheng): This differs from the behavior of both IE and Firefox: the
-  // origin is inherited from the document that loaded the URL.
-  if (Document::ShouldInheritSecurityOriginFromOwner(Url())) {
-    Frame* owner_frame = frame_->Tree().Parent();
-    if (!owner_frame)
-      owner_frame = frame_->Loader().Opener();
-    if (auto* owner_local_frame = DynamicTo<LocalFrame>(owner_frame))
-      owner_document = owner_local_frame->GetDocument();
-  }
-
-  // Re-validate Document Policy feature before installing the new document.
-  if (!RuntimeEnabledFeatures::DocumentPolicyEnabled(
-          owner_document ? owner_document->GetExecutionContext() : nullptr)) {
-    document_policy_ = DocumentPolicy::ParsedDocumentPolicy{};
-  }
-
-  if (document_policy_.feature_state.contains(
-          mojom::blink::DocumentPolicyFeature::kForceLoadAtTop)) {
-    navigation_scroll_allowed_ = !(
-        document_policy_
-            .feature_state[mojom::blink::DocumentPolicyFeature::kForceLoadAtTop]
-            .BoolValue());
-  }
-
+void DocumentLoader::InitializeWindow(Document* owner_document) {
   auto sandbox_flags = CalculateSandboxFlags();
   auto security_origin = CalculateOrigin(owner_document, sandbox_flags);
 
-  GlobalObjectReusePolicy global_object_reuse_policy =
-      CalculateGlobalObjectReusePolicy(security_origin.get());
-
-  if (GetFrameLoader().StateMachine()->IsDisplayingInitialEmptyDocument()) {
-    GetFrameLoader().StateMachine()->AdvanceTo(
-        FrameLoaderStateMachine::kCommittedFirstRealLoad);
-  }
-
-  LocalDOMWindow* previous_window = frame_->DomWindow();
-
   // In some rare cases, we'll re-use a LocalDOMWindow for a new Document. For
   // example, when a script calls window.open("..."), the browser gives
   // JavaScript a window synchronously but kicks off the load in the window
@@ -1680,7 +1634,7 @@ void DocumentLoader::CommitNavigation() {
   // commits. To make that happen, we "securely transition" the existing
   // LocalDOMWindow to the Document that results from the network load. See also
   // Document::IsSecureTransitionTo.
-  if (global_object_reuse_policy != GlobalObjectReusePolicy::kUseExisting) {
+  if (!ShouldReuseDOMWindow(frame_.Get(), security_origin.get())) {
     auto* agent = GetWindowAgentForOrigin(frame_.Get(), security_origin.get());
     frame_->SetDOMWindow(MakeGarbageCollected<LocalDOMWindow>(*frame_, agent));
 
@@ -1723,18 +1677,69 @@ void DocumentLoader::CommitNavigation() {
   security_origin = security_origin->GetOriginForAgentCluster(
       frame_->DomWindow()->GetAgent()->cluster_id());
 
-  frame_->DomWindow()->GetSecurityContext().SetContentSecurityPolicy(
-      content_security_policy_.Get());
-  frame_->DomWindow()->GetSecurityContext().ApplySandboxFlags(sandbox_flags);
+  SecurityContext& security_context = frame_->DomWindow()->GetSecurityContext();
+  security_context.SetContentSecurityPolicy(content_security_policy_.Get());
+  security_context.ApplySandboxFlags(sandbox_flags);
   // Conceptually, SecurityOrigin doesn't have to be initialized after sandbox
   // flags are applied, but there's a UseCounter in SetSecurityOrigin() that
   // wants to inspect sandbox flags.
-  frame_->DomWindow()->GetSecurityContext().SetSecurityOrigin(
-      std::move(security_origin));
+  security_context.SetSecurityOrigin(std::move(security_origin));
   // Requires SecurityOrigin to be initialized.
   OriginTrialContext::AddTokensFromHeader(
       frame_->DomWindow(), response_.HttpHeaderField(http_names::kOriginTrial));
 
+  if (auto* parent = frame_->Tree().Parent()) {
+    const SecurityContext* parent_context = parent->GetSecurityContext();
+    security_context.SetInsecureRequestPolicy(
+        parent_context->GetInsecureRequestPolicy());
+    for (auto to_upgrade : parent_context->InsecureNavigationsToUpgrade())
+      security_context.AddInsecureNavigationUpgrade(to_upgrade);
+  }
+  frame_->DomWindow()->SetAddressSpace(ip_address_space_);
+}
+
+void DocumentLoader::CommitNavigation() {
+  CHECK_GE(state_, kCommitted);
+  DCHECK(frame_->GetPage());
+  DCHECK(!frame_->GetDocument() || !frame_->GetDocument()->IsActive());
+  DCHECK_EQ(frame_->Tree().ChildCount(), 0u);
+
+  // Prepare a DocumentInit before clearing the frame, because it may need to
+  // inherit an aliased security context.
+  Document* owner_document = nullptr;
+
+  // TODO(dcheng): This differs from the behavior of both IE and Firefox: the
+  // origin is inherited from the document that loaded the URL.
+  if (Document::ShouldInheritSecurityOriginFromOwner(Url())) {
+    Frame* owner_frame = frame_->Tree().Parent();
+    if (!owner_frame)
+      owner_frame = frame_->Loader().Opener();
+    if (auto* owner_local_frame = DynamicTo<LocalFrame>(owner_frame))
+      owner_document = owner_local_frame->GetDocument();
+  }
+
+  // Re-validate Document Policy feature before installing the new document.
+  if (!RuntimeEnabledFeatures::DocumentPolicyEnabled(
+          owner_document ? owner_document->GetExecutionContext() : nullptr)) {
+    document_policy_ = DocumentPolicy::ParsedDocumentPolicy{};
+  }
+
+  if (document_policy_.feature_state.contains(
+          mojom::blink::DocumentPolicyFeature::kForceLoadAtTop)) {
+    navigation_scroll_allowed_ = !(
+        document_policy_
+            .feature_state[mojom::blink::DocumentPolicyFeature::kForceLoadAtTop]
+            .BoolValue());
+  }
+
+  LocalDOMWindow* previous_window = frame_->DomWindow();
+  InitializeWindow(owner_document);
+
+  if (GetFrameLoader().StateMachine()->IsDisplayingInitialEmptyDocument()) {
+    GetFrameLoader().StateMachine()->AdvanceTo(
+        FrameLoaderStateMachine::kCommittedFirstRealLoad);
+  }
+
   SecurityContextInit security_init(frame_->DomWindow());
   // FeaturePolicy and DocumentPolicy require SecurityOrigin and origin trials
   // to be initialized.
@@ -1752,26 +1757,16 @@ void DocumentLoader::CommitNavigation() {
       document_policy_,
       response_.HttpHeaderField(http_names::kDocumentPolicyReportOnly));
 
-  if (auto* parent = frame_->Tree().Parent()) {
-    SecurityContext& this_context = frame_->DomWindow()->GetSecurityContext();
-    const SecurityContext* parent_context = parent->GetSecurityContext();
-    this_context.SetInsecureRequestPolicy(
-        parent_context->GetInsecureRequestPolicy());
-    for (auto to_upgrade : parent_context->InsecureNavigationsToUpgrade())
-      this_context.AddInsecureNavigationUpgrade(to_upgrade);
-  }
-  frame_->DomWindow()->SetAddressSpace(ip_address_space_);
-
   WillCommitNavigation();
 
-  DocumentInit init = DocumentInit::Create()
+  Document* document = frame_->DomWindow()->InstallNewDocument(
+      DocumentInit::Create()
           .WithWindow(frame_->DomWindow(), owner_document)
           .WithURL(Url())
           .WithTypeFrom(MimeType())
           .WithSrcdocDocument(loading_srcdoc_)
           .WithNewRegistrationContext()
-                          .WithWebBundleClaimedUrl(web_bundle_claimed_url_);
-  Document* document = frame_->DomWindow()->InstallNewDocument(init);
+          .WithWebBundleClaimedUrl(web_bundle_claimed_url_));
 
   // Clear the user activation state.
   // TODO(crbug.com/736415): Clear this bit unconditionally for all frames.
@@ -1898,7 +1893,7 @@ void DocumentLoader::CommitNavigation() {
     } else {
       GetLocalFrameClient().DispatchDidCommitLoad(
           history_item_.Get(), LoadTypeToCommitType(load_type_),
-          global_object_reuse_policy);
+          previous_window != frame_->DomWindow());
     }
     // TODO(dgozman): make DidCreateScriptContext notification call currently
     // triggered by installing new document happen here, after commit.

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

@@ -99,12 +99,6 @@ namespace mojom {
 enum class CommitResult : int32_t;
 }
 
-// Indicates whether the global object (i.e. Window instance) associated with
-// the previous document in a browsing context was replaced or reused for the
-// new Document corresponding to the just-committed navigation; effective in the
-// main world and all isolated worlds. WindowProxies are not affected.
-enum class GlobalObjectReusePolicy { kCreateNew, kUseExisting };
-
 // The DocumentLoader fetches a main resource and handles the result.
 // TODO(https://crbug.com/855189). This was originally structured to have a
 // provisional load, then commit but that is no longer necessary and this class
@@ -343,7 +337,7 @@ class CORE_EXPORT DocumentLoader : public GarbageCollected<DocumentLoader>,
   scoped_refptr<SecurityOrigin> CalculateOrigin(
       Document* owner_document,
       network::mojom::blink::WebSandboxFlags);
-  GlobalObjectReusePolicy CalculateGlobalObjectReusePolicy(SecurityOrigin*);
+  void InitializeWindow(Document* owner_document);
   void DidInstallNewDocument(Document*);
   void WillCommitNavigation();
   void DidCommitNavigation();

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

@@ -83,8 +83,6 @@ class Cursor;
 
 namespace blink {
 
-enum class GlobalObjectReusePolicy;
-
 class CORE_EXPORT EmptyChromeClient : public ChromeClient {
  public:
   ~EmptyChromeClient() override = default;
@@ -248,7 +246,7 @@ class CORE_EXPORT EmptyLocalFrameClient : public LocalFrameClient {
   void DispatchDidReceiveTitle(const String&) override {}
   void DispatchDidCommitLoad(HistoryItem*,
                              WebHistoryCommitType,
-                             GlobalObjectReusePolicy) override {}
+                             bool) override {}
   void DispatchDidFailLoad(const ResourceError&,
                            WebHistoryCommitType) override {}
   void DispatchDidFinishDocumentLoad() override {}