Flatten DevToolsClientImpl hierarchy in chromedriver.

It turns out we may not need to have a deeply nested hierarchy,
so this PR attempts to flatten this hierarchy as we're
creating child session instances.

This avoids a crash that would happen for grandchildren when
they're trying to use their parent's socket to send messages.

BUG=chromedriver:3165

Change-Id: I4fb22a7aeaf67321a062deb831ebc68ace904b7a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1851227
Commit-Queue: Johannes Henkel <johannes@chromium.org>
Reviewed-by: John Chen <johnchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704519}

chrome/test/chromedriver/chrome/devtools_client_impl.cc

@@ -79,8 +79,8 @@ DevToolsClientImpl::DevToolsClientImpl(const SyncWebSocketFactory& factory,
                                        const std::string& id)
     : socket_(factory.Run()),
       url_(url),
-      parent_(nullptr),
       owner_(nullptr),
+      parent_(nullptr),
       crashed_(false),
       detached_(false),
       id_(id),
@@ -99,8 +99,8 @@ DevToolsClientImpl::DevToolsClientImpl(
     const FrontendCloserFunc& frontend_closer_func)
     : socket_(factory.Run()),
       url_(url),
-      parent_(nullptr),
       owner_(nullptr),
+      parent_(nullptr),
       crashed_(false),
       detached_(false),
       id_(id),
@@ -114,9 +114,9 @@ DevToolsClientImpl::DevToolsClientImpl(
 
 DevToolsClientImpl::DevToolsClientImpl(DevToolsClientImpl* parent,
                                        const std::string& session_id)
-    : parent_(parent),
-      owner_(nullptr),
+    : owner_(nullptr),
       session_id_(session_id),
+      parent_(parent),
       crashed_(false),
       detached_(false),
       id_(session_id),
@@ -136,8 +136,8 @@ DevToolsClientImpl::DevToolsClientImpl(
     const ParserFunc& parser_func)
     : socket_(factory.Run()),
       url_(url),
-      parent_(nullptr),
       owner_(nullptr),
+      parent_(nullptr),
       crashed_(false),
       detached_(false),
       id_(id),
@@ -297,6 +297,10 @@ DevToolsClientImpl::ResponseInfo::ResponseInfo(const std::string& method)
 
 DevToolsClientImpl::ResponseInfo::~ResponseInfo() {}
 
+DevToolsClientImpl* DevToolsClientImpl::GetRootClient() {
+  return parent_ ? parent_ : this;
+}
+
 Status DevToolsClientImpl::SendCommandInternal(
     const std::string& method,
     const base::DictionaryValue& params,
@@ -324,8 +328,7 @@ Status DevToolsClientImpl::SendCommandInternal(
     VLOG(1) << "DevTools WebSocket Command: " << method << " (id=" << command_id
             << ") " << id_ << " " << FormatValueForDisplay(params);
   }
-  SyncWebSocket* socket =
-      (parent_ != nullptr) ? parent_->socket_.get() : socket_.get();
+  SyncWebSocket* socket = GetRootClient()->socket_.get();
   if (!socket->Send(message)) {
     return Status(kDisconnected, "unable to send message to renderer");
   }

chrome/test/chromedriver/chrome/devtools_client_impl.h

@@ -120,6 +120,7 @@ class DevToolsClientImpl : public DevToolsClient {
   Status HandleReceivedEvents() override;
   void SetDetached() override;
   void SetOwner(WebViewImpl* owner) override;
+  DevToolsClientImpl* GetRootClient();
 
  private:
   enum ResponseState {
@@ -164,10 +165,13 @@ class DevToolsClientImpl : public DevToolsClient {
 
   std::unique_ptr<SyncWebSocket> socket_;
   GURL url_;
-  DevToolsClientImpl* parent_;
   // WebViewImpl that owns this instance; nullptr for browser-wide DevTools.
   WebViewImpl* owner_;
   const std::string session_id_;
+  // parent_ / children_: it's a flat hierarchy - nesting is at most one level
+  // deep. children_ holds child sessions - identified by their session id -
+  // which send/receive messages via the socket_ of their parent.
+  DevToolsClientImpl* parent_;
   std::map<std::string, DevToolsClientImpl*> children_;
   bool crashed_;
   bool detached_;

chrome/test/chromedriver/chrome/web_view_impl.cc

@@ -195,10 +195,14 @@ WebViewImpl::~WebViewImpl() {}
 
 WebViewImpl* WebViewImpl::CreateChild(const std::string& session_id,
                                       const std::string& target_id) const {
-  DevToolsClientImpl* parent_client =
-      static_cast<DevToolsClientImpl*>(client_.get());
+  // While there may be a deep hierarchy of WebViewImpl instances, the
+  // hierarchy for DevToolsClientImpl is flat - there's a root which
+  // sends/receives over the socket, and all child sessions are considered
+  // its children (one level deep at most).
+  DevToolsClientImpl* root_client =
+      static_cast<DevToolsClientImpl*>(client_.get())->GetRootClient();
   std::unique_ptr<DevToolsClient> child_client(
-      std::make_unique<DevToolsClientImpl>(parent_client, session_id));
+      std::make_unique<DevToolsClientImpl>(root_client, session_id));
   WebViewImpl* child = new WebViewImpl(target_id, w3c_compliant_, browser_info_,
                                        std::move(child_client), nullptr,
                                        navigation_tracker_->IsNonBlocking()