Bug 895982 - JS debugger: use of promises in server doesn't preserve request/reply ordering. r=jimb

toolkit/devtools/server/main.js

@@ -716,6 +716,15 @@ function DebuggerServerConnection(aPrefix, aTransport)
   this._actorPool = new ActorPool(this);
   this._extraPools = [];
 
+  // Responses to a given actor must be returned the the client
+  // in the same order as the requests that they're replying to, but
+  // Implementations might finish serving requests in a different
+  // order.  To keep things in order we generate a promise for each
+  // request, chained to the promise for the request before it.
+  // This map stores the latest request promise in the chain, keyed
+  // by an actor ID string.
+  this._actorResponses = new Map;
+
   /*
    * We can forward packets to other servers, if the actors on that server
    * all use a distinct prefix on their names. This is a map from prefixes
@@ -942,19 +951,23 @@ DebuggerServerConnection.prototype = {
       return;
     }
 
-    resolve(ret)
-      .then(function (aResponse) {
-        if (!aResponse.from) {
-          aResponse.from = aPacket.to;
-        }
-        return aResponse;
-      })
-      .then(this.transport.send.bind(this.transport))
-      .then(null, (e) => {
-        return this._unknownError(
-          "error occurred while processing '" + aPacket.type,
-          e);
-      });
+    let pendingResponse = this._actorResponses.get(actor.actorID) || resolve(null);
+    let response = pendingResponse.then(() => {
+      return ret;
+    }).then(aResponse => {
+      if (!aResponse.from) {
+        aResponse.from = aPacket.to;
+      }
+      this.transport.send(aResponse);
+    }).then(null, (e) => {
+      let errorPacket = this._unknownError(
+        "error occurred while processing '" + aPacket.type,
+        e);
+      errorPacket.from = aPacket.to;
+      this.transport.send(errorPacket);
+    });
+
+    this._actorResponses.set(actor.actorID, response);
   },
 
   /**

toolkit/devtools/server/protocol.js

@@ -824,6 +824,12 @@ let Actor = Class({
       message: err.toString()
     });
   },
+
+  _queueResponse: function(create) {
+    let pending = this._pendingResponse || promise.resolve(null);
+    let response = create(pending);
+    this._pendingResponse = response;
+  }
 });
 exports.Actor = Actor;
 
@@ -915,14 +921,16 @@ let actorProto = function(actorProto) {
           conn.send(response);
         };
 
-        if (ret && ret.then) {
-          ret.then(sendReturn).then(null, this.writeError.bind(this));
-        } else {
-          sendReturn(ret);
-        }
-
+        this._queueResponse(p => {
+          return p
+            .then(() => ret)
+            .then(sendReturn)
+            .then(null, this.writeError.bind(this));
+        })
       } catch(e) {
-        this.writeError(e);
+        this._queueResponse(p => {
+          return p.then(() => this.writeError(e));
+        });
       }
     };
 

toolkit/devtools/server/tests/unit/test_protocol_async.js

@@ -0,0 +1,165 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Make sure we get replies in the same order that we sent their
+ * requests even when earlier requests take several event ticks to
+ * complete.
+ */
+
+let protocol = devtools.require("devtools/server/protocol");
+let {method, Arg, Option, RetVal} = protocol;
+let promise = devtools.require("sdk/core/promise");
+let events = devtools.require("sdk/event/core");
+
+function simpleHello() {
+  return {
+    from: "root",
+    applicationType: "xpcshell-tests",
+    traits: [],
+  }
+}
+
+let RootActor = protocol.ActorClass({
+  typeName: "root",
+  initialize: function(conn) {
+    protocol.Actor.prototype.initialize.call(this, conn);
+    // Root actor owns itself.
+    this.manage(this);
+    this.actorID = "root";
+    this.sequence = 0;
+  },
+
+  sayHello: simpleHello,
+
+  simpleReturn: method(function() {
+    return this.sequence++;
+  }, {
+    response: { value: RetVal() },
+  }),
+
+  promiseReturn: method(function(toWait) {
+    // Guarantee that this resolves after simpleReturn returns.
+    let deferred = promise.defer();
+    let sequence = this.sequence++;
+
+    // Wait until the number of requests specified by toWait have
+    // happened, to test queuing.
+    let check = () => {
+      if ((this.sequence - sequence) < toWait) {
+        do_execute_soon(check);
+        return;
+      }
+      deferred.resolve(sequence);
+    }
+    do_execute_soon(check);
+
+    return deferred.promise;
+  }, {
+    request: { toWait: Arg(0, "number") },
+    response: { value: RetVal("number") },
+  }),
+
+  simpleThrow: method(function() {
+    throw new Error(this.sequence++);
+  }, {
+    response: { value: RetVal("number") }
+  }),
+
+  promiseThrow: method(function() {
+    // Guarantee that this resolves after simpleReturn returns.
+    let deferred = promise.defer();
+    let sequence = this.sequence++;
+    // This should be enough to force a failure if the code is broken.
+    do_timeout(150, () => {
+      deferred.reject(sequence++);
+    });
+    return deferred.promise;
+  }, {
+    response: { value: RetVal("number") },
+  })
+});
+
+let RootFront = protocol.FrontClass(RootActor, {
+  initialize: function(client) {
+    this.actorID = "root";
+    protocol.Front.prototype.initialize.call(this, client);
+    // Root owns itself.
+    this.manage(this);
+  }
+});
+
+function run_test()
+{
+  DebuggerServer.createRootActor = RootActor;
+  DebuggerServer.init(() => true);
+
+  let trace = connectPipeTracing();
+  let client = new DebuggerClient(trace);
+  let rootClient;
+
+  client.connect((applicationType, traits) => {
+    rootClient = RootFront(client);
+
+    let calls = [];
+    let sequence = 0;
+
+    // Execute a call that won't finish processing until 2
+    // more calls have happened
+    calls.push(rootClient.promiseReturn(2).then(ret => {
+      do_check_eq(sequence, 0); // Check right return order
+      do_check_eq(ret, sequence++); // Check request handling order
+    }));
+
+    // Put a few requests into the backlog
+
+    calls.push(rootClient.simpleReturn().then(ret => {
+      do_check_eq(sequence, 1); // Check right return order
+      do_check_eq(ret, sequence++); // Check request handling order
+    }));
+
+    calls.push(rootClient.simpleReturn().then(ret => {
+      do_check_eq(sequence, 2); // Check right return order
+      do_check_eq(ret, sequence++); // Check request handling order
+    }));
+
+    calls.push(rootClient.simpleThrow().then(() => {
+      do_check_true(false, "simpleThrow shouldn't succeed!");
+    }, error => {
+      do_check_eq(sequence++, 3); // Check right return order
+      return promise.resolve(null);
+    }));
+
+    calls.push(rootClient.promiseThrow().then(() => {
+      do_check_true(false, "promiseThrow shouldn't succeed!");
+    }, error => {
+      do_check_eq(sequence++, 4); // Check right return order
+      do_check_true(true, "simple throw should throw");
+      return promise.resolve(null);
+    }));
+
+    calls.push(rootClient.simpleReturn().then(ret => {
+      do_check_eq(sequence, 5); // Check right return order
+      do_check_eq(ret, sequence++); // Check request handling order
+    }));
+
+    // Break up the backlog with a long request that waits
+    // for another simpleReturn before completing
+    calls.push(rootClient.promiseReturn(1).then(ret => {
+      do_check_eq(sequence, 6); // Check right return order
+      do_check_eq(ret, sequence++); // Check request handling order
+    }));
+
+    calls.push(rootClient.simpleReturn().then(ret => {
+      do_check_eq(sequence, 7); // Check right return order
+      do_check_eq(ret, sequence++); // Check request handling order
+    }));
+
+    promise.all.apply(null, calls).then(() => {
+      client.close(() => {
+        do_test_finished();
+      });
+    })
+  });
+  do_test_pending();
+}

toolkit/devtools/server/tests/unit/xpcshell.ini

@@ -42,6 +42,7 @@ reason = bug 821285
 [test_eval-03.js]
 [test_eval-04.js]
 [test_eval-05.js]
+[test_protocol_async.js]
 [test_protocol_simple.js]
 [test_protocol_longstring.js]
 [test_protocol_children.js]