Change in gwt[master]: Adds support for unwrapping of thrown non-jso objects from J...

[Goktug Gokdogan]

Goktug Gokdogan has uploaded a new change for review.

https://gwt-review.googlesource.com/3640


Change subject: Adds support for unwrapping of thrown non-jso objects from
JavaScriptException.
......................................................................

Adds support for unwrapping of thrown non-jso objects from
JavaScriptException.

This is the second part of exception wrap/unwrap fix which adds support to
unwrapping
of non-jso objects thrown by native code.

Change-Id: I09c92ac03abbdb930085e2aaf97001b4d495abd9
---
M dev/core/src/com/google/gwt/dev/shell/MethodDispatch.java
M dev/core/src/com/google/gwt/dev/shell/ModuleSpace.java
M
dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/Exceptions.java
M user/src/com/google/gwt/core/client/JavaScriptException.java
M user/src/com/google/gwt/core/client/impl/StackTraceCreator.java
M user/test/com/google/gwt/core/client/JavaScriptExceptionTest.java
6 files changed, 96 insertions(+), 76 deletions(-)



diff --git a/dev/core/src/com/google/gwt/dev/shell/MethodDispatch.java
b/dev/core/src/com/google/gwt/dev/shell/MethodDispatch.java
index 8f1c062..fb5b3a8 100644
--- a/dev/core/src/com/google/gwt/dev/shell/MethodDispatch.java
+++ b/dev/core/src/com/google/gwt/dev/shell/MethodDispatch.java
@@ -109,21 +109,14 @@
/**
* Send an exception back to the client. This will either wrap a Java
* Throwable as a Java Object to be sent over the wire, or if the
exception is
- * a JavaScriptObject unwinding through the stack, send the JSO instead.
+ * a JavaScriptException unwinding through the stack, send the original
thrown
+ * object instead.
*/
private void wrapException(JsValue returnValue, Throwable t) {
ModuleSpace.setThrownJavaException(t);

- // See if we're in the process of throwing a JavaScriptObject; remove
- // it from the JavaScriptException object and throw the JS object
instead
- Object jsoException = ModuleSpace.getJavaScriptExceptionException(
- classLoader, t);
-
- if (jsoException != null) {
- JsValueGlue.set(returnValue, classLoader, jsoException.getClass(),
- jsoException);
- } else {
- JsValueGlue.set(returnValue, classLoader, t.getClass(), t);
- }
+ Object thrown = ModuleSpace.getThrownObject(classLoader, t);
+ Class<?> type = thrown == null ? Object.class : thrown.getClass();
+ JsValueGlue.set(returnValue, classLoader, type, thrown);
}
}
diff --git a/dev/core/src/com/google/gwt/dev/shell/ModuleSpace.java
b/dev/core/src/com/google/gwt/dev/shell/ModuleSpace.java
index 4798106..6094f17 100644
--- a/dev/core/src/com/google/gwt/dev/shell/ModuleSpace.java
+++ b/dev/core/src/com/google/gwt/dev/shell/ModuleSpace.java
@@ -101,15 +101,16 @@
}

/**
- * Get the JavaScriptObject wrapped by a JavaScriptException. We have to
do
+ * Get the original thrown object. If the exception is
JavaScriptException,
+ * gets the object wrapped by a JavaScriptException. We have to do
* this reflectively, since the JavaScriptException object is from an
* arbitrary classloader. If the object is not a JavaScriptException, or
is
- * not from the given ClassLoader, we'll return null.
+ * not from the given ClassLoader, or the exception is not set we'll
return
+ * exception itself.
*/
- static Object getJavaScriptExceptionException(ClassLoader cl,
- Object javaScriptException) {
- if (javaScriptException.getClass().getClassLoader() != cl) {
- return null;
+ static Object getThrownObject(ClassLoader cl, Object exception) {
+ if (exception.getClass().getClassLoader() != cl) {
+ return exception;
}

Exception caught;
@@ -117,12 +118,16 @@
Class<?> javaScriptExceptionClass = Class.forName(
"com.google.gwt.core.client.JavaScriptException", true, cl);

- if (!javaScriptExceptionClass.isInstance(javaScriptException)) {
+ if (!javaScriptExceptionClass.isInstance(exception)) {
// Not a JavaScriptException
- return null;
+ return exception;
}
- Method getException =
javaScriptExceptionClass.getMethod("getException");
- return getException.invoke(javaScriptException);
+ Method isThrownSet =
javaScriptExceptionClass.getMethod("isThrownSet");
+ if (!((Boolean) isThrownSet.invoke(exception))) {
+ return exception;
+ }
+ Method getThrown = javaScriptExceptionClass.getMethod("getThrown");
+ return getThrown.invoke(exception);
} catch (NoSuchMethodException e) {
caught = e;
} catch (ClassNotFoundException e) {
diff --git
a/dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/Exceptions.java
b/dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/Exceptions.java
index 4e075f5..2a2301b 100644
---
a/dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/Exceptions.java
+++
b/dev/core/super/com/google/gwt/dev/jjs/intrinsic/com/google/gwt/lang/Exceptions.java
@@ -16,7 +16,6 @@
package com.google.gwt.lang;

import com.google.gwt.core.client.JavaScriptException;
-import com.google.gwt.core.client.JavaScriptObject;

/**
* This is a magic class the compiler uses to throw and check exceptions.
@@ -32,9 +31,9 @@

static Object unwrap(Object e) {
if (e instanceof JavaScriptException) {
- JavaScriptObject exception = ((JavaScriptException)
e).getException();
- if (exception != null) {
- return exception;
+ JavaScriptException jse = ((JavaScriptException) e);
+ if (jse.isThrownSet()) {
+ return jse.getThrown();
}
}
return e;
diff --git a/user/src/com/google/gwt/core/client/JavaScriptException.java
b/user/src/com/google/gwt/core/client/JavaScriptException.java
index 8f15edd..85d90f0 100644
--- a/user/src/com/google/gwt/core/client/JavaScriptException.java
+++ b/user/src/com/google/gwt/core/client/JavaScriptException.java
@@ -45,6 +45,8 @@
*/
public final class JavaScriptException extends RuntimeException {

+ private static final Object NOT_SET = new Object();
+
private static String getExceptionDescription(Object e) {
if (e instanceof JavaScriptObject) {
return getExceptionDescription0((JavaScriptObject) e);
@@ -133,7 +135,7 @@
this.message = "JavaScript " + name + " exception: " + description;
this.name = name;
this.description = description;
- this.e = null;
+ this.e = NOT_SET;
}

/**
@@ -144,7 +146,21 @@
protected JavaScriptException(String message) {
super(message);
this.message = this.description = message;
- this.e = null;
+ this.e = NOT_SET;
+ }
+
+ /**
+ * Returns {@code true} if a thrown object is not set for the exception.
+ */
+ public boolean isThrownSet() {
+ return e != NOT_SET;
+ }
+
+ /**
+ * Returns the original thrown object from javascript; may be {@code
null}.
+ */
+ public Object getThrown() {
+ return e == NOT_SET ? null : e;
}

/**
@@ -152,24 +168,23 @@
* <code>null</code>.
*/
public String getDescription() {
- if (message == null) {
- init();
- }
+ ensureInit();
return description;
}

/**
* Returns the original JavaScript the exception; may be
<code>null</code>.
+ *
+ * @deprecated deprecated in favor for {@link #getThrown()} and {@link
#isThrownSet()}
*/
+ @Deprecated
public JavaScriptObject getException() {
return (e instanceof JavaScriptObject) ? (JavaScriptObject) e : null;
}

@Override
public String getMessage() {
- if (message == null) {
- init();
- }
+ ensureInit();
return message;
}

@@ -178,16 +193,17 @@
* <code>null</code>.
*/
public String getName() {
- if (message == null) {
- init();
- }
+ ensureInit();
return name;
}

- private void init() {
- name = getExceptionName(e);
- description = description + ": " + getExceptionDescription(e);
- message = "(" + name + ") " + getExceptionProperties(e) + description;
+ private void ensureInit() {
+ if (message == null) {
+ Object exception = getThrown();
+ name = getExceptionName(exception);
+ description = description + ": " +
getExceptionDescription(exception);
+ message = "(" + name + ") " + getExceptionProperties(exception) +
description;
+ }
}

}
diff --git
a/user/src/com/google/gwt/core/client/impl/StackTraceCreator.java
b/user/src/com/google/gwt/core/client/impl/StackTraceCreator.java
index 8d804b1..58bb0f4 100644
--- a/user/src/com/google/gwt/core/client/impl/StackTraceCreator.java
+++ b/user/src/com/google/gwt/core/client/impl/StackTraceCreator.java
@@ -74,7 +74,7 @@
}-*/;

public void createStackTrace(JavaScriptException e) {
- JsArrayString stack = inferFrom(e.getException());
+ JsArrayString stack = inferFrom(e.getThrown());

StackTraceElement[] stackTrace = new
StackTraceElement[stack.length()];
for (int i = 0, j = stackTrace.length; i < j; i++) {
@@ -121,7 +121,7 @@
*
* @param e a JavaScriptObject
*/
- public JsArrayString inferFrom(JavaScriptObject e) {
+ public JsArrayString inferFrom(Object e) {
return JavaScriptObject.createArray().cast();
}

@@ -206,7 +206,7 @@
}

@Override
- public JsArrayString inferFrom(JavaScriptObject e) {
+ public JsArrayString inferFrom(Object e) {
throw new RuntimeException("Should not reach here");
}

@@ -237,8 +237,9 @@
}

@Override
- public JsArrayString inferFrom(JavaScriptObject e) {
- JsArrayString stack = getStack(e);
+ public JsArrayString inferFrom(Object e) {
+ JavaScriptObject jso = (e instanceof JavaScriptObject) ?
(JavaScriptObject) e : null;
+ JsArrayString stack = getStack(jso);
for (int i = 0, j = stack.length(); i < j; i++) {
stack.set(i, extractName(stack.get(i)));
}
@@ -296,7 +297,7 @@

@Override
public void createStackTrace(JavaScriptException e) {
- JsArrayString stack = inferFrom(e.getException());
+ JsArrayString stack = inferFrom(e.getThrown());
parseStackTrace(e, stack);
}

@@ -307,7 +308,7 @@
}

@Override
- public JsArrayString inferFrom(JavaScriptObject e) {
+ public JsArrayString inferFrom(Object e) {
JsArrayString stack = super.inferFrom(e);
if (stack.length() == 0) {
// Safari should fall back to default Collector:
diff --git
a/user/test/com/google/gwt/core/client/JavaScriptExceptionTest.java
b/user/test/com/google/gwt/core/client/JavaScriptExceptionTest.java
index ef3acdf..af1272d 100644
--- a/user/test/com/google/gwt/core/client/JavaScriptExceptionTest.java
+++ b/user/test/com/google/gwt/core/client/JavaScriptExceptionTest.java
@@ -98,7 +98,7 @@

private static void assertJavaScriptException(Object expected, Throwable
exception) {
assertTrue(exception instanceof JavaScriptException);
- assertEquals(expected, ((JavaScriptException)
exception).getException());
+ assertEquals(expected, ((JavaScriptException) exception).getThrown());
}

private static void assertJsoProperties(boolean
extraPropertiesShouldBePresent) {
@@ -109,7 +109,8 @@
} catch (JavaScriptException e) {
assertEquals("myName", e.getName());
assertDescription(e, "myDescription");
- assertSame(jso, e.getException());
+ assertTrue(e.isThrownSet());
+ assertSame(jso, e.getThrown());
assertTrue(e.getMessage().contains("myName"));
assertTrue(e.getMessage().contains(e.getDescription()));
if (extraPropertiesShouldBePresent) {
@@ -182,18 +183,19 @@
e = makeJSO();
assertJavaScriptException(e, catchJava(createThrowNativeRunnable(e)));

- // TODO(goktug): non-jso objects will be supported in follow up CL
- //
- // e = "testing";
- // assertJavaScriptException(e,
catchJava(createThrowNativeRunnable(e)));
- //
- // e = makeJavaObject();
- // assertJavaScriptException(e,
catchJava(createThrowNativeRunnable(e)));
- //
- // e = null;
- // assertJavaScriptException(e,
catchJava(createThrowNativeRunnable(e)));
+ e = "testing";
+ assertJavaScriptException(e, catchJava(createThrowNativeRunnable(e)));
+
+ e = null;
+ assertJavaScriptException(e, catchJava(createThrowNativeRunnable(e)));
+
+ e = makeJavaObject();
+ assertJavaScriptException(e, catchJava(createThrowNativeRunnable(e)));

e = new RuntimeException();
+ assertSame(e, catchJava(createThrowNativeRunnable(e)));
+
+ e = new JavaScriptException("exception message"); // Thrown is not set
assertSame(e, catchJava(createThrowNativeRunnable(e)));

e = new JavaScriptException(makeJSO());
@@ -207,21 +209,22 @@
e = makeJSO();
assertSame(e, nativeJavaNativeSandwich(e));

- // TODO(goktug): non-jso objects will be supported in follow up CL
- //
- // e = "testing";
- // assertEquals(e, nativeJavaNativeSandwich(e));
- // if (GWT.isScript()) { // Devmode will not preserve the same String
instance
- // assertSame(e, nativeJavaNativeSandwich(e));
- // }
- //
- // e = makeJavaObject();
- // assertJavaScriptException(e,
catchJava(createThrowNativeRunnable(e)));
- //
- // e = null;
- // assertSame(e, nativeJavaNativeSandwich(e));
+ e = "testing";
+ assertEquals(e, nativeJavaNativeSandwich(e));
+ if (GWT.isScript()) { // Devmode will not preserve the same String
instance
+ assertSame(e, nativeJavaNativeSandwich(e));
+ }
+
+ e = null;
+ assertSame(e, nativeJavaNativeSandwich(e));
+
+ e = makeJavaObject();
+ assertSame(e, nativeJavaNativeSandwich(e));

e = new RuntimeException();
+ assertSame(e, nativeJavaNativeSandwich(e));
+
+ e = new JavaScriptException("exception message"); // Thrown is not set
assertSame(e, nativeJavaNativeSandwich(e));

JavaScriptObject jso = makeJSO();
@@ -283,7 +286,8 @@
} catch (JavaScriptException e) {
assertEquals("null", e.getName());
assertDescription(e, "null");
- assertEquals(null, e.getException());
+ assertTrue(e.isThrownSet());
+ assertEquals(null, e.getThrown());
assertTrue(e.getMessage().contains("null"));
}
}
@@ -296,7 +300,8 @@
} catch (JavaScriptException e) {
assertEquals(o.getClass().getName(), e.getName());
assertDescription(e, "myLameObject");
- assertEquals(null, e.getException());
+ assertTrue(e.isThrownSet());
+ assertEquals(o, e.getThrown());
assertTrue(e.getMessage().contains(o.getClass().getName()));
assertTrue(e.getMessage().contains(e.getDescription()));
}
@@ -309,7 +314,8 @@
} catch (JavaScriptException e) {
assertEquals("String", e.getName());
assertDescription(e, "foobarbaz");
- assertEquals(null, e.getException());
+ assertTrue(e.isThrownSet());
+ assertEquals("foobarbaz", e.getThrown());
assertTrue(e.getMessage().contains(e.getDescription()));
}
}

--
To view, visit https://gwt-review.googlesource.com/3640
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I09c92ac03abbdb930085e2aaf97001b4d495abd9
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Goktug Gokdogan <gok...@google.com>

[Roberto Lublinerman]

Roberto Lublinerman has posted comments on this change.

Change subject: Adds support for unwrapping of thrown non-jso objects from
JavaScriptException.
......................................................................

Patch Set 1: Code-Review+1

I am not familiar with this code, it LGTM but if someone more familiar can
chime in that would be great.

--
To view, visit https://gwt-review.googlesource.com/3640
To unsubscribe, visit https://gwt-review.googlesource.com/settings

Gerrit-MessageType: comment

Gerrit-Change-Id: I09c92ac03abbdb930085e2aaf97001b4d495abd9
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Goktug Gokdogan <gok...@google.com>

Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Roberto Lublinerman <rlu...@google.com>
Gerrit-HasComments: No