Headless: posttask protocol message replies.

Currently, headless employs post task to asynchronously dispatch
events.

This, however, breaks the assumption that certain events
should be dispatched before the message response is delivered. This
patch restores the invariant.

R=caseq, alexclarke

Change-Id: Ie58c0ee060df7874a6e378ff6962a82c303f864b
Reviewed-on: https://chromium-review.googlesource.com/1048725Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Andrey Lushnikov <lushnikov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556783}

headless/lib/browser/headless_devtools_client_impl.cc

@@ -143,17 +143,23 @@ void HeadlessDevToolsClientImpl::DispatchProtocolMessage(
     return;
   }
 
-  if (!DispatchMessageReply(*message_dict) &&
-      !DispatchEvent(std::move(message), *message_dict)) {
+  bool success = false;
+  if (message_dict->HasKey("id"))
+    success = DispatchMessageReply(std::move(message), *message_dict);
+  else
+    success = DispatchEvent(std::move(message), *message_dict);
+  if (!success)
     DLOG(ERROR) << "Unhandled protocol message: " << json_message;
-  }
 }
 
 bool HeadlessDevToolsClientImpl::DispatchMessageReply(
+    std::unique_ptr<base::Value> owning_message,
     const base::DictionaryValue& message_dict) {
   const base::Value* id_value = message_dict.FindKey("id");
-  if (!id_value)
+  if (!id_value) {
+    NOTREACHED() << "ID must be specified.";
     return false;
+  }
   auto it = pending_messages_.find(id_value->GetInt());
   if (it == pending_messages_.end()) {
     NOTREACHED() << "Unexpected reply";
@@ -164,21 +170,46 @@ bool HeadlessDevToolsClientImpl::DispatchMessageReply(
   if (!callback.callback_with_result.is_null()) {
     const base::DictionaryValue* result_dict;
     if (message_dict.GetDictionary("result", &result_dict)) {
-      std::move(callback.callback_with_result).Run(*result_dict);
+      browser_main_thread_->PostTask(
+          FROM_HERE,
+          base::BindOnce(
+              &HeadlessDevToolsClientImpl::DispatchMessageReplyWithResultTask,
+              weak_ptr_factory_.GetWeakPtr(), std::move(owning_message),
+              std::move(callback.callback_with_result), result_dict));
     } else if (message_dict.GetDictionary("error", &result_dict)) {
       auto null_value = std::make_unique<base::Value>();
       DLOG(ERROR) << "Error in method call result: " << *result_dict;
-      std::move(callback.callback_with_result).Run(*null_value);
+      browser_main_thread_->PostTask(
+          FROM_HERE,
+          base::BindOnce(
+              &HeadlessDevToolsClientImpl::DispatchMessageReplyWithResultTask,
+              weak_ptr_factory_.GetWeakPtr(), std::move(null_value),
+              std::move(callback.callback_with_result), null_value.get()));
     } else {
       NOTREACHED() << "Reply has neither result nor error";
       return false;
     }
   } else if (!callback.callback.is_null()) {
-    std::move(callback.callback).Run();
+    browser_main_thread_->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            [](base::WeakPtr<HeadlessDevToolsClientImpl> self,
+               base::OnceClosure callback) {
+              if (self)
+                std::move(callback).Run();
+            },
+            weak_ptr_factory_.GetWeakPtr(), std::move(callback.callback)));
   }
   return true;
 }
 
+void HeadlessDevToolsClientImpl::DispatchMessageReplyWithResultTask(
+    std::unique_ptr<base::Value> owning_message,
+    base::OnceCallback<void(const base::Value&)> callback,
+    const base::Value* result_dict) {
+  std::move(callback).Run(*result_dict);
+}
+
 bool HeadlessDevToolsClientImpl::DispatchEvent(
     std::unique_ptr<base::Value> owning_message,
     const base::DictionaryValue& message_dict) {

headless/public/internal/headless_devtools_client_impl.h

@@ -157,8 +157,12 @@ class HEADLESS_EXPORT HeadlessDevToolsClientImpl
                              std::unique_ptr<base::Value> params,
                              CallbackType callback);
 
-  bool DispatchMessageReply(const base::DictionaryValue& message_dict);
-
+  bool DispatchMessageReply(std::unique_ptr<base::Value> owning_message,
+                            const base::DictionaryValue& message_dict);
+  void DispatchMessageReplyWithResultTask(
+      std::unique_ptr<base::Value> owning_message,
+      base::OnceCallback<void(const base::Value&)> callback,
+      const base::Value* result_dict);
   using EventHandler = base::RepeatingCallback<void(const base::Value&)>;
   using EventHandlerMap = std::unordered_map<std::string, EventHandler>;
 

headless/public/util/compositor_controller_unittest.cc

@@ -148,6 +148,7 @@ class CompositorControllerTest : public ::testing::Test {
         mock_host_.get(),
         base::StringPrintf("{\"id\":%d,\"result\":%s}", last_command_id_,
                            result_json.c_str()));
+    task_runner_->RunPendingTasks();
   }
 
   void SendNeedsBeginFramesEvent(bool needs_begin_frames) {

headless/public/util/virtual_time_controller_test.cc

@@ -135,6 +135,7 @@ TEST_F(VirtualTimeControllerTest, MaxVirtualTimeTaskStarvationCount) {
   client_.DispatchProtocolMessage(mock_host_.get(),
                                   "{\"id\":0,\"result\":{\"virtualTimeBase\":1."
                                   "0,\"virtualTimeTicksBase\":1.0}}");
+  task_runner_->RunPendingTasks();
 
   EXPECT_TRUE(set_up_complete_);
   EXPECT_FALSE(budget_expired_);
@@ -231,6 +232,7 @@ TEST_F(VirtualTimeControllerTest, InterleavesTasksWithVirtualTime) {
   client_.DispatchProtocolMessage(mock_host_.get(),
                                   "{\"id\":0,\"result\":{\"virtualTimeBase\":1."
                                   "0,\"virtualTimeTicksBase\":1.0}}");
+  task_runner_->RunPendingTasks();
 
   EXPECT_TRUE(set_up_complete_);
   EXPECT_FALSE(budget_expired_);
@@ -305,6 +307,7 @@ TEST_F(VirtualTimeControllerTest, CanceledTask) {
   client_.DispatchProtocolMessage(mock_host_.get(),
                                   "{\"id\":0,\"result\":{\"virtualTimeBase\":1."
                                   "0,\"virtualTimeTicksBase\":1.0}}");
+  task_runner_->RunPendingTasks();
 
   EXPECT_TRUE(set_up_complete_);
   EXPECT_FALSE(budget_expired_);
@@ -399,6 +402,7 @@ TEST_F(VirtualTimeControllerTest, MultipleTasks) {
       base::StringPrintf("{\"id\":0,\"result\":{\"virtualTimeBase\":1.0,"
                          "\"virtualTimeTicksBase\":1.0}}"));
 
+  task_runner_->RunPendingTasks();
   EXPECT_TRUE(set_up_complete_);
   EXPECT_FALSE(budget_expired_);
 }