[fuchsia] Enable mixed-mode remote debugging.

* Enables reporting of  user-mode debugging to debug
  DevToolsPerContextListeners.
* Remove DISALLOW_COPY_AND_ASSIGN in WebEngineDevToolsController
  implementations.
* Add test enabling both user and debug-mode debugging.

Bug: 1018371
Change-Id: Ie38c1c9318190e1e9c374f9ccf86a749aa531027
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1898473
Commit-Queue: Fabrice de Gans-Riberi <fdegans@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715018}

fuchsia/engine/browser/web_engine_devtools_controller.cc

@@ -33,6 +33,9 @@ class DevToolsSocketFactory : public content::DevToolsSocketFactory {
         ip_end_point_(std::move(ip_end_point)) {}
   ~DevToolsSocketFactory() override = default;
 
+  DevToolsSocketFactory(const DevToolsSocketFactory&) = delete;
+  DevToolsSocketFactory& operator=(const DevToolsSocketFactory&) = delete;
+
   // content::DevToolsSocketFactory implementation.
   std::unique_ptr<net::ServerSocket> CreateForHttpServer() override {
     const int kTcpListenBackLog = 5;
@@ -60,8 +63,6 @@ class DevToolsSocketFactory : public content::DevToolsSocketFactory {
  private:
   OnDevToolsPortChanged on_devtools_port_;
   net::IPEndPoint ip_end_point_;
-
-  DISALLOW_COPY_AND_ASSIGN(DevToolsSocketFactory);
 };
 
 void StartRemoteDebuggingServer(OnDevToolsPortChanged on_devtools_port,
@@ -80,6 +81,9 @@ class NoopController : public WebEngineDevToolsController {
   NoopController() = default;
   ~NoopController() override = default;
 
+  NoopController(const NoopController&) = delete;
+  NoopController& operator=(const NoopController&) = delete;
+
   // WebEngineDevToolsController implementation:
   void OnContextCreated() override {}
   void OnContextDestroyed() override {}
@@ -95,9 +99,6 @@ class NoopController : public WebEngineDevToolsController {
   void GetDevToolsPort(base::OnceCallback<void(uint16_t)> callback) override {
     std::move(callback).Run(0);
   }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(NoopController);
 };
 
 // "User-mode" makes DevTools accessible to remote devices for Frames specified
@@ -107,9 +108,11 @@ class UserModeController : public WebEngineDevToolsController {
  public:
   explicit UserModeController(uint16_t server_port)
       : ip_endpoint_(net::IPAddress::IPv6AllZeros(), server_port) {}
-
   ~UserModeController() override { DCHECK(!is_remote_debugging_started_); }
 
+  UserModeController(const UserModeController&) = delete;
+  UserModeController& operator=(const UserModeController&) = delete;
+
   // WebEngineDevToolsController implementation:
   void OnContextCreated() override {}
   void OnContextDestroyed() override {
@@ -188,16 +191,12 @@ class UserModeController : public WebEngineDevToolsController {
   base::flat_set<content::WebContents*> debuggable_contents_;
 
   std::vector<base::OnceCallback<void(uint16_t)>> get_port_callbacks_;
-
-  DISALLOW_COPY_AND_ASSIGN(UserModeController);
 };
 
 // "Debug-mode" is used for on-device testing, and makes all Frames available
-// for debugging by clients on the same device. DevTools is only started when
+// for debugging by clients on the same device. DevTools is only reported when
 // the first Frame finishes loading its main document, so that the
 // DevToolsPerContextListeners can start interacting with it immediately.
-// Derives from NoopController to inherit its handling of requests for
-// user-mode debugging, and for the DevTools port, by clients.
 class DebugModeController : public WebEngineDevToolsController {
  public:
   explicit DebugModeController(
@@ -206,9 +205,11 @@ class DebugModeController : public WebEngineDevToolsController {
       devtools_listeners_.AddInterfacePtr(std::move(listener));
     }
   }
-
   ~DebugModeController() override = default;
 
+  DebugModeController(const DebugModeController&) = delete;
+  DebugModeController& operator=(const DebugModeController&) = delete;
+
   // DevToolsController implementation:
   void OnContextCreated() override {
     StartRemoteDebuggingServer(
@@ -263,8 +264,94 @@ class DebugModeController : public WebEngineDevToolsController {
 
   fidl::InterfacePtrSet<fuchsia::web::DevToolsPerContextListener>
       devtools_listeners_;
+};
+
+// "Mixed-mode" is used when both user and debug remote debugging are active at
+// the same time. The service lifespan is tied to the Context and all Frames are
+// available for remote debugging.
+class MixedModeController : public WebEngineDevToolsController {
+ public:
+  explicit MixedModeController(
+      std::vector<fuchsia::web::DevToolsPerContextListenerPtr> listeners,
+      uint16_t server_port)
+      : ip_endpoint_(net::IPAddress::IPv6AllZeros(), server_port) {
+    for (auto& listener : listeners) {
+      devtools_listeners_.AddInterfacePtr(std::move(listener));
+    }
+  }
+  ~MixedModeController() override = default;
+
+  MixedModeController(const MixedModeController&) = delete;
+  MixedModeController& operator=(const MixedModeController&) = delete;
+
+  // WebEngineDevToolsController implementation:
+  void OnContextCreated() override {
+    StartRemoteDebuggingServer(
+        base::BindOnce(&MixedModeController::OnDevToolsPortChanged,
+                       base::Unretained(this)),
+        ip_endpoint_);
+  }
+  void OnContextDestroyed() override {
+    content::DevToolsAgentHost::StopRemoteDebuggingServer();
+  }
+  bool OnFrameCreated(content::WebContents* contents,
+                      bool user_debugging) override {
+    return true;
+  }
+  void OnFrameLoaded(content::WebContents* contents) override {
+    frame_loaded_ = true;
+    MaybeSendRemoteDebuggingCallbacks();
+  }
+  void OnFrameDestroyed(content::WebContents* contents) override {}
+  content::DevToolsAgentHost::List RemoteDebuggingTargets() override {
+    return content::DevToolsAgentHost::GetOrCreateAll();
+  }
+  void GetDevToolsPort(base::OnceCallback<void(uint16_t)> callback) override {
+    get_port_callbacks_.emplace_back(std::move(callback));
+    MaybeNotifyGetPortCallbacks();
+  }
+
+ private:
+  void OnDevToolsPortChanged(uint16_t port) {
+    devtools_port_ = port;
+    MaybeNotifyGetPortCallbacks();
+    MaybeSendRemoteDebuggingCallbacks();
+  }
+
+  void MaybeNotifyGetPortCallbacks() {
+    if (!devtools_port_)
+      return;
+    for (auto& callback : get_port_callbacks_)
+      std::move(callback).Run(devtools_port_.value());
+    get_port_callbacks_.clear();
+  }
 
-  DISALLOW_COPY_AND_ASSIGN(DebugModeController);
+  void MaybeSendRemoteDebuggingCallbacks() {
+    if (!frame_loaded_ || !devtools_port_)
+      return;
+
+    // If |devtools_port_| is valid then notify all listeners, otherwise
+    // disconnect them.
+    if (devtools_port_.value() == 0) {
+      devtools_listeners_.CloseAll();
+    } else {
+      for (const auto& listener : devtools_listeners_.ptrs()) {
+        listener->get()->OnHttpPortOpen(devtools_port_.value());
+      }
+    }
+  }
+
+  const net::IPEndPoint ip_endpoint_;
+
+  // Currently active DevTools port. Set to 0 on service startup error.
+  base::Optional<uint16_t> devtools_port_;
+
+  std::vector<base::OnceCallback<void(uint16_t)>> get_port_callbacks_;
+
+  bool frame_loaded_ = false;
+
+  fidl::InterfacePtrSet<fuchsia::web::DevToolsPerContextListener>
+      devtools_listeners_;
 };
 
 }  //  namespace
@@ -273,6 +360,7 @@ class DebugModeController : public WebEngineDevToolsController {
 std::unique_ptr<WebEngineDevToolsController>
 WebEngineDevToolsController::CreateFromCommandLine(
     const base::CommandLine& command_line) {
+  base::Optional<uint16_t> devtools_port;
   if (command_line.HasSwitch(switches::kRemoteDebuggingPort)) {
     // Set up DevTools to listen on all network routes on the command-line
     // provided port.
@@ -289,12 +377,12 @@ WebEngineDevToolsController::CreateFromCommandLine(
         (!net::IsPortValid(parsed_port) || net::IsWellKnownPort(parsed_port))) {
       LOG(WARNING) << "Invalid HTTP debugger service port number "
                    << command_line_port_value;
-      return nullptr;
+    } else {
+      devtools_port = parsed_port;
     }
-
-    return std::make_unique<UserModeController>(parsed_port);
   }
 
+  std::vector<fuchsia::web::DevToolsPerContextListenerPtr> listeners;
   if (command_line.HasSwitch(switches::kRemoteDebuggerHandles)) {
     // Initialize the Debug devtools listeners.
     std::string handle_ids_str =
@@ -302,7 +390,6 @@ WebEngineDevToolsController::CreateFromCommandLine(
 
     // Extract individual handle IDs from the comma-separated list.
     base::StringTokenizer tokenizer(handle_ids_str, ",");
-    std::vector<fuchsia::web::DevToolsPerContextListenerPtr> listeners;
     while (tokenizer.GetNext()) {
       uint32_t handle_id = 0;
       if (!base::StringToUint(tokenizer.token(), &handle_id))
@@ -311,9 +398,18 @@ WebEngineDevToolsController::CreateFromCommandLine(
       listener.Bind(zx::channel(zx_take_startup_handle(handle_id)));
       listeners.emplace_back(std::move(listener));
     }
+  }
 
+  if (devtools_port) {
+    if (listeners.empty()) {
+      return std::make_unique<UserModeController>(devtools_port.value());
+    } else {
+      return std::make_unique<MixedModeController>(std::move(listeners),
+                                                   devtools_port.value());
+    }
+  } else if (listeners.empty()) {
+    return std::make_unique<NoopController>();
+  } else {
     return std::make_unique<DebugModeController>(std::move(listeners));
   }
-
-  return std::make_unique<NoopController>();
 }

fuchsia/engine/context_provider_impl.cc

@@ -232,7 +232,9 @@ void ContextProviderImpl::Create(
     launch_command.AppendSwitchNative(
         switches::kRemoteDebuggingPort,
         base::NumberToString(params.remote_debugging_port()));
-  } else if (devtools_listeners_.size() != 0) {
+  }
+
+  if (devtools_listeners_.size() != 0) {
     // Connect DevTools listeners to the new Context process.
     std::vector<std::string> handles_ids;
     for (auto& devtools_listener : devtools_listeners_.ptrs()) {

fuchsia/engine/web_engine_debug_integration_test.cc

@@ -111,10 +111,13 @@ class WebEngineDebugIntegrationTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(WebEngineDebugIntegrationTest);
 };
 
+enum class UserModeDebugging { kEnabled = 0, kDisabled = 1 };
+
 // Helper struct to intiialize all data necessary for a Context to create a
 // Frame and navigate it to a specific URL.
 struct TestContextAndFrame {
   explicit TestContextAndFrame(fuchsia::web::ContextProvider* context_provider,
+                               UserModeDebugging user_mode_debugging,
                                std::string url) {
     // Create a Context, a Frame and navigate it to |url|.
     auto directory = base::fuchsia::OpenDirectory(
@@ -124,6 +127,8 @@ struct TestContextAndFrame {
 
     fuchsia::web::CreateContextParams create_params;
     create_params.set_service_directory(std::move(directory));
+    if (user_mode_debugging == UserModeDebugging::kEnabled)
+      create_params.set_remote_debugging_port(0);
     context_provider->Create(std::move(create_params), context.NewRequest());
     context->CreateFrame(frame.NewRequest());
     frame->GetNavigationController(controller.NewRequest());
@@ -148,7 +153,8 @@ struct TestContextAndFrame {
 // Test the Debug service is properly started and accessible.
 TEST_F(WebEngineDebugIntegrationTest, DebugService) {
   std::string url = test_server_.GetURL("/title1.html").spec();
-  TestContextAndFrame frame_data(web_context_provider_.get(), url);
+  TestContextAndFrame frame_data(web_context_provider_.get(),
+                                 UserModeDebugging::kDisabled, url);
   ASSERT_TRUE(frame_data.context);
 
   // Test the debug information is correct.
@@ -175,7 +181,8 @@ TEST_F(WebEngineDebugIntegrationTest, DebugService) {
 
 TEST_F(WebEngineDebugIntegrationTest, MultipleDebugClients) {
   std::string url1 = test_server_.GetURL("/title1.html").spec();
-  TestContextAndFrame frame_data1(web_context_provider_.get(), url1);
+  TestContextAndFrame frame_data1(web_context_provider_.get(),
+                                  UserModeDebugging::kDisabled, url1);
   ASSERT_TRUE(frame_data1.context);
 
   // Test the debug information is correct.
@@ -204,7 +211,8 @@ TEST_F(WebEngineDebugIntegrationTest, MultipleDebugClients) {
 
   // Create a second Context, a second Frame and navigate it to title2.html.
   std::string url2 = test_server_.GetURL("/title2.html").spec();
-  TestContextAndFrame frame_data2(web_context_provider_.get(), url2);
+  TestContextAndFrame frame_data2(web_context_provider_.get(),
+                                  UserModeDebugging::kDisabled, url2);
   ASSERT_TRUE(frame_data2.context);
 
   // Ensure each DevTools listener has the right information.
@@ -238,3 +246,45 @@ TEST_F(WebEngineDebugIntegrationTest, MultipleDebugClients) {
   dev_tools_listener_.RunUntilNumberOfPortsIs(0);
   dev_tools_listener2.RunUntilNumberOfPortsIs(0);
 }
+
+// Test the Debug service is accessible when the User service is requested.
+TEST_F(WebEngineDebugIntegrationTest, DebugAndUserService) {
+  std::string url = test_server_.GetURL("/title1.html").spec();
+  TestContextAndFrame frame_data(web_context_provider_.get(),
+                                 UserModeDebugging::kEnabled, url);
+  ASSERT_TRUE(frame_data.context);
+
+  dev_tools_listener_.RunUntilNumberOfPortsIs(1u);
+
+  // Check we are getting the same port on both the debug and user APIs.
+  base::RunLoop run_loop;
+  cr_fuchsia::ResultReceiver<
+      fuchsia::web::Context_GetRemoteDebuggingPort_Result>
+      port_receiver(run_loop.QuitClosure());
+  frame_data.context->GetRemoteDebuggingPort(
+      cr_fuchsia::CallbackToFitFunction(port_receiver.GetReceiveCallback()));
+  run_loop.Run();
+
+  ASSERT_TRUE(port_receiver->is_response());
+  uint16_t remote_debugging_port = port_receiver->response().port;
+  ASSERT_EQ(remote_debugging_port, *dev_tools_listener_.debug_ports().begin());
+
+  // Test the debug information is correct.
+  base::Value devtools_list =
+      cr_fuchsia::GetDevToolsListFromPort(remote_debugging_port);
+  ASSERT_TRUE(devtools_list.is_list());
+  EXPECT_EQ(devtools_list.GetList().size(), 1u);
+
+  base::Value* devtools_url = devtools_list.GetList()[0].FindPath("url");
+  ASSERT_TRUE(devtools_url->is_string());
+  EXPECT_EQ(devtools_url->GetString(), url);
+
+  base::Value* devtools_title = devtools_list.GetList()[0].FindPath("title");
+  ASSERT_TRUE(devtools_title->is_string());
+  EXPECT_EQ(devtools_title->GetString(), "title 1");
+
+  // Unbind the context and wait for the listener to no longer have any active
+  // DevTools port.
+  frame_data.context.Unbind();
+  dev_tools_listener_.RunUntilNumberOfPortsIs(0);
+}