Fixing legacy Firefox driver (and atoms) to throw proper exception on…

… an attempt to return recursive object.
SeleniumHQ · Dec 8, 2017 · c86d229 · c86d229
1 parent e4ad062
commit c86d229
Show file tree

Hide file tree

Showing 4 changed files with 80 additions and 65 deletions.
diff --git a/java/client/test/org/openqa/selenium/ExecutingJavascriptTest.java b/java/client/test/org/openqa/selenium/ExecutingJavascriptTest.java
@@ -32,7 +32,6 @@
 import static org.junit.Assert.fail;
 import static org.junit.Assume.assumeTrue;
 import static org.openqa.selenium.testing.Driver.CHROME;
-import static org.openqa.selenium.testing.Driver.FIREFOX;
 import static org.openqa.selenium.testing.Driver.HTMLUNIT;
 import static org.openqa.selenium.testing.Driver.IE;
 import static org.openqa.selenium.testing.Driver.MARIONETTE;
@@ -566,7 +565,6 @@ public void shouldHandleObjectThatThatHaveToJSONMethod() {
   @Ignore(CHROME)
   @Ignore(value = IE, issue = "540")
   @Ignore(SAFARI)
-  @Ignore(value = FIREFOX, issue = "540")
   @Ignore(HTMLUNIT)
   public void shouldHandleRecursiveStructures() {
     driver.get(pages.simpleTestPage);

diff --git a/javascript/atoms/inject.js b/javascript/atoms/inject.js
@@ -121,7 +121,7 @@ bot.inject.wrapValue = function(value) {
         // a ton of compiler warnings.
         value = /**@type {!Object}*/ (value);
         if (seen.indexOf(value) >= 0) {
-          throw new bot.Error(bot.ErrorCode.UNKNOWN_ERROR,
+          throw new bot.Error(bot.ErrorCode.JAVASCRIPT_ERROR,
             'Recursive object cannot be transferred');
         }
 

diff --git a/javascript/firefox-driver/js/firefoxDriver.js b/javascript/firefox-driver/js/firefoxDriver.js
@@ -306,7 +306,11 @@ function injectAndExecuteScript(respond, parameters, isAsync, timer) {
         }
 
         var result = unwrappedDoc['__webdriver_evaluate']['result'];
-        respond.value = Utils.wrapResult(result, doc);
+        try {
+          respond.value = Utils.wrapResult(result, doc);
+        } catch (e) {
+          respond.sendError(new WebDriverError(bot.ErrorCode.JAVASCRIPT_ERROR, e));
+        }
         respond.status = unwrappedDoc['__webdriver_evaluate']['code'];
 
         // I have no idea why this started happening. Roll on m-day

diff --git a/javascript/firefox-driver/js/utils.js b/javascript/firefox-driver/js/utils.js
@@ -822,83 +822,96 @@ Utils.unwrapParameters = function(wrappedParameters, doc) {
 
 
 Utils.wrapResult = function(result, doc) {
-  result = fxdriver.moz.unwrap(result);
+  var _wrap = function(result, doc, seen) {
+    result = fxdriver.moz.unwrap(result);
 
-  // Sophisticated.
-  switch (typeof result) {
-    case 'string':
-    case 'number':
-    case 'boolean':
-      return result;
-
-    case 'function':
-      return result.toString();
+    // Sophisticated.
+    switch (typeof result) {
+      case 'string':
+      case 'number':
+      case 'boolean':
+        return result;
 
-    case 'undefined':
-      return null;
+      case 'function':
+        return result.toString();
 
-    case 'object':
-      if (result == null) {
+      case 'undefined':
         return null;
-      }
 
-      // There's got to be a more intelligent way of detecting this.
-      if (result.nodeType == 1 && result['tagName']) {
-        return {'ELEMENT': Utils.addToKnownElements(result)};
-      }
+      case 'object':
+        if (result == null) {
+          return null;
+        }
 
-      if (typeof result.getMonth === 'function') {
-        return result.toJSON();
-      }
+        if (seen.indexOf(result) >= 0) {
+          throw new bot.Error(bot.ErrorCode.JAVASCRIPT_ERROR,
+                              'Recursive object cannot be transferred');
+        }
 
-      if (typeof result.length === 'number' &&
-          !(result.propertyIsEnumerable('length'))) {
-        var array = [];
-        for (var i = 0; i < result.length; i++) {
-          array.push(Utils.wrapResult(result[i], doc));
+        // There's got to be a more intelligent way of detecting this.
+        if (result.nodeType == 1 && result['tagName']) {
+          return {'ELEMENT': Utils.addToKnownElements(result)};
         }
-        return array;
-      }
 
-      // Document. Grab the document element.
-      if (result.nodeType == 9) {
-        return Utils.wrapResult(result.documentElement);
-      }
+        if (typeof result.getMonth === 'function') {
+          return result.toJSON();
+        }
 
-      try {
-        var nodeList = result.QueryInterface(CI.nsIDOMNodeList);
-        var array = [];
-        for (var i = 0; i < nodeList.length; i++) {
-          array.push(Utils.wrapResult(result.item(i), doc));
+        seen.push(result);
+
+        if (typeof result.length === 'number' &&
+            !(result.propertyIsEnumerable('length'))) {
+          var array = [];
+          for (var i = 0; i < result.length; i++) {
+            array.push(_wrap(result[i], doc, seen));
+          }
+          return array;
+        }
+
+        // Document. Grab the document element.
+        if (result.nodeType == 9) {
+          return _wrap(result.documentElement, doc, seen);
         }
-        return array;
-      } catch (ignored) {
-        goog.log.warning(Utils.LOG_, 'Error wrapping NodeList', ignored);
-      }
 
-      try {
-        // There's got to be a better way, but 'result instanceof Error' returns false
-        if (Object.getPrototypeOf(result) != null && goog.string.endsWith(Object.getPrototypeOf(result).toString(), 'Error')) {
-          try {
-            return fxdriver.error.toJSON(result);
-          } catch (ignored2) {
-            goog.log.info(Utils.LOG_, 'Error', ignored2);
-            return result.toString();
+        var nodeList;
+        try {
+          nodeList = result.QueryInterface(CI.nsIDOMNodeList);
+        } catch (ignored) {
+        }
+        if (nodeList) {
+          var array = [];
+          for (var i = 0; i < nodeList.length; i++) {
+            array.push(_wrap(result.item(i), doc, seen));
           }
+          return array;
         }
-      } catch (ignored) {
-        goog.log.info(Utils.LOG_, 'Error', ignored);
-      }
 
-      var convertedObj = {};
-      for (var prop in result) {
-        convertedObj[prop] = Utils.wrapResult(result[prop], doc);
-      }
-      return convertedObj;
+        try {
+          // There's got to be a better way, but 'result instanceof Error' returns false
+          if (Object.getPrototypeOf(result) != null && goog.string.endsWith(
+                  Object.getPrototypeOf(result).toString(), 'Error')) {
+            try {
+              return fxdriver.error.toJSON(result);
+            } catch (ignored2) {
+              goog.log.info(Utils.LOG_, 'Error', ignored2);
+              return result.toString();
+            }
+          }
+        } catch (ignored) {
+          goog.log.info(Utils.LOG_, 'Error', ignored);
+        }
 
-    default:
-      return result;
-  }
+        var convertedObj = {};
+        for (var prop in result) {
+          convertedObj[prop] = _wrap(result[prop], doc, seen);
+        }
+        return convertedObj;
+
+      default:
+        return result;
+    }
+  };
+  return _wrap(result, doc, []);
 };