Bug 1370850 - Serialise undefined script evaluation return value to null; r=maja_zf
☠☠ backed out by 00c3f75ed9a0 ☠ ☠
authorAndreas Tolfsen <ato@mozilla.com>
Wed, 07 Jun 2017 12:46:14 +0100
changeset 413513 eaef7cd5e2884314fb81c70d1b9576772a38e415
parent 413512 62c0be1d862980516240e8e186cbab34597ebfbf
child 413514 58a93f7d6b20b5809ed14294eabccd825aeb174f
push id1490
push usermtabara@mozilla.com
push dateMon, 31 Jul 2017 14:08:16 +0000
treeherdermozilla-release@70e32e6bf15e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmaja_zf
bugs1370850
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1370850 - Serialise undefined script evaluation return value to null; r=maja_zf When a call through the content frame proxy is interrupted by the dialogueObserver, the synchronous promise that is meant to wait for a response from the frame script is resolved immediately with an undefined return value. When an undefined value is assigned to the response body, it gets dropped during JSON serialisation. To ensure the "value" field expected from the Execute Script and Execute Async Script commands is populated, we need to assign a null value to resp.body.value. We can treat undefined as null by calling evaluate.toJSON again on the return value from the proxied frame script call. This effectively means we serialise it twice, since it first needs to be serialised to cross the IPC border, though the second computation only looks at primitives and no known web element store is required. It would be nicer if the content frame script itself would be able to return early with null by installing a user prompt notification event, but this is not possible because the tabmodal dialogue that appears blocks script execution. This means we need to rely on the dialogueObserver in testing/marionette/proxy.js to take care of the dialogue for us. MozReview-Commit-ID: D14TA2TYYXI
testing/marionette/driver.js
testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py
--- a/testing/marionette/driver.js
+++ b/testing/marionette/driver.js
@@ -866,34 +866,36 @@ GeckoDriver.prototype.executeAsyncScript
   resp.body.value = yield this.execute_(script, args, scriptTimeout, opts);
 };
 
 GeckoDriver.prototype.execute_ = function (script, args, timeout, opts = {}) {
   switch (this.context) {
     case Context.CONTENT:
       // evaluate in content with lasting side-effects
       if (!opts.sandboxName) {
-        return this.listener.execute(script, args, timeout, opts);
+        return this.listener.execute(script, args, timeout, opts)
+            .then(evaluate.toJSON);
 
       // evaluate in content with sandbox
       } else {
-        return this.listener.executeInSandbox(script, args, timeout, opts);
+        return this.listener.executeInSandbox(script, args, timeout, opts)
+            .then(evaluate.toJSON);
       }
 
     case Context.CHROME:
       let sb = this.sandboxes.get(opts.sandboxName, opts.newSandbox);
       if (opts.sandboxName) {
         sb = sandbox.augment(sb, new logging.Adapter(this.marionetteLog));
         sb = sandbox.augment(sb, {global: sb});
       }
 
       opts.timeout = timeout;
       let wargs = evaluate.fromJSON(args, this.curBrowser.seenEls, sb.window);
-      let evaluatePromise = evaluate.sandbox(sb, script, wargs, opts);
-      return evaluatePromise.then(res => evaluate.toJSON(res, this.curBrowser.seenEls));
+      return evaluate.sandbox(sb, script, wargs, opts)
+          .then(res => evaluate.toJSON(res, this.curBrowser.seenEls));
   }
 };
 
 /**
  * Navigate to given URL.
  *
  * Navigates the current browsing context to the given URL and waits for
  * the document to load or the session's page timeout duration to elapse
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py
@@ -1,17 +1,17 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import os
 import urllib
 
 from marionette_driver import By, errors
-from marionette_driver.marionette import HTMLElement
+from marionette_driver.marionette import Alert, HTMLElement
 from marionette_driver.wait import Wait
 
 from marionette_harness import MarionetteTestCase, skip_if_mobile, WindowManagerMixin
 
 
 def inline(doc):
     return "data:text/html;charset=utf-8,{}".format(urllib.quote(doc))
 
@@ -26,16 +26,33 @@ globals = set([
               "navigator",
               "URL",
               "window",
               ])
 
 
 class TestExecuteContent(MarionetteTestCase):
 
+    def alert_present(self):
+        try:
+            Alert(self.marionette).text
+            return True
+        except errors.NoAlertPresentException:
+            return False
+
+    def wait_for_alert_closed(self, timeout=None):
+        Wait(self.marionette, timeout=timeout).until(
+            lambda _: not self.alert_present())
+
+    def tearDown(self):
+        if self.alert_present():
+            alert = self.marionette.switch_to_alert()
+            alert.dismiss()
+            self.wait_for_alert_closed()
+
     def assert_is_defined(self, property, sandbox="default"):
         self.assertTrue(self.marionette.execute_script(
             "return typeof arguments[0] != 'undefined'", [property], sandbox=sandbox),
             "property {} is undefined".format(property))
 
     def assert_is_web_element(self, element):
         self.assertIsInstance(element, HTMLElement)
 
@@ -335,16 +352,21 @@ class TestExecuteContent(MarionetteTestC
             return {
               toJSON () {
                 return document.documentElement;
               }
             }""",
             sandbox=None)
         self.assert_is_web_element(el)
 
+    def test_return_value_on_alert(self):
+        res = self.marionette._send_message("executeScript", {"script": "alert()"})
+        self.assertIn("value", res)
+        self.assertIsNone(res)
+
 
 class TestExecuteChrome(WindowManagerMixin, TestExecuteContent):
 
     def setUp(self):
         super(TestExecuteChrome, self).setUp()
 
         self.marionette.set_context("chrome")
 
@@ -403,16 +425,19 @@ class TestExecuteChrome(WindowManagerMix
         pass
 
     def test_system_sandbox_wrappedjsobject(self):
         pass
 
     def test_access_chrome_objects_in_event_listeners(self):
         pass
 
+    def test_return_value_on_alert(self):
+        pass
+
 
 class TestElementCollections(MarionetteTestCase):
 
     def assertSequenceIsInstance(self, seq, typ):
         for item in seq:
             self.assertIsInstance(item, typ)
 
     def test_array(self):