NoSuchElementException's from PhantomJS are converted into ErrorHandler$UnknownServerException

[lukeis]

Originally reported on Google Code with ID 5000

What steps will reproduce the problem?
1. Start a selenium grid node, connected to a local selenium grib hub, and using phantomjs
instances. Example command line (irrelevant config trimmed):

% java -jar selenium-server.jar -role node -browser "browserName=phantomjs,maxInstances=2"

2. In the testing client, use org.openqa.selenium.remote.RemoteWebDriver to connect
to a website.
3. Request a non-existent element using:

driver.findElement(By.id("foobar"))

What is the expected output? What do you see instead?
The client should receive a NoSuchElementException.

The client actually receives...
org.openqa.selenium.WebDriverException: Error Message => 'Unable to find element with
id 'foobar''

Selenium version:
OS: Ubuntu 12.10
Browser: phantomjs (aka. Ghostdriver)
Browser version: 1.8 (actually, current head from source control).

Reported by gazimon on 2013-01-11 03:55:44

[lukeis]

I don't pretend to understand the exception handling in RemoteWebDriver, but this patch
to getRootExceptionCause() in java/server/src/org/openqa/selenium/remote/server/rest/ResultConfig.java
seems to address the problem in a sensible way:

-    return ec.isMappableError(nextCause) ? nextCause : rootCause;
+    if (nextCause instanceof ScreenshotException) {
+        // Note that screenshot's are passed by inserting a custom exception into
+        // the cause chain, so we need to allow for this situation also.
+       nextCause = reversedChain.next();
+    }
+    if (ec.isMappableError(nextCause)) {
+       return nextCause;
+    }
+    // Fallback to the root cause
+    return rootCause;

Reported by gazimon on 2013-01-11 04:04:28

[lukeis]

Sorry, here's a better patch for getRootExceptionCause():

Index: server/src/org/openqa/selenium/remote/server/rest/ResultConfig.java
===================================================================
--- server/src/org/openqa/selenium/remote/server/rest/ResultConfig.java (revision 18450)
+++ server/src/org/openqa/selenium/remote/server/rest/ResultConfig.java (working copy)
@@ -26,6 +26,7 @@
 import org.openqa.selenium.remote.HttpCommandExecutor;
 import org.openqa.selenium.remote.JsonToBeanConverter;
 import org.openqa.selenium.remote.PropertyMunger;
+import org.openqa.selenium.remote.ScreenshotException;
 import org.openqa.selenium.remote.SessionId;
 import org.openqa.selenium.remote.SessionNotFoundException;
 import org.openqa.selenium.remote.SimplePropertyDescriptor;
@@ -354,7 +355,16 @@
       return rootCause;
     }
     Throwable nextCause = reversedChain.next();
-    return ec.isMappableError(nextCause) ? nextCause : rootCause;
+    if (nextCause instanceof ScreenshotException) {
+        // Note that screenshot's are passed by inserting a custom exception into
+        // the cause chain, so we need to allow for this situation also.
+       nextCause = reversedChain.next();
+    }
+    if (ec.isMappableError(nextCause)) {
+       return nextCause;
+    }
+    // Fallback to the root cause
+    return rootCause;
   }

   private HandlerFactory getHandlerFactory(Class<? extends RestishHandler> handlerClazz)
{

Reported by gazimon on 2013-01-11 04:33:28

[lukeis]

An alternative (or additional) approach would be for the response ErrorHandler to use
the response status to create the innermost exception (patch shown below). However,
the current implementation appears to deliberately avoid using the response status
for this purpose, so maybe this is incorrect.

--- client/src/org/openqa/selenium/remote/ErrorHandler.java (revision 18450)
+++ client/src/org/openqa/selenium/remote/ErrorHandler.java (working copy)
@@ -104,7 +104,7 @@
         message = String.valueOf(e);
       }

-      Throwable serverError = rebuildServerError(rawErrorData);
+      Throwable serverError = rebuildServerError(rawErrorData, outerErrorType);

       // If serverError is null, then the server did not provide a className (only
expected if
       // the server is a Java process) or a stack trace. The lack of a className is
OK, but
@@ -199,7 +199,8 @@
     return null;
   }

-  private Throwable rebuildServerError(Map<String, Object> rawErrorData) {
+  private Throwable rebuildServerError(Map<String, Object> rawErrorData,
+         Class<? extends WebDriverException> fallbackErrorType) {

     if (!rawErrorData.containsKey(CLASS) && !rawErrorData.containsKey(STACK_TRACE))
{
       // Not enough information for us to try to rebuild an error.
@@ -227,7 +228,13 @@
     }

     if (toReturn == null) {
-      toReturn = new UnknownServerException(message);
+      if (fallbackErrorType != null) {
+        toReturn = createThrowable(fallbackErrorType,
+                  new Class<?>[] {String.class},
+                  new Object[] {message});
+      } else {
+       toReturn = new UnknownServerException(message);
+      }
     }

     // Note: if we have a class name above, we should always have a stack trace.

Reported by gazimon on 2013-01-11 04:34:37

[lukeis]

Reproduced.

I'm not sure that it should be fixed at grid/client side. From my point of view, other
remote points worked with current code so the source of problem is at phantomjs side.

Another issue is that phantomjs at grid2 ignores webdriver.remote.quietExceptions=True
capability and tried to take screenshot.

Also be note that this issue is not reproduced with PhantomJS started locally.

Information:
* Hub started by: java -jar selenium-server-standalone-2.33.0.jar -role hub
* Node started by: java -jar selenium-server-standalone-2.33.0.jar -role node -browser
"browserName=phantomjs,maxInstances=2" -debug
* Node debug log is attached.
* Test script is attached.

Reported by a.u.savchuk on 2013-08-05 11:56:12

• Status changed: Accepted
• Labels added: Browser-PhantomJS, Component-Grid

---

• Attachment: t.py
• Attachment: node2.log

[lukeis]

Hello, thanks for the comment and analysis.

Certainly it would be good if PhantomJS honoured the webdriver.remote.quietExceptions
flag (although I note that it appears to be optional). It is also plausible that this
problem would not occur if PhantomJS supported this flag (since we see it does not
occur with other browsers that honour the quietExceptions flag).

However, that does not change the fact that there appears to be a bug in the Selenium
server code, where ResultConfig.getRootExceptionCause does not correctly handle a nested
ScreenshotException. It is reasonable to discuss whether this is expected behaviour
or a bug (I would suggest that converting a specific exception into a general exception
is a bug), but I don't think it is wise to dismiss a potential error simply because
it is not exposed by other browsers.

Reported by gazimon on 2013-08-06 00:54:10

[lukeis]

Reported by barancev on 2013-11-02 17:35:25

[lukeis]

GhostDriver/PhantomJS is reporting the error correctly.
I have very similar tests (duh!) to check the kind of exception RemoteWebDriver maps
those errors to, and it's correct.

So, this issue must lie at some level of the Selenium Server/Grid.

Reported by detronizator on 2014-01-01 15:59:43

[lukeis]

Reported by luke.semerau on 2015-09-17 17:45:10

• Labels added: Restrict-AddIssueComment-Commit