DevTools: don't update all frames aggresively upon render frame swap.

We used to detach from targets aggressively because of the timing of the
UpdateFrames call. Current frame host was still non-cross-process and hence
was detached from. This change migrates from bulk-frame update to treating
render frame host swap in AutoAttachToFrame.

Bug: 805248
Change-Id: Id6c8a665676167fea5048c23db20cfc8fafd86f1
Reviewed-on: https://chromium-review.googlesource.com/882167
Commit-Queue: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531805}

content/browser/devtools/protocol/target_auto_attacher.cc

@@ -141,26 +141,40 @@ void TargetAutoAttacher::AgentHostClosed(DevToolsAgentHost* host) {
 }
 
 bool TargetAutoAttacher::ShouldThrottleFramesNavigation() {
-  return auto_attach_ && attach_to_frames_ && wait_for_debugger_on_start_;
+  return auto_attach_ && attach_to_frames_;
 }
 
 DevToolsAgentHost* TargetAutoAttacher::AutoAttachToFrame(
-    NavigationHandle* navigation_handle) {
+    NavigationHandleImpl* navigation_handle) {
   if (!ShouldThrottleFramesNavigation())
     return nullptr;
-  if (!navigation_handle->GetRenderFrameHost() ||
-      !navigation_handle->GetRenderFrameHost()->IsCrossProcessSubframe()) {
-    return nullptr;
-  }
+
+  FrameTreeNode* frame_tree_node = navigation_handle->frame_tree_node();
+  RenderFrameHostImpl* new_host = navigation_handle->GetRenderFrameHost();
   scoped_refptr<DevToolsAgentHost> agent_host =
-      RenderFrameDevToolsAgentHost::GetOrCreateForDangling(
-          static_cast<NavigationHandleImpl*>(navigation_handle)
-              ->frame_tree_node());
-  if (auto_attached_hosts_.find(agent_host) != auto_attached_hosts_.end())
+      RenderFrameDevToolsAgentHost::FindForDangling(frame_tree_node);
+
+  bool old_cross_process = !!agent_host;
+  bool new_cross_process = new_host && new_host->IsCrossProcessSubframe();
+
+  if (old_cross_process == new_cross_process)
     return nullptr;
-  attach_callback_.Run(agent_host.get(), true /* waiting_for_debugger */);
-  auto_attached_hosts_.insert(agent_host);
-  return agent_host.get();
+
+  if (new_cross_process) {
+    agent_host =
+        RenderFrameDevToolsAgentHost::GetOrCreateForDangling(frame_tree_node);
+    DCHECK(auto_attached_hosts_.find(agent_host) == auto_attached_hosts_.end());
+    attach_callback_.Run(agent_host.get(), wait_for_debugger_on_start_);
+    auto_attached_hosts_.insert(agent_host);
+    return wait_for_debugger_on_start_ ? agent_host.get() : nullptr;
+  }
+
+  DCHECK(old_cross_process);
+  auto it = auto_attached_hosts_.find(agent_host);
+  DCHECK(it != auto_attached_hosts_.end());
+  auto_attached_hosts_.erase(it);
+  detach_callback_.Run(agent_host.get());
+  return nullptr;
 }
 
 void TargetAutoAttacher::ReattachServiceWorkers(bool waiting_for_debugger) {

content/browser/devtools/protocol/target_auto_attacher.h

@@ -11,7 +11,7 @@
 
 namespace content {
 
-class NavigationHandle;
+class NavigationHandleImpl;
 class RenderFrameHostImpl;
 
 namespace protocol {
@@ -31,11 +31,10 @@ class TargetAutoAttacher : public ServiceWorkerDevToolsManager::Observer {
   void SetAttachToFrames(bool attach_to_frames);
 
   void UpdateServiceWorkers();
-  void UpdateFrames();
   void AgentHostClosed(DevToolsAgentHost* host);
 
   bool ShouldThrottleFramesNavigation();
-  DevToolsAgentHost* AutoAttachToFrame(NavigationHandle* navigation_handle);
+  DevToolsAgentHost* AutoAttachToFrame(NavigationHandleImpl* navigation_handle);
 
  private:
   using Hosts = base::flat_set<scoped_refptr<DevToolsAgentHost>>;
@@ -52,6 +51,8 @@ class TargetAutoAttacher : public ServiceWorkerDevToolsManager::Observer {
   void WorkerVersionDoomed(ServiceWorkerDevToolsAgentHost* host) override;
   void WorkerDestroyed(ServiceWorkerDevToolsAgentHost* host) override;
 
+  void UpdateFrames();
+
   AttachCallback attach_callback_;
   DetachCallback detach_callback_;
   RenderFrameHostImpl* render_frame_host_;

content/browser/devtools/protocol/target_handler.cc

@@ -9,6 +9,7 @@
 #include "base/values.h"
 #include "content/browser/devtools/devtools_manager.h"
 #include "content/browser/devtools/devtools_session.h"
+#include "content/browser/frame_host/navigation_handle_impl.h"
 #include "content/public/browser/devtools_agent_host_client.h"
 #include "content/public/browser/navigation_throttle.h"
 
@@ -164,8 +165,8 @@ NavigationThrottle::ThrottleCheckResult
 TargetHandler::Throttle::WillProcessResponse() {
   if (!target_handler_)
     return PROCEED;
-  agent_host_ =
-      target_handler_->auto_attacher_.AutoAttachToFrame(navigation_handle());
+  agent_host_ = target_handler_->auto_attacher_.AutoAttachToFrame(
+      static_cast<NavigationHandleImpl*>(navigation_handle()));
   if (!agent_host_.get())
     return PROCEED;
   target_handler_->auto_attached_sessions_[agent_host_.get()]->SetThrottle(
@@ -226,10 +227,6 @@ void TargetHandler::DidCommitNavigation() {
   auto_attacher_.UpdateServiceWorkers();
 }
 
-void TargetHandler::RenderFrameHostChanged() {
-  auto_attacher_.UpdateFrames();
-}
-
 std::unique_ptr<NavigationThrottle> TargetHandler::CreateThrottleForNavigation(
     NavigationHandle* navigation_handle) {
   if (!auto_attacher_.ShouldThrottleFramesNavigation())

content/browser/devtools/render_frame_devtools_agent_host.cc

@@ -132,6 +132,12 @@ RenderFrameDevToolsAgentHost::GetOrCreateForDangling(
   return result;
 }
 
+// static
+scoped_refptr<DevToolsAgentHost> RenderFrameDevToolsAgentHost::FindForDangling(
+    FrameTreeNode* frame_tree_node) {
+  return FindAgentHost(frame_tree_node);
+}
+
 // static
 bool DevToolsAgentHost::HasFor(WebContents* web_contents) {
   FrameTreeNode* node =
@@ -513,9 +519,6 @@ void RenderFrameDevToolsAgentHost::DidStartNavigation(
 void RenderFrameDevToolsAgentHost::RenderFrameHostChanged(
     RenderFrameHost* old_host,
     RenderFrameHost* new_host) {
-  for (auto* target : protocol::TargetHandler::ForAgentHost(this))
-    target->RenderFrameHostChanged();
-
   if (old_host != frame_host_)
     return;
 

content/browser/devtools/render_frame_devtools_agent_host.h

@@ -52,6 +52,8 @@ class CONTENT_EXPORT RenderFrameDevToolsAgentHost
   // Prefer GetOrCreateFor instead.
   static scoped_refptr<DevToolsAgentHost> GetOrCreateForDangling(
       FrameTreeNode* frame_tree_node);
+  static scoped_refptr<DevToolsAgentHost> FindForDangling(
+      FrameTreeNode* frame_tree_node);
 
   static void OnWillSendNavigationRequest(
       FrameTreeNode* frame_tree_node,

third_party/WebKit/LayoutTests/http/tests/devtools/oopif/oopif-targets-expected.txt

+Tests that targets are created for oopif iframes.
+
+
+Targets before navigate
+Main
+
+Targets after navigate
+Main
+iframe.html
+inner-iframe.html
+inner-iframe.html
+
+Targets on about:blank
+Main
+

third_party/WebKit/LayoutTests/http/tests/devtools/oopif/oopif-targets.js

+// Copyright 2018 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.
+
+(async function() {
+  TestRunner.addResult(`Tests that targets are created for oopif iframes.\n`);
+
+  TestRunner.addResult('\nTargets before navigate');
+  TestRunner.addResults(SDK.targetManager.targets().map(t => t.name()).sort());
+
+  await TestRunner.navigatePromise('resources/page.html');
+  TestRunner.addResult('\nTargets after navigate');
+  TestRunner.addResults(SDK.targetManager.targets().map(t => t.name()).sort());
+
+  await TestRunner.navigatePromise('about:blank');
+  TestRunner.addResult('\nTargets on about:blank');
+  TestRunner.addResults(SDK.targetManager.targets().map(t => t.name()).sort());
+
+  TestRunner.completeTest();
+})();

third_party/WebKit/LayoutTests/http/tests/devtools/oopif/resources/iframe.html

+<body>
+    <div>
+        <iframe id="iframe-iframe1" src="http://127.0.0.1:8000/devtools/oopif/resources/inner-iframe.html?first"></iframe>
+    </div>
+    <div>
+        <iframe id="iframe-iframe2" src="http://devtools.oopif.test:8000/devtools/oopif/resources/inner-iframe.html?second"></iframe>
+    </div>
+</body>

third_party/WebKit/LayoutTests/http/tests/devtools/oopif/resources/inner-iframe.html

+<body>IFRAME</body>

third_party/WebKit/LayoutTests/http/tests/devtools/oopif/resources/page.html

+<body>
+    <div>
+        <iframe id="page-iframe1" src="http://127.0.0.1:8000/devtools/oopif/resources/iframe.html?first"></iframe>
+    </div>
+    <div>
+        <iframe id="page-iframe2" src="http://devtools.oopif.test:8000/devtools/oopif/resources/iframe.html?second"></iframe>
+    </div>
+</body>