Bug 1819029 - [marionette] Fix returning response wrapped in "value" …

…field. r=webdriver-reviewers,jdescottes Differential Revision: https://phabricator.services.mozilla.com/D171084
jamienicol · Feb 28, 2023 · 7dcf557 · 7dcf557
1 parent 22676d9
commit 7dcf557
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 36 deletions.
diff --git a/remote/marionette/driver.sys.mjs b/remote/marionette/driver.sys.mjs
@@ -645,7 +645,7 @@ GeckoDriver.prototype.getContext = function() {
  *     If an element that was passed as part of <var>args</var> or that is
  *     returned as result has gone stale.
  */
-GeckoDriver.prototype.executeScript = async function(cmd) {
+GeckoDriver.prototype.executeScript = function(cmd) {
   let { script, args } = cmd.parameters;
   let opts = {
     script: cmd.parameters.script,
@@ -656,7 +656,7 @@ GeckoDriver.prototype.executeScript = async function(cmd) {
     line: cmd.parameters.line,
   };
 
-  return { value: await this.execute_(script, args, opts) };
+  return this.execute_(script, args, opts);
 };
 
 /**
@@ -717,7 +717,7 @@ GeckoDriver.prototype.executeScript = async function(cmd) {
  *     If an element that was passed as part of <var>args</var> or that is
  *     returned as result has gone stale.
  */
-GeckoDriver.prototype.executeAsyncScript = async function(cmd) {
+GeckoDriver.prototype.executeAsyncScript = function(cmd) {
   let { script, args } = cmd.parameters;
   let opts = {
     script: cmd.parameters.script,
@@ -729,7 +729,7 @@ GeckoDriver.prototype.executeAsyncScript = async function(cmd) {
     async: true,
   };
 
-  return { value: await this.execute_(script, args, opts) };
+  return this.execute_(script, args, opts);
 };
 
 GeckoDriver.prototype.execute_ = async function(
@@ -2958,7 +2958,7 @@ GeckoDriver.prototype.setupReftest = async function(cmd) {
 };
 
 /** Run a reftest. */
-GeckoDriver.prototype.runReftest = async function(cmd) {
+GeckoDriver.prototype.runReftest = function(cmd) {
   let {
     test,
     references,
@@ -2979,17 +2979,15 @@ GeckoDriver.prototype.runReftest = async function(cmd) {
   lazy.assert.string(expected);
   lazy.assert.array(references);
 
-  return {
-    value: await this._reftest.run(
-      test,
-      references,
-      expected,
-      timeout,
-      pageRanges,
-      width,
-      height
-    ),
-  };
+  return this._reftest.run(
+    test,
+    references,
+    expected,
+    timeout,
+    pageRanges,
+    width,
+    height
+  );
 };
 
 /**
@@ -3088,9 +3086,7 @@ GeckoDriver.prototype.print = async function(cmd) {
     printSettings
   );
 
-  return {
-    value: btoa(binaryString),
-  };
+  return btoa(binaryString);
 };
 
 GeckoDriver.prototype.setPermission = async function(cmd) {

diff --git a/remote/marionette/server.sys.mjs b/remote/marionette/server.sys.mjs
@@ -16,7 +16,6 @@ ChromeUtils.defineESModuleGetters(lazy, {
   MarionettePrefs: "chrome://remote/content/marionette/prefs.sys.mjs",
   Message: "chrome://remote/content/marionette/message.sys.mjs",
   Response: "chrome://remote/content/marionette/message.sys.mjs",
-  WebReference: "chrome://remote/content/marionette/element.sys.mjs",
 });
 
 XPCOMUtils.defineLazyGetter(lazy, "logger", () =>
@@ -302,11 +301,37 @@ export class TCPConnection {
 
     let rv = await fn.bind(this.driver)(cmd);
 
+    // Bug 1819029: Some older commands cannot return a response wrapped within
+    // a value field because it would break compatibility with geckodriver and
+    // Marionette client. It's unlikely that we are going to fix that.
+    //
+    // Warning: No more commands should be added to this list!
+    const commandsNoValueResponse = [
+      "Marionette:Quit",
+      "WebDriver:FindElements",
+      "WebDriver:CloseChromeWindow",
+      "WebDriver:CloseWindow",
+      "WebDriver:FullscreenWindow",
+      "WebDriver:GetCookies",
+      "WebDriver:GetElementRect",
+      "WebDriver:GetTimeouts",
+      "WebDriver:GetWindowHandles",
+      "WebDriver:GetWindowRect",
+      "WebDriver:MaximizeWindow",
+      "WebDriver:MinimizeWindow",
+      "WebDriver:NewSession",
+      "WebDriver:NewWindow",
+      "WebDriver:SetWindowRect",
+    ];
+
     if (rv != null) {
-      if (lazy.WebReference.isReference(rv) || typeof rv != "object") {
-        resp.body = { value: rv };
-      } else {
+      // By default the Response' constructor sets the body to `{ value: null }`.
+      // As such we only want to override the value if it's neither `null` nor
+      // `undefined`.
+      if (commandsNoValueResponse.includes(cmd.name)) {
         resp.body = rv;
+      } else {
+        resp.body.value = rv;
       }
     }
   }

diff --git a/testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py b/testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py
@@ -442,7 +442,9 @@ def test_in_app_quit_forced_because_callback_does_not_shutdown(self):
 
         self.marionette.start_session()
 
-    @unittest.skipIf(mozinfo.info["ccov"], "Bug 1789085 - Lost ServerSocket connection")
+    @unittest.skipIf(
+        mozinfo.info.get("ccov"), "Bug 1789085 - Lost ServerSocket connection"
+    )
     def test_in_app_quit_with_callback_that_raises_an_exception(self):
         def errorneous_callback():
             raise Exception("foo")

diff --git a/testing/web-platform/meta/webdriver/tests/get_element_property/get.py.ini b/testing/web-platform/meta/webdriver/tests/get_element_property/get.py.ini
@@ -1,16 +1,4 @@
 [get.py]
-  [test_primitives[js_primitive2-py_primitive2\]]
-    expected: FAIL
-
-  [test_primitives[js_primitive3-py_primitive3\]]
-    expected: FAIL
-
-  [test_primitives_set_by_execute_script[js_primitive2-py_primitive2\]]
-    expected: FAIL
-
-  [test_primitives_set_by_execute_script[js_primitive3-py_primitive3\]]
-    expected: FAIL
-
   [test_web_reference[frame-Frame\]]
     bug: 1274251
     expected: FAIL