DevTools: avoid OTR profile use after free in ~Brwoser

DevToolsBrowserContextManager::OnBrowserRemoved is called from
Browser destructor and the destructor acesses the profile after
notifying its observers. If the profile is deleted synchronously
in the DevTools' observer it will cause use after free. This change
restores the logic changed in
https://chromium-review.googlesource.com/c/chromium/src/+/2141097

Bug: 1075664
Change-Id: I02b924d8a45fa6a74fdc2ccc2647033f5f86d904
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2168515Reviewed-by: Ramin Halavati <rhalavati@chromium.org>
Reviewed-by: Peter Marshall <petermarshall@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Yury Semikhatsky <yurys@chromium.org>
Cr-Commit-Position: refs/heads/master@{#763413}

chrome/browser/devtools/devtools_browser_context_manager.cc

@@ -127,7 +127,12 @@ void DevToolsBrowserContextManager::OnBrowserRemoved(Browser* browser) {
   Profile* otr_profile = it->second;
   otr_profiles_.erase(it);
   otr_profile->RemoveObserver(this);
-  ProfileDestroyer::DestroyProfileWhenAppropriate(otr_profile);
+  // We cannot delete immediately here: the profile might still be referenced
+  // during the browser tear-down process.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&ProfileDestroyer::DestroyProfileWhenAppropriate,
+                     base::Unretained(otr_profile)));
 
   std::move(pending_disposal->second).Run(true, "");
   pending_context_disposals_.erase(pending_disposal);

chrome/browser/devtools/protocol/devtools_protocol_browsertest.cc

@@ -28,13 +28,14 @@ namespace {
 
 const char kIdParam[] = "id";
 const char kMethodParam[] = "method";
+const char kParamsParam[] = "params";
 
 }  // namespace
 
 class DevToolsProtocolTest : public InProcessBrowserTest,
                              public content::DevToolsAgentHostClient {
  public:
-  DevToolsProtocolTest() : last_sent_id_(0) {}
+  DevToolsProtocolTest() = default;
 
  protected:
   typedef base::RepeatingCallback<bool(const base::Value&)> NotificationMatcher;
@@ -48,9 +49,15 @@ class DevToolsProtocolTest : public InProcessBrowserTest,
     base::StringPiece message_str(reinterpret_cast<const char*>(message.data()),
                                   message.size());
     auto parsed_message = base::JSONReader::Read(message_str);
-    auto id = parsed_message->FindIntPath("id");
-    if (id) {
-      // TODO: implement handling of results from method calls (when needed).
+    if (auto id = parsed_message->FindIntPath("id")) {
+      base::Value* result;
+      ASSERT_TRUE(result = parsed_message->FindDictPath("result"));
+      result_ = result->Clone();
+      in_dispatch_ = false;
+      if (*id && *id == waiting_for_command_result_id_) {
+        waiting_for_command_result_id_ = 0;
+        std::move(run_loop_quit_closure_).Run();
+      }
     } else {
       std::string* notification = parsed_message->FindStringPath("method");
       EXPECT_TRUE(notification);
@@ -70,13 +77,40 @@ class DevToolsProtocolTest : public InProcessBrowserTest,
   }
 
   void SendCommand(const std::string& method) {
+    SendCommand(method, base::Value(), false);
+  }
+
+  void SendCommandSync(const std::string& method) {
+    SendCommandSync(method, base::Value());
+  }
+
+  void SendCommandSync(const std::string& method, base::Value&& params) {
+    SendCommand(method, std::move(params), true);
+  }
+
+  void SendCommand(const std::string& method,
+                   base::Value&& params,
+                   bool synchronous) {
+    in_dispatch_ = true;
     base::Value command(base::Value::Type::DICTIONARY);
     command.SetKey(kIdParam, base::Value(++last_sent_id_));
     command.SetKey(kMethodParam, base::Value(method));
+    if (!params.is_none())
+      command.SetPath(kParamsParam, std::move(params));
     std::string json_command;
     base::JSONWriter::Write(command, &json_command);
     agent_host_->DispatchProtocolMessage(
         this, base::as_bytes(base::make_span(json_command)));
+    // Some messages are dispatched synchronously.
+    // Only run loop if we are not finished yet.
+    if (in_dispatch_ && synchronous)
+      WaitForResponse();
+    in_dispatch_ = false;
+  }
+
+  void WaitForResponse() {
+    waiting_for_command_result_id_ = last_sent_id_;
+    RunLoopUpdatingQuitClosure();
   }
 
   void RunLoopUpdatingQuitClosure() {
@@ -86,6 +120,12 @@ class DevToolsProtocolTest : public InProcessBrowserTest,
     run_loop.Run();
   }
 
+  void AttachToBrowser() {
+    agent_host_ = content::DevToolsAgentHost::CreateForBrowser(
+        nullptr, content::DevToolsAgentHost::CreateServerSocketCallback());
+    agent_host_->AttachClient(this);
+  }
+
   void Attach() {
     agent_host_ = content::DevToolsAgentHost::GetOrCreateFor(web_contents());
     agent_host_->AttachClient(this);
@@ -124,13 +164,15 @@ class DevToolsProtocolTest : public InProcessBrowserTest,
     return std::move(waiting_for_notification_params_);
   }
 
- private:
   // DevToolsAgentHostClient interface
   void AgentHostClosed(content::DevToolsAgentHost* agent_host) override {}
 
   scoped_refptr<content::DevToolsAgentHost> agent_host_;
-  int last_sent_id_;
+  int last_sent_id_ = 0;
   base::OnceClosure run_loop_quit_closure_;
+  bool in_dispatch_ = false;
+  int waiting_for_command_result_id_ = 0;
+  base::Value result_;
   std::vector<std::string> notifications_;
   std::vector<base::Value> notification_params_;
   std::string waiting_for_notification_;
@@ -163,6 +205,25 @@ IN_PROC_BROWSER_TEST_F(DevToolsProtocolTest,
               security_state_issue_ids->GetList().end());
 }
 
+IN_PROC_BROWSER_TEST_F(DevToolsProtocolTest, CreateDeleteContext) {
+  AttachToBrowser();
+  for (int i = 0; i < 2; i++) {
+    SendCommandSync("Target.createBrowserContext");
+    std::string* context_id_value = result_.FindStringPath("browserContextId");
+    ASSERT_TRUE(context_id_value);
+    std::string context_id = *context_id_value;
+
+    base::DictionaryValue params;
+    params.SetStringPath("url", "about:blank");
+    params.SetStringPath("browserContextId", context_id);
+    SendCommandSync("Target.createTarget", std::move(params));
+
+    params = base::DictionaryValue();
+    params.SetStringPath("browserContextId", context_id);
+    SendCommandSync("Target.disposeBrowserContext", std::move(params));
+  }
+}
+
 IN_PROC_BROWSER_TEST_F(DevToolsProtocolTest, VisibleSecurityStateSecureState) {
   net::EmbeddedTestServer https_server(net::EmbeddedTestServer::TYPE_HTTPS);
   https_server.ServeFilesFromSourceDirectory(GetChromeTestDataDir());