[DevTools] Maintain the order of pending messages.

This keeps pending messages in a list, and identifies
waiting_for_response with a hash map. In this way
we avoid reordering messages coming from the client.
Previously, we ordered some messages coming from the
client by their call_id, but this assumes that the client
sends them in order of their call id, which isn't
always true.

Change-Id: Idd87e37fff64736117a9d0d780e7bf4f20e59974
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1863631
Commit-Queue: Johannes Henkel <johannes@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706720}

content/browser/devtools/devtools_session.cc

@@ -137,42 +137,44 @@ void DevToolsSession::AttachToAgent(blink::mojom::DevToolsAgent* agent) {
   session_.set_disconnect_handler(base::BindOnce(
       &DevToolsSession::MojoConnectionDestroyed, base::Unretained(this)));
 
-  // If we auto attached to a new oopif the session is not suspended but the
-  // render frame host may be updated during navigation, so outstanding messages
-  // are resent to the new host.
-  if (!suspended_sending_messages_to_agent_) {
-    for (const auto& pair : waiting_for_response_messages_) {
-      int call_id = pair.first;
-      const WaitingMessage& message = pair.second;
-      DispatchProtocolMessageToAgent(call_id, message.method, message.message);
-    }
-  } else {
-    std::vector<SuspendedMessage> new_suspended_messages;
-    new_suspended_messages.reserve(waiting_for_response_messages_.size());
-    for (const auto& pair : waiting_for_response_messages_) {
-      int call_id = pair.first;
-      const WaitingMessage& message = pair.second;
-      if (TerminateOnCrossProcessNavigation(message.method)) {
-        // We'll not receive any responses from the old agent, so send errors to
-        // the client.
+  // Set cookie to an empty struct to reattach next time instead of attaching.
+  if (!session_state_cookie_)
+    session_state_cookie_ = blink::mojom::DevToolsSessionState::New();
+
+  // We're attaching to a new agent while suspended; therefore, messages that
+  // have been sent previously either need to be terminated or re-sent once we
+  // resume, as we will not get any responses from the old agent at this point.
+  if (suspended_sending_messages_to_agent_) {
+    for (auto it = pending_messages_.begin(); it != pending_messages_.end();) {
+      const Message& message = *it;
+      if (waiting_for_response_.count(message.call_id) &&
+          TerminateOnCrossProcessNavigation(message.method)) {
+        // Send error to the client and remove the message from pending.
         auto error = protocol::InternalResponse::createErrorResponse(
-            call_id, protocol::DispatchResponse::kServerError,
+            message.call_id, protocol::DispatchResponse::kServerError,
             kTargetClosedMessage);
-        sendProtocolResponse(call_id, std::move(error));
+        sendProtocolResponse(message.call_id, std::move(error));
+        it = pending_messages_.erase(it);
       } else {
-        new_suspended_messages.push_back(
-            {call_id, message.method, message.message});
+        // We'll send or re-send the message in ResumeSendingMessagesToAgent.
+        ++it;
       }
     }
-    suspended_messages_.insert(suspended_messages_.begin(),
-                               new_suspended_messages.begin(),
-                               new_suspended_messages.end());
-    waiting_for_response_messages_.clear();
+    waiting_for_response_.clear();
+    return;
   }
 
-  // Set cookie to an empty struct to reattach next time instead of attaching.
-  if (!session_state_cookie_)
-    session_state_cookie_ = blink::mojom::DevToolsSessionState::New();
+  // The session is not suspended but the render frame host may be updated
+  // during navigation because:
+  // - auto attached to a new OOPIF
+  // - cross-process navigation in the main frame
+  // Therefore, we re-send outstanding messages to the new host.
+  for (const Message& message : pending_messages_) {
+    if (waiting_for_response_.count(message.call_id)) {
+      DispatchProtocolMessageToAgent(message.call_id, message.method,
+                                     message.message);
+    }
+  }
 }
 
 void DevToolsSession::MojoConnectionDestroyed() {
@@ -269,13 +271,13 @@ void DevToolsSession::fallThrough(int call_id,
   // In browser-only mode, we should've handled everything in dispatcher.
   DCHECK(!browser_only_);
 
-  if (suspended_sending_messages_to_agent_) {
-    suspended_messages_.push_back({call_id, method, message});
+  auto it = pending_messages_.insert(pending_messages_.end(),
+                                     {call_id, method, message});
+  if (suspended_sending_messages_to_agent_)
     return;
-  }
 
   DispatchProtocolMessageToAgent(call_id, method, message);
-  waiting_for_response_messages_[call_id] = {method, message};
+  waiting_for_response_[call_id] = it;
 }
 
 void DevToolsSession::DispatchProtocolMessageToAgent(
@@ -305,13 +307,15 @@ void DevToolsSession::SuspendSendingMessagesToAgent() {
 void DevToolsSession::ResumeSendingMessagesToAgent() {
   DCHECK(!browser_only_);
   suspended_sending_messages_to_agent_ = false;
-  for (const SuspendedMessage& message : suspended_messages_) {
+  for (auto it = pending_messages_.begin(); it != pending_messages_.end();
+       ++it) {
+    const Message& message = *it;
+    if (waiting_for_response_.count(message.call_id))
+      continue;
     DispatchProtocolMessageToAgent(message.call_id, message.method,
                                    message.message);
-    waiting_for_response_messages_[message.call_id] = {message.method,
-                                                       message.message};
+    waiting_for_response_[message.call_id] = it;
   }
-  suspended_messages_.clear();
 }
 
 // The following methods handle responses or notifications coming from
@@ -365,7 +369,13 @@ void DevToolsSession::DispatchProtocolResponse(
     int call_id,
     blink::mojom::DevToolsSessionStatePtr updates) {
   ApplySessionStateUpdates(std::move(updates));
-  waiting_for_response_messages_.erase(call_id);
+  auto it = waiting_for_response_.find(call_id);
+  // TODO(johannes): Consider shutting down renderer instead of just
+  // dropping the message. See shutdownForBadMessage().
+  if (it == waiting_for_response_.end())
+    return;
+  pending_messages_.erase(it->second);
+  waiting_for_response_.erase(it);
   DispatchProtocolResponseOrNotification(client_, agent_host_,
                                          std::move(message));
   // |this| may be deleted at this point.

content/browser/devtools/devtools_session.h

@@ -5,10 +5,10 @@
 #ifndef CONTENT_BROWSER_DEVTOOLS_DEVTOOLS_SESSION_H_
 #define CONTENT_BROWSER_DEVTOOLS_DEVTOOLS_SESSION_H_
 
+#include <list>
 #include <map>
 #include <memory>
 #include <string>
-#include <vector>
 
 #include "base/containers/flat_map.h"
 #include "base/memory/weak_ptr.h"
@@ -115,22 +115,17 @@ class DevToolsSession : public protocol::FrontendChannel,
   HandlersMap handlers_;
   std::unique_ptr<protocol::UberDispatcher> dispatcher_;
 
-  // These messages were queued after suspending, not sent to the agent,
-  // and will be sent after resuming.
-  struct SuspendedMessage {
-    int call_id;
-    std::string method;
-    std::string message;
-  };
-  std::vector<SuspendedMessage> suspended_messages_;
   bool suspended_sending_messages_to_agent_ = false;
 
-  // These messages have been sent to agent, but did not get a response yet.
-  struct WaitingMessage {
+  struct Message {
+    int call_id;
     std::string method;
     std::string message;
   };
-  std::map<int, WaitingMessage> waiting_for_response_messages_;
+  // Messages that were sent to the agent or queued after suspending.
+  std::list<Message> pending_messages_;
+  // Pending messages that were sent and are thus waiting for a response.
+  base::flat_map<int, std::list<Message>::iterator> waiting_for_response_;
 
   // |session_state_cookie_| always corresponds to a state before
   // any of the waiting for response messages have been handled.

third_party/blink/web_tests/http/tests/inspector-protocol/runtime/eval-await-navigation-message-order-expected.txt

+Tests the order in which not finished asynchronous Runtime.evaluate calls are temintated on navigation.
+promise created (100021)
+promise created (100013)
+promise created (100017)
+receiving result 100021:
+{
+    error : {
+        code : -32000
+        message : Inspected target navigated or closed
+    }
+    id : 100021
+    sessionId : <string>
+}
+receiving result 100013:
+{
+    error : {
+        code : -32000
+        message : Inspected target navigated or closed
+    }
+    id : 100013
+    sessionId : <string>
+}
+receiving result 100017:
+{
+    error : {
+        code : -32000
+        message : Inspected target navigated or closed
+    }
+    id : 100017
+    sessionId : <string>
+}
+

third_party/blink/web_tests/http/tests/inspector-protocol/runtime/eval-await-navigation-message-order.js

+(async function(testRunner) {
+  var {page, session, dp} = await testRunner.startURL(
+      'http://first.test:8000/inspector-protocol/resources/test-page.html',
+      `Tests the order in which not finished asynchronous Runtime.evaluate calls are temintated on navigation.`);
+
+  // Runtime.enable so we can capture console.log below. This command completes.
+  await dp.Runtime.enable();
+  dp.Runtime.onConsoleAPICalled(e => { testRunner.log(e.params.args[0].value); });
+
+  const callIdsToWatch = new Set();
+
+  // Creates a promise which doesn't resolve, so therefore, we won't get a
+  // response - only once we've navigated below, we'll get an error response
+  // due to command termination. Request ID will be call_id.
+  async function createNonResolvingPromise(call_id) {
+    const saveRequestId = DevToolsAPI._requestId;
+    DevToolsAPI._requestId = call_id - 1;
+    dp.Runtime.evaluate({
+      expression: `new Promise(() => console.log('promise created (` + call_id + `)'))`,
+      awaitPromise: true
+    });
+    DevToolsAPI._requestId = saveRequestId;
+    await dp.Runtime.onceConsoleAPICalled();  // Be sure to capture the log.
+    callIdsToWatch.add(call_id);
+  }
+
+  // Override the dispatch routine so we can intercept and log the responses in
+  // the order in which they arrive.
+  const originalDispatch = DevToolsAPI.dispatchMessage;
+  DevToolsAPI.dispatchMessage = function(message) {
+    var obj = JSON.parse(message);
+    if (callIdsToWatch.has(obj.id)) {
+      testRunner.log(obj, 'receiving result ' + obj.id + ':\n', ['sessionId']);
+    }
+    originalDispatch(message);
+  }
+
+  // Now create three promises with out-of-order call ids.
+  await createNonResolvingPromise(100021);
+  await createNonResolvingPromise(100013);
+  await createNonResolvingPromise(100017);
+
+  // Navigate, this causes the error notifications to arrive - in the same order
+  // in which the original requests were sent.
+  await page.navigate('http://second.test:8000/inspector-protocol/resources/test-page.html');
+
+  testRunner.completeTest();
+})

third_party/blink/web_tests/http/tests/inspector-protocol/runtime/eval-await-navigation-paused-message-order-expected.txt

+Tests the order in which unfinished Runtime.{enable,evaluate} calls are handled around paused navigation.
+promise created (100021)
+promise created (100013)
+promise created (100017)
+Unpausing navigation ...
+receiving result 100021:
+{
+    error : {
+        code : -32000
+        message : Inspected target navigated or closed
+    }
+    id : 100021
+    sessionId : <string>
+}
+receiving result 100013:
+{
+    error : {
+        code : -32000
+        message : Inspected target navigated or closed
+    }
+    id : 100013
+    sessionId : <string>
+}
+receiving result 100017:
+{
+    error : {
+        code : -32000
+        message : Inspected target navigated or closed
+    }
+    id : 100017
+    sessionId : <string>
+}
+receiving result 100091:
+{
+    id : 100091
+    result : {
+    }
+    sessionId : <string>
+}
+receiving result 100081:
+{
+    id : 100081
+    result : {
+    }
+    sessionId : <string>
+}
+receiving result 100086:
+{
+    id : 100086
+    result : {
+    }
+    sessionId : <string>
+}
+Hi from request 10099!
+receiving result 100099:
+{
+    id : 100099
+    result : {
+        result : {
+            type : undefined
+        }
+    }
+    sessionId : <string>
+}
+

third_party/blink/web_tests/http/tests/inspector-protocol/runtime/eval-await-navigation-paused-message-order.js

+
+(async function(testRunner) {
+  var {page, session, dp} = await testRunner.startURL(
+      'http://first.test:8000/inspector-protocol/resources/test-page.html',
+      `Tests the order in which unfinished Runtime.{enable,evaluate} calls are handled around paused navigation.`);
+
+  // Unlike eval-avait-navigation-message-order.js, this test uses
+  // the Fetch domain to pause navigation; in this way we can send additional
+  // commands during the pause and observe how they're handled when navigation
+  // completes.
+
+  // Runtime.enable so we can capture console.log below. This command completes.
+  await dp.Runtime.enable();
+  dp.Runtime.onConsoleAPICalled(e => { testRunner.log(e.params.args[0].value); });
+
+  const callIdsToWatch = new Set();
+
+  // Creates a promise which doesn't resolve, so therefore, we won't get a
+  // response - only once we've navigated below, we'll get an error response
+  // due to command termination. Request ID will be call_id.
+  async function createNonResolvingPromise(call_id) {
+    const saveRequestId = DevToolsAPI._requestId;
+    DevToolsAPI._requestId = call_id - 1;
+    dp.Runtime.evaluate({
+      expression: `new Promise(() => console.log('promise created (` + call_id + `)'))`,
+      awaitPromise: true
+    });
+    DevToolsAPI._requestId = saveRequestId;
+    await dp.Runtime.onceConsoleAPICalled();  // Be sure to capture the log.
+    callIdsToWatch.add(call_id);
+  }
+
+  // Override the dispatch routine so we can intercept and log the responses in
+  // the order in which they arrive.
+  const originalDispatch = DevToolsAPI.dispatchMessage;
+  DevToolsAPI.dispatchMessage = function(message) {
+    var obj = JSON.parse(message);
+    if (callIdsToWatch.has(obj.id)) {
+      testRunner.log(obj, 'receiving result ' + obj.id + ':\n', ['sessionId']);
+    }
+    originalDispatch(message);
+  }
+
+  // Now create three promises with out-of-order call ids.
+  await createNonResolvingPromise(100021);
+  await createNonResolvingPromise(100013);
+  await createNonResolvingPromise(100017);
+
+  // This makes it so that we'll get a Fetch.requestPaused event after
+  // issuing page.navigate below.
+  await dp.Fetch.enable();
+
+  // Navigate, this causes the error notifications to arrive - in the same order
+  // in which the original requests were sent.
+  const navigatePromise = page.navigate('http://second.test:8000/inspector-protocol/resources/test-page.html');
+
+  const requestId = (await dp.Fetch.onceRequestPaused()).params.requestId;
+  // The DevToolsSession is now in suspended state. Messages will be queued
+  // but not executed until after the navigation is unpaused and completes.
+
+  // Send three Runtime.enable commands with specific command ids.
+  // We'll observe that these commands complete in the order in which they
+  // were issued, after the navigation is unpaused.
+  const saveRequestId = DevToolsAPI._requestId;
+  DevToolsAPI._requestId = 100090;
+  callIdsToWatch.add(100091);
+  dp.Runtime.enable();
+  DevToolsAPI._requestId = 100080;
+  callIdsToWatch.add(100081);
+  dp.Runtime.enable();
+  DevToolsAPI._requestId = 100085;
+  callIdsToWatch.add(100086);
+  dp.Runtime.enable();
+
+  // If we issue commands that are not idempotent, that is, commands
+  // that would get terminated on cross process navigation, while a navigation
+  // is paused, they'll get queued and issued after the navigation (just
+  // like Runtime.enable above).
+  DevToolsAPI._requestId = 100098;
+  callIdsToWatch.add(100099);
+  dp.Runtime.evaluate({
+    expression: `console.log('Hi from request 10099!')`, awaitPromise: true
+  });
+  DevToolsAPI._requestId = saveRequestId;
+
+  testRunner.log('Unpausing navigation ...');
+  await dp.Fetch.continueRequest({requestId});
+  await navigatePromise;
+
+  testRunner.completeTest();
+})