Change in gwt[master]: Use JSON.parse() instead of eval() to deserialize rpc callba...

[John Ahlroos]

John Ahlroos has uploaded a new change for review.

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


Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Use JSON.parse() instead of eval() to deserialize rpc callback payload

Changes deserialization of the RPC callback payload to use JSON.parse()
instead of eval() to avoid memory leaks in IE9. Handles special
cases (Infinity, -Infinity and NaN) by converting them into valid
JSON values before serialization.

Changes the RPC protocol version to 8 where protocol version <8 will
still be parsed with eval().

Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
---
M
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java
M
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
M
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
3 files changed, 18 insertions(+), 4 deletions(-)



diff --git
a/user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java
b/user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java
index 9a8d093..07fa1be 100644
---
a/user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java
+++
b/user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java
@@ -43,7 +43,7 @@
* The current RPC protocol version. This version differs from the
previous
* one in that it supports {@links RpcToken}s.
*/
- public static final int SERIALIZATION_STREAM_VERSION = 7;
+ public static final int SERIALIZATION_STREAM_VERSION = 8;

/**
* The oldest supported RPC protocol version.
diff --git
a/user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
b/user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
index db74b38..a394d78 100644
---
a/user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
+++
b/user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
@@ -89,7 +89,15 @@
}

public void writeDouble(double fieldValue) {
- append(String.valueOf(fieldValue));
+ if (fieldValue >= Double.POSITIVE_INFINITY) {
+ append("1e1000");
+ } else if (fieldValue <= Double.NEGATIVE_INFINITY) {
+ append("-1e1000");
+ } else if (Double.isNaN(fieldValue)) {
+ append("0");
+ } else {
+ append(String.valueOf(fieldValue));
+ }
}

public void writeFloat(float fieldValue) {
diff --git
a/user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
b/user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
index f85f42e..f70da39 100644
---
a/user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
+++
b/user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
@@ -33,6 +33,10 @@
private static native JavaScriptObject eval(String encoded) /*-{
return eval(encoded);
}-*/;
+
+ private static native JavaScriptObject parse(String encoded) /*-{
+ return JSON.parse(encoded);
+ }-*/;

private static native int getLength(JavaScriptObject array) /*-{
return array.length;
@@ -51,8 +55,10 @@
}

@Override
- public void prepareToRead(String encoded) throws SerializationException {
- results = eval(encoded);
+ public void prepareToRead(String encoded) throws SerializationException {
+ String versionStr = encoded.substring(encoded.lastIndexOf(",")+1,
encoded.lastIndexOf("]"));
+ int version = Integer.parseInt(versionStr);
+ results = version < 8 ? eval(encoded) : parse(encoded);
index = getLength(results);
super.prepareToRead(encoded);


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

[Thomas Broyer]

Thomas Broyer has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 1: Code-Review-2

(2 comments)

AFAICT, this change is kind of useless without some "negotiation" of the
protocol version on the server-side (writing with the same protocol version
as the request used), and deferred-binding on the client-side to use
protocol version 7 on IE6-7 (where JSON.parse is not available; I think we
can safely assume all other browsers have JSON.parse:
http://caniuse.com/json).

This requires a huge refactoring of the server code though.

This change, as is (except entirely removing the eval() codepath), will be
OK in a year from now though, when we'll definitely remove support for
IE6-7 (I though we talked about the end of this year on the SC meeting,
which would rather be GWT 2.6, but IIUC Ray & Daniel's I/O talk they said
GWT 3.0)

....................................................
File
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
Line 97: append("0");
That means that infinity and NaN don't roundtrip, which is a breaking
change.

Couldn't we emit "+Infinity", "-Infinity" and "NaN" as strings and modify
ClientSerializationStreamReader to use Number(x) for parsing (will accept
either one of number or string, and act accordingly, parsing "+Infinity"
and "-Infinity" as positive and negative infinity, and failing to
parse "NaN" as a number thus returning NaN; ServerSerializationStreamReader
already does the right thing as it uses Double.parseDouble).

ClientSerializationStreamReader in DevMode could (should?) also use JSON
parsing then.


....................................................
File
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 37: private static native JavaScriptObject parse(String encoded) /*-{
We have JsonUtils.safeEval() already.

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>
Gerrit-HasComments: Yes

[Thomas Broyer]

Thomas Broyer has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 1:

Brian, I added you as a reviewer as you're the author of the RPC protocol
documentation:
https://docs.google.com/document/d/1eG0YocsYYbNAtivkLtcaiEE5IOF5u4LUol8-LL0TIKU/edit

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: No

[Colin Alworth]

Colin Alworth has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 1:

(1 comment)

The ServerSerializationStreamWriter will also need modifications, two that
I can think of:
* writeLong currently is using " instead of ', which isn't valid in JSON.
This should be failing in the unit tests, as long as testing something that
uses parse instead of eval.
* very large payloads will now break when the array concatenating code
kicks in, writing ].concat([ to join arrays that are otherwise too big for
ie6/7 (and 8?). This isn't valid JSON, so this will break when the object
graph reaches a certain size.

Can you make sure to run the unit tests remotely in each IE version to
verify that you changes will behave? I believe there is a test for each
direction of each primitive, but not a test for very large data sets.

....................................................
File
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
Line 97: append("0");

Infinity does actually work round trip in my quick testing in this way, as
1e1000 or the like is a) legal json and b) outside the range of double, so
will end up as +Infinity on the JS side after parsing, or when wrapped up
in Double.parseDouble. However, 0 being used as NaN won't fly, you're
right. One idea is to emit null, then read out the null as a special case -
in my testing this behaved well in all browsers.

Sending the strings "+Infinity", "NaN" could also work, but then you need
parse those as strings, not as numbers after reading it as json, i think in
my first try at this I decided that was more trouble than it was worth.

No need to use >= or <= for infinity checks, == should be sufficient.

writeFloat will also need similar treatment.

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: Yes

[John Ahlroos]

John Ahlroos has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 1:

(2 comments)

....................................................
File
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
Line 97: append("0");

Yes 1e1000 should work for Infinity but the NaN case won't. Actually I
think changing it to strings is a better idea since it also more readable.


....................................................
File
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 37: private static native JavaScriptObject parse(String encoded) /*-{
Yes, didn't know about that. Should definitely use it instead.

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 1
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>

Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>

[John Ahlroos]

Hello Leeroy Jenkins, Thomas Broyer,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#2).

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Use JSON.parse() instead of eval() to deserialize rpc callback payload

Changes deserialization of the RPC callback payload to use JSON.parse()
instead of eval() to avoid memory leaks in IE9. Handles special

cases (Infinity, -Infinity and NaN) by converting them into strings
before they are sent to the client. ClientSerializationStreamReader.
readDouble() will then use Number(x) to convert the string back to
a number representation.

Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
---
M
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
M
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
2 files changed, 9 insertions(+), 8 deletions(-)

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 2

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>

[John Ahlroos]

John Ahlroos has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 2:

Removed the protocol version for now since it will cause a lot of work to
change.

Instead, changed the commit so the only real change is that NaN and
Infinity is sent as strings instead as a numbers making the JSON valid. On
the client side then JSON.parse can be used to parse the JSON and finally
when reading the double values Number(x) is used to make sure the doubles
(and floats) are numbers. This should be backward compatible with IE6/7 as
well as JSONUtil.safeEval() will fall back to eval() in that case.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 2

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>

Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: No

[Thomas Broyer]

Thomas Broyer has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 2:

(1 comment)

As Colin said, this change is not backwards-compatible when transmitting
large payloads (which will use array .concat()). Could this be handled
dynamically? (look for ").concat(" in the payload and use eval() in this
case –could lead to some false positives but that should be rare enough,
and relatively harmless–, otherwise use JSON.parse if available or eval()
otherwise). As a side note, JsonUtils.safeEval has some overhead in the
non-JSON.parse case compared to the bare eval() done in
ClientSerializationStreamReader, and even JsonUtils.unsafeEval has a small
overhead.

I also wonder if changing the serialization of Infinity and NaN isn't
enough to necessitate a bump of protocol version.

If it were just me, I'd drop IE6/7 support as soon as the next version (and
IE8 by fall next year, or no later than in 2 years from now), then we could
simply remove the .concat() workaround and switch everyone to JSON.parse
(and bumping the protocol version to 8). I'd be happy to discuss that again
on gwt-steering.

....................................................
File
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 83: return new
Number(this.@com.google.gwt.user.client.rpc.impl.ClientSerializationStreamReader::results[--this.@com.google.gwt.user.client.rpc.impl.ClientSerializationStreamReader::index]);
Not 'new Number(x)' (which will return a Number *object*), rather
just 'Number(x)' (which will return a Number *value*)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 2

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: Yes

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 2:

Haven't looked at the code yet, but it seems like we should rev the
protocol version number and modify the code to support both the old and new
way. On the client, the version can be set differently for the IE 6/7
permutation, and hopefully the version not in use can be optimized out. On
the server it should support both.

But before we get into that, what's the motivation for this change? Most
users of GWT-RPC don't care about the details about the wire format. What
improvements do they get by changing it?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 2

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: No

[John Ahlroos]

John Ahlroos has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 2:

Brian:

The main reason I put up this patch is that currently IE9 is leaking memory
quite badly for each RPC call you make. The root cause is that eval() on
IE9 is designed to keep everything evaled in memory until the browsers
session dies. See
https://code.google.com/p/google-web-toolkit/issues/detail?id=57366 for
more details and discussion.

Thanks by the way for the great google doc, hadn't seen it before Thomas
posted it here. Maybe put it up on the new gwt site in some form?

Thomas, Colin:

Many times it takes an equal amount of effort to create some imaginative
hack for IE6/7 than to actually create a new feature and support all the
other browsers. Would gladly see IE6/7 support dropped as soon as possible.
IE8 is still a headache in some cases but not as much as those two.

Will look into the .concat workaround next, and see if can figure something
out.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 2

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

[John Ahlroos]

Hello Leeroy Jenkins, Thomas Broyer,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#3).

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Use JSON.parse() instead of eval() to deserialize rpc callback payload

Changes deserialization of the RPC callback payload to use JSON.parse()
instead of eval() to avoid memory leaks in IE9. Handles special

cases (Infinity, -Infinity and NaN) by converting them into strings
before they are sent to the client. ClientSerializationStreamReader.
readDouble() will then use Number(x) to convert the string back to
a number representation.

Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
---

M user/src/com/google/gwt/core/client/JsonUtils.java
M
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
M
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
3 files changed, 30 insertions(+), 5 deletions(-)

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 3

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

[Matthew Dempsky]

Matthew Dempsky has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 3:

(2 comments)

....................................................
File
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 57: if(!JsonUtils.hasJsonParse() ||
encoded.contains("].concat([")){
GWT style is: "if (condition) {".

Does the .contains() check really do anything for us here? Won't that be
handled by the IllegalArgumentException fallback below anyway? We need to
fallback if there are unstringified NaN or Infinity values too after all,
right?


Line 69: try {
Indented too far here.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 3

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>

Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>
Gerrit-HasComments: Yes

[Thomas Broyer]

Thomas Broyer has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 3: -Code-Review

(1 comment)

That starts to look good.

As discussed previously, I think we should bump the protocol version, and
conditioning the server output (whether to use the .concat hack or not) the
request version should be easy (in
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamWriter.java,
there's already something about long serialization).

We could also just punt to when we remove support for IE6/7. The, we can
either bump the protocol version again (version 9 would be identical to 8
except for the absence of the .concat hack) or change clients to throw an
IncompatibleServiceException (but then stale clients running in IE6/7 will
choke on deserialization of big arrays; not really a problem right?)

....................................................
File user/src/com/google/gwt/core/client/JsonUtils.java
Line 129: public static native boolean hasJsonParse() /*-{
We should rather add an accessor for the static field (e.g.
hasNativeJsonParse)

As an alternative, maybe we could use JSNI to access this field and keep it
private. This is hack, but it's only temporary until we remove support for
IE6/7 (where we could unconditionally use JsonUtils.safeEval(), and that
one would unconditionally use JSON.parse, and that field would disappear).

Given that hasNativeJsonParse would soon unconditionally return 'true', I'd
prefer that we don't introduce such an API in 2.6 that we'll deprecate as
soon as 3.0.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 3

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

[Matthew Dempsky]

Matthew Dempsky has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 3:

(2 comments)

....................................................
File user/src/com/google/gwt/core/client/JsonUtils.java
Line 129: public static native boolean hasJsonParse() /*-{

com.google.gwt.json.client.JSONParser already uses JSNI to bypass the
private, so there's precedent for this hack.

....................................................
File
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 57: if(!JsonUtils.hasJsonParse() ||
encoded.contains("].concat([")){

Hm, actually if we don't care about IE6/IE7 anymore anyway, why are we even
worrying about whether JsonUtils.safeEval() is fast on IE6/IE7? Can't we
just do:

try {
results = JsonUtils.safeEval(encoded);
} catch (IllegalArgumentException iae) {
// Input was invalid JSON; e.g., using ].concat([ or non-stringified
NaN/Infinity.
results = eval(encoded);

}


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 3

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 3:

"The main reason I put up this patch is that currently IE9 is leaking
memory quite badly for each RPC call you make. The root cause is that
eval() on IE9 is designed to keep everything evaled in memory until the
browsers session dies. See
https://code.google.com/p/google-web-toolkit/issues/detail?id=57366 for
more details and discussion."

Thanks, this is a good reason. Please add it (or something like it) to the
patch description.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 3

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: No

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 3:

(3 comments)

....................................................
File user/src/com/google/gwt/core/client/JsonUtils.java
Line 129: public static native boolean hasJsonParse() /*-{

I'm neutral on this, but I'll point out that there's very little downside
to keeping this method around forever.


....................................................
File
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
Line 92: if (Double.isNaN(fieldValue) || Double.isInfinite(fieldValue))
{
Call getVersion() here and do this only for the new version.

....................................................
File
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 57: if(!JsonUtils.hasJsonParse() ||
encoded.contains("].concat([")){

Please no content sniffing. We should check the version number.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 3

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: Yes

[John Ahlroos]

John Ahlroos has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 3:

(2 comments)

....................................................
File user/src/com/google/gwt/core/client/JsonUtils.java
Line 129: public static native boolean hasJsonParse() /*-{

I am too neutral on this, if JSONParser is already using it though JSNI
then the ClientSerializationStreamWriter could do it as well and there
would be no need to open this up.

....................................................
File
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 57: if(!JsonUtils.hasJsonParse() ||
encoded.contains("].concat([")){

Wouldn't passing everything though JSON.parse() yield an unnecessary
performance hit in cases where we know .concat/NaN/Infity has been used and
the JSON.parse() will throw the exception?

The version should be easily enough deducible from the encoded string and
using that we could then simply do:

if(version < 8){
eval()
} else {
try{
JSONUtils.safeEval()
} catch(IllegalArgumentException iae){
throw new IncompatibleRemoteServiceException("Expected valid
JSON, got something else.");

}
}


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 3

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

[John Ahlroos]

Hello Leeroy Jenkins, Thomas Broyer,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#4).

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Use JSON.parse() instead of eval() to deserialize rpc callback payload

IE9 is currently leaking memory on every RPC call you make. The root
cause is that eval() on IE9 is designed to keep everyting evaled in
memory until the browser session dies (see
http://support.microsoft.com/kb/2572253).

To work around this issue this patch instead of using eval() on the
RPC callback payload uses JSON.parse which does not have these problems.
Since JSON
does not support the special values NaN, Infinity and -Infinity they are now
converted into strings and later converted back with Number(x) on the
client side.

This patch also increases the RPC request version number to 8 and allows the
browser to request which version the server implementation should respond
with.
Browsers without support for JSON.parse (IE6/7) will always request version
7.

Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
---
M
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java
M
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
M

user/src/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamWriter.java
M user/src/com/google/gwt/user/server/rpc/RPC.java
M user/src/com/google/gwt/user/server/rpc/RPCRequest.java
M user/src/com/google/gwt/user/server/rpc/RemoteServiceServlet.java
M
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamWriter.java
M
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
8 files changed, 99 insertions(+), 14 deletions(-)

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

[John Ahlroos]

John Ahlroos has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 4:

Patch Set 4 now bumps the default version number and changes the versioning
so that the server now always responds with the same version as the browser
has requested. If JSON.parse() is not available, the client will always
request version 7.

I didn't do anything to the concat part on the server side, if it kicks in
the JSONUtils.safeEval() will fail and the eval() will be used instead. It
could be refactored further to not kick in at all if the version < 8 but I
think that could be done in another changeset.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: No

[Thomas Broyer]

Thomas Broyer has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 4:

(6 comments)

Looking at the history, I stumbled on another case besides the .concat hack
where the protocol does not emit JSON but rather JSON-like JavaScript: the
ClientSerializationStreamReader in DevMode uses Rhino to eval() the
response, and Rhino has a hard limit on the length of a string literal, so
the server emits a concatenation of string literals in this case. See
https://code.google.com/p/google-web-toolkit/source/detail?r=10888

That means we'll want to:

* only emit those concatenated strings in getVersion() < 8
* parse the response without Rhino (and as pure JSON) when it's in version
8.

About detecting version 8, I wonder if we shouldn't change the response
format to include the version outside the JSON. I have no idea what impact
it would have on the code though, so maybe it's a bad idea and extracting
the version from the JSON like you did is the way to go.

But much more importantly: I'd wait for our decision whether we continue to
support IE6/7 before going any farther. If we drop IE6/7 support, we can
simplify the client *and* server code.

Also, currently the server-side code supports back to version 5 of the
GWT-RPC protocol; but that support is actually limited to accepting
requests from old clients, because responses are always sent with the last
known version (according to the changes made in this patch). Is this really
needed? should backwards-compatibility be kept at all? only for the last 2
versions? (7 and 8 here) Note that clients will choke just after parsing
the response because getVersion() doesn't exactly match what they know how
to deal with, so supporting older requests on the server-side means that
the RPC is executed but the client can't decode the response. I don't know
what's best here (vs. failing early), but we're not really concerned here,
as we're only changing the response format, so we can continue to support
old requests; I don't think it's worth supporting clients older than v7
though (cached/running clients while the server is being updated/redeployed)

FYI, ServerSerializationStreamWriter has some code that depends on
getVersion() in writeLong, which is why I though the changes would be
limited to that class. Digging further, that code was added in
https://code.google.com/p/google-web-toolkit/source/detail?r=8274 and
simply copied from AbstractSerializationStreamWriter (shared between client
and server), so I think it was an oversight.

FYI, backwards-compat on the server-side for decoding requests was added at
https://code.google.com/p/google-web-toolkit/source/detail?r=8269

....................................................
File
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
Line 92: if (Double.isNaN(fieldValue) || Double.isInfinite(fieldValue))
{

if (getVersion() > 7 && (Double.isNaN(fieldValue) ||
Double.isInfinite(fieldValue))

(except with a the '7' replaced by a constant in
AbstractSerializationStream)


....................................................
File
user/src/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamWriter.java
Line 191: int version = isJsonParseSupported() ? getVersion() : 7;
Use a constant instead of '7' (see comment on
AbstractSerializationStreamWriter too)


....................................................
File
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamWriter.java
Depending on the version, this class should decide whether to use
LengthConstrainedArray (the .concat hack) or not (or tweak its behavior to
enable/disable the .concat hack)

That way, we can decide on the client-side whether to use eval() or
JSON.parse and be sure that in the JSON.parse case (version 8) we should
have valid JSON as input.


Line 636: if (getVersion() < 8) {
Should be done in AbstractSerializationStreamWriter (see comment there)


....................................................
File
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 61: if(version < 8){
Formatting (spaces):

if•(version < 8)•{

and use a constant instead of a "magic number".


Line 62: // Versions prior to 8 uses invalid JSON; e.g., using
].concat([ or non-stringified NaN/Infinity.
Versions prior to 8 use JavaScript, not JSON.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: Yes

[Matthew Dempsky]

Matthew Dempsky has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 4:

(1 comment)

....................................................
File
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamWriter.java

Line 636: if (getVersion() < 8) {

Also, I think the comparison is backwards? Shouldn't we be writing the
stringified value on getVersion() >= 8?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 4:

(4 comments)

To add to the versioning mess, there are Google projects that are using
GWT-RPC to serialize data without using the "RPC" part (via hacks). For
example, GWT-RPC encoded data might be stored client-side or embedded into
the HTML page served up at initial page load. I think they're just calling
the static methods and not worrying about versioning since we haven't
changed the protocol in years. So this could cause unexpected breakage for
them. On the other hand if they're using a serialization policy they're
probably used to it :-)

It seems like we should do this gradually and start by supporting 7 and 8
for decoding, and make the decision in the caller to use version 8 when we
know it's safe?

We will need to announce the change for people using trunk and include it
in release notes for the next stable version.

....................................................
File
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java
Line 44: * one in that it supports {@links RpcToken}s.
update comment

....................................................
File
user/src/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamWriter.java
Line 191: int version = isJsonParseSupported() ? getVersion() : 7;

Maybe extract a "chooseClientVersion()" method?


....................................................
File user/src/com/google/gwt/user/server/rpc/RPC.java
Line 381: AbstractSerializationStream.SERIALIZATION_STREAM_VERSION);
This is tricky. Seems like there are four choices:

(1) If we bump the version to 8 here then all clients need to immediately
support version 8 for decoding responses. (That includes IE6/7 since we
haven't dropped them quite yet.)

(2) Leave it leave it at 7, so we don't need to worry about getting version
8 to work in old browsers. But that means new browsers need to handle both
version 7 and version 8 responses.

(3) I was hoping that each permutation would only need to support one
version. To do that, responses should always use the version sent in the
request, as you've started on, but we would also need to choose the correct
version for sending failures. In that case I'd deprecate this method and
force the callers to make the decision. (But we're still faced with
choosing a version for the deprecated method.)

(4) Wait to commit this change until we've actually dropped IE6/7.


Line 455: AbstractSerializationStream.SERIALIZATION_STREAM_VERSION);
Similar version issues apply here, except that you've already fixed it for
the success case. I think we should deprecate this method. We should choose
the same version here as in encodeResponseForFailure (use the same
constant).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

[Thomas Broyer]

Thomas Broyer has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 4:

(1 comment)

We need to take a decision re. supporting version 7 in parallel to version
8 and how to do it (for those using the RPC class without doing remote
calls).

When we'll drop support for IE6/7 (and thus need to continue supporting
version 7 of the protocol) is being discussed by the steering committee, so
we should IMO stop working on this patch pending that decision; unless
Google wants to get this fixed sooner in master with a transition period
(but then you could also do that in your branch).

If there's a transition period where both protocol versions have to be
supported in parallel, then we need to decide how best to do it. I'd like
to propose moving this discussion to gwt-contrib (generally speaking, we
should try to only discuss in Gerrit about the code itself, and move other
discussions to gwt-contrib, or ask the SC to make a decision, depending on
the kind of thing that needs to be decided)

....................................................
File user/src/com/google/gwt/user/server/rpc/RPC.java
Line 381: AbstractSerializationStream.SERIALIZATION_STREAM_VERSION);

Which I'd summarize as / reduce to:

* we bump to 8 but then we wait until we actually dropped IE6/7 (which
we're currently deciding)
* we support both 7 and 8, with IE6/7 continuing using version 7 and all
other browsers use version 8; the server-side responds with the same
version that was used in the request. I agree that using permutations
rather than a dynamic switch would make the code clearer/cleaner. That
doesn't solve the problem re. those encodeResponseForFailure/Success
methods though; and we'd introduce them only for a short transition period:
they wouldn't be needed as soon as we remove support for IE6/7 (and thus
the need to support version 7 of the protocol in parallel to version 8). If
the transition period ends before the next GWT release, then I'm OK to make
that change, otherwise I wouldn't like making such a change.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 4:

Okay, I started a discussion in contrib:
https://groups.google.com/forum/?fromgroups#!topic/google-web-toolkit-contributors/NWyaRsfy0-Q

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: No

[Artur Signell]

Artur Signell has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 4:

Would it be a feasible solution to introduce protocol version 8 today
(which is valid JSON) but keep the default version at 7 until we drop
support for IE6/IE7? This would disconnect this issue from the IE6/IE7
discussion.

By making it possible to change the server to use version 8 for
communicating with the client (through e.g. a system property) any user can
today fix the memory leak issue in IE9 (provided they do not need IE6/IE7
support) and all other users will get the fix automatically once we drop
old IE support and bump the default version.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

Gerrit-Reviewer: Artur Signell <ar...@vaadin.com>

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 4:

Feasible? I'd guess yes. Up to the implementer to decide how they want to
proceed.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 4

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>

[John Ahlroos]

John Ahlroos has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................


Patch Set 4:

Discussed this offline with Artur for some time last Friday and I like the
idea of setting the RPC version behind a property which application
developers can set. It allows developers who knows they don't need to care
about IE6/7, but needs to get this IE9 leak fixed asap, to just bump the
version and start using JSON.parse() directly. It also allows us to wait
with the decision of dropping IE6/7 completely until 2.6/3.0 without
breaking any existing applications.

It also does mean developers which currently are supporting IE6-10 need to
make a decision, either drop IE6/7 support and bump the version or live
with the IE9 leak in eval(). Since we are anyway dropping support at some
point it would be a good hint for those developers to start transitioning
from IE6 to the modern world.

The way I thought of implementing this was to introduce a new constant
SERIALIZATION_STREAM_MAX_VERSION (8) which would denote the maximum version
supported by the server and SERIALIZATION_STREAM_VERSION would remain as 7
for now. We could also bump the SERIALIZATION_STREAM_MIN_VERSION to 7 if we
so decide. I am still a bit confused over why the min version is 5 when the
server still always is responding with the latest version.

The client would always use SERIALIZATION_STREAM_VERSION in the RPC request
and the server would by default respond with SERIALIZATION_STREAM_VERSION
(I would remove the version juggling currently implemented). If a custom
version is set by the application developer via a property on the server
then the server would respond with that instead. With version 8 the server
would emit valid JSON while previous versions would emit what currently is
emitted, hacks and all.

Once the client gets the response it would figure out which version is in
use (by reading the payload as currently implemented, maybe moving the
version out of the payload could be done in another changeset) and decode
it with JSON.parse or eval(), whatever is appropriate for that version.

By doing it this way, this changeset does not grow to be a huge change and
current behaviour is preserved for now at least. Once we get the decision
to drop support for IE6/7 we just set
SERIALIZATION_STREAM_VERSION=SERIALIZATION_STREAM_MAX_VERSION and
everything should still work ok.

I don't have a good idea where I would put the property though. Should it
be a System property as Artur suggests or should it instead be a setter in
the RemoteServiceServlet? Ideas?

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................


Patch Set 4:

On the server side, I don't think we need a max version? If the client
sends version 8 it should understand it and respond with version 8. Perhaps
we could add an HTTP header to make it more likely that we send back the
right version even if the message itself can't be parsed?

We do need a default version because of those unfortunate static methods in
the API. Perhaps that can be configurable. See
RemoteServiceServlet.getCodeServerPort for one possible way. I'm a bit wary
of adding a mutable global variable, though, versus fixing the callers.

On the client side, a configuration property adds flexibility and we can
set it for each permutation. It might be simpler just to set the version to
use, rather than a max version. Perhaps the IE6/7 permutation should stop
the compile with an error if it's set to 8?

[John Ahlroos]

Hello Leeroy Jenkins, Thomas Broyer,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#5).

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Use JSON.parse() instead of eval() to deserialize rpc callback payload

IE9 is currently leaking memory on every RPC call you make. The root
cause is that eval() on IE9 is designed to keep everyting evaled in
memory until the browser session dies (see
http://support.microsoft.com/kb/2572253).

To work around this issue this patch instead of using eval() on the
RPC callback payload uses JSON.parse which does not have these problems.
Since JSON
does not support the special values NaN, Infinity and -Infinity they are now
converted into strings and later converted back with Number(x) on the
client side.

The default RPC version still remains as 7 and this patch introduces a
new system variable "gwt.rpc.version" which can be set to 8 to resolve
the memory leak. Special cases where hacks have been applied to
circumvent browser limitations (string literal size limitation, array
size limitation) will still be applied in version 8 and will still be
evaled insted of JSON.parsed on the client side.

Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
---
M
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java
M
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
M

user/src/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
M user/src/com/google/gwt/user/server/rpc/RPC.java
M
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamReader.java
M
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamWriter.java
M
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
7 files changed, 56 insertions(+), 24 deletions(-)

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 5

[John Ahlroos]

John Ahlroos has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 5:

Okay, so patch set 5 takes a much simpler approach and does not change the
current behaviour in any way. It only adds a system
property "gwt.rpc.version" which can be set to change the behaviour to use
JSON.parse. This I think, is a good intermediate solution until we bump the
version permanently to 8 and allows customers to work around the memory
leak.

I did not change how devmode evaluates the response (still using Rhino) and
I think it deserves another changeset entirely since it is not related to
fixing this memory leak in any way.

The special cases (array concatenation, string concatenation) are still
sent as Javascript even in version 8. In that case the client will try to
parse with JSON.parse and fallback to eval. The memory leak will still
persist there but I think those corner cases are so rare that they can at
least for now be omitted.

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 5
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Artur Signell <ar...@vaadin.com>
Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: No

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................


Patch Set 5:

(3 comments)

I like where this is going. As I understand it, all clients will support
parsing 7 and 8 messages. When the server creates a message it can be 7 or
8 depending on the system property, defaulting to 7. Client-created
messages are always version 7.

But it seems confusing that version 8 messages are sometimes JSON and
sometimes not. It seems like if we decide to apply a hack we should set the
version back to 7, so that version 7 messages are always non-JSON and
version 8 messages are always JSON. That way we don't need a fallback when
parsing; the version should always tell us what to do.

....................................................
File
user/src/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 328
I think instead of deleting this check, we need to check that it's a
version that this client knows how to decode.


....................................................
File
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 60: int version = Integer.parseInt(versionStr);
It's unclear how this works. Maybe extract a readVersion(String encoded)
method and write a few tests with some example strings?


Line 70: results = eval(encoded);
I'm not sure we need a fallback? The server should have set the version
correctly.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 5
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Artur Signell <ar...@vaadin.com>
Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: Yes

[John Ahlroos]

John Ahlroos has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................


Patch Set 5:

(3 comments)

....................................................
File
user/src/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 328

The check should probably be something like:

(SERIALIZATION_STREAM_MIN_VERSION <= getVersion() && getVersion() <=
SERIALIZATION_STREAM_MAX_VERSION)

I'll add it back.

....................................................
File
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 60: int version = Integer.parseInt(versionStr);

The version number is always added last in the JSON array so it looks like
this "[.... ,7]". I just find the last ',' and last ']' and assume the
value between is the version number.

Sure I can extract it into a method and write some tests for it.

Line 70: results = eval(encoded);

The fallback is needed when the concat etc. hacks are applied and
JSON.parse will fail since the message is not valid JSON.

But if we change the server side implementation so that if the hacks are
applied the version automatically becomes 7 then this indeed would not be
needed.

[John Ahlroos]

Hello Leeroy Jenkins, Thomas Broyer,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#6).

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Use JSON.parse() instead of eval() to deserialize rpc callback payload

IE9 is currently leaking memory on every RPC call you make. The root
cause is that eval() on IE9 is designed to keep everyting evaled in
memory until the browser session dies (see
http://support.microsoft.com/kb/2572253).

To work around this issue this patch instead of using eval() on the
RPC callback payload uses JSON.parse which does not have these problems.
Since JSON
does not support the special values NaN, Infinity and -Infinity they are now
converted into strings and later converted back with Number(x) on the
client side.

The default RPC version still remains as 7 and this patch introduces a
new system variable "gwt.rpc.version" which can be set to 8 to resolve
the memory leak. Special cases where hacks have been applied to
circumvent browser limitations (string literal size limitation, array
size limitation) will still be applied in version 8 and will still be

evaled instead of JSON.parsed on the client side.

Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
---
M
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java
M
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStreamWriter.java
M
user/src/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
M user/src/com/google/gwt/user/server/rpc/RPC.java
M
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamReader.java
M
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamWriter.java
M
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java

M user/test/com/google/gwt/user/RPCSuite.java
A
user/test/com/google/gwt/user/client/rpc/impl/WebModeClientSerializationStreamReaderTest.java
9 files changed, 149 insertions(+), 26 deletions(-)

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 6

[John Ahlroos]

Hello Leeroy Jenkins, Thomas Broyer,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#7).

9 files changed, 163 insertions(+), 26 deletions(-)

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f

Gerrit-PatchSet: 7

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 7:

(4 comments)

Looking good.

....................................................
File
user/src/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 328: if (getVersion() < SERIALIZATION_STREAM_MIN_VERSION
Before, this code would throw for any version !=7 and with this patch, we
are accepting down to version 5. That doesn't seem necessary since Version
7 has been the standard for a long time. So maybe we should increase
MIN_VERSION to 7?


....................................................
File user/src/com/google/gwt/user/server/rpc/RPC.java
Line 602: :
AbstractSerializationStream.SERIALIZATION_STREAM_VERSION;
Probably should check that it's between min version and max version and
throw a SerializationException if it isn't.

Also, getInteger() can throw.


....................................................
File
user/src/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamWriter.java
Line 93: javascript |=
Pattern.compile("(\"[\\w]*\")(\\+)+").matcher(token).find();
I'm not sure how much we should worry about server-side performance here,
but here are some ideas:

- Pattern.compile() should be moved to a constant.
- We should have another method that skips scanning the string and call
that where possible. For example, addToken(int) should skip the scan. There
is also a path from writeLong that should probably skip it.
Ideally we should move the string concatenation generating code so we don't
need to scan to figure out what we already did.


....................................................
File
user/super/com/google/gwt/user/translatable/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 59
It seems like we should keep the check for a version between min and max?

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 7
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Artur Signell <ar...@vaadin.com>
Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: Yes

[John Ahlroos]

Hello Leeroy Jenkins, Thomas Broyer,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#8).

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Use JSON.parse() instead of eval() to deserialize rpc callback payload

9 files changed, 189 insertions(+), 30 deletions(-)

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 8

[John Ahlroos]

John Ahlroos has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 7:

(2 comments)

....................................................
File
user/src/com/google/gwt/user/client/rpc/impl/ClientSerializationStreamReader.java
Line 328: if (getVersion() < SERIALIZATION_STREAM_MIN_VERSION

There a a couple of places where the min version is explicitly used
(ServerSerializationStreamWriter.writeLong() for instance). If we bump the
min version then this code would then be obsolete and should either be
removed or taken into consideration in version 7. I would rather do that in
another changeset so those cases can be discussed separatly.

....................................................
File user/src/com/google/gwt/user/server/rpc/RPC.java
Line 602: :
AbstractSerializationStream.SERIALIZATION_STREAM_VERSION;

Yes, that would be a good idea.

AFAIK Integer.getInteger() does not throw but returns null. At least
according to the oracle docs
http://docs.oracle.com/javase/6/docs/api/java/lang/Integer.html#getInteger%28java.lang.String%29

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 7

Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Artur Signell <ar...@vaadin.com>
Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: Yes

[John Ahlroos]

Hello Leeroy Jenkins, Thomas Broyer,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#9).

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Use JSON.parse() instead of eval() to deserialize rpc callback payload

9 files changed, 187 insertions(+), 28 deletions(-)

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 9

[John Ahlroos]

Hello Leeroy Jenkins, Thomas Broyer,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#10).

Gerrit-PatchSet: 10

[John Ahlroos]

John Ahlroos has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 10:

Leeroy seems to be in a bad mood today :)

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 10
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Artur Signell <ar...@vaadin.com>
Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>
Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: No

[John Ahlroos]

Hello Leeroy Jenkins, Thomas Broyer,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#11).

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Use JSON.parse() instead of eval() to deserialize rpc callback payload

M
user/test/com/google/gwt/user/server/rpc/impl/ServerSerializationStreamWriterTest.java
10 files changed, 217 insertions(+), 27 deletions(-)

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 11

[Leif Åstrand]

Leif Åstrand has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................

Patch Set 11: Code-Review-1

This patch does not work as it stands for a couple of reasons:

Special numbers (e.g. NaN) values are sent as strings, but they are in the
string table which means that the receiver just gets an index to the table
without knowing whether that is the actual value or just a string table
index.

The sent value is not valid JSON in a couple of situations. This does at
least include escaping unicode values using \x (JSON only supports \u) and
sending long values as base64 strings enclosed using ' (JSON only
supports ")

A readVersion method has been added to the super sourced version of
ClientSerializationStreamReader but it is missing from the original version.

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I6062180397f5fabed1dd5f08140c2bd43a19fa9f
Gerrit-PatchSet: 11
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Artur Signell <ar...@vaadin.com>
Gerrit-Reviewer: Brian Slesinsky <skyb...@google.com>
Gerrit-Reviewer: Colin Alworth <nilo...@gmail.com>
Gerrit-Reviewer: John Ahlroos <jo...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jen...@gwtproject.org>

Gerrit-Reviewer: Leif Åstrand <le...@vaadin.com>

Gerrit-Reviewer: Matthew Dempsky <mdem...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.br...@gmail.com>

Gerrit-HasComments: No

[Brian Slesinsky]

Brian Slesinsky has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................


Patch Set 11:

Sounds like we need more tests. Serialization code generally does benefit
from a thorough test suite.

[Leif Åstrand]

Leif Åstrand has posted comments on this change.

Change subject: Use JSON.parse() instead of eval() to deserialize rpc
callback payload
......................................................................


Patch Set 11:

The existing test suite seems to have quite good coverage, but it does not
help when it only tests the default protocol version, i.e. 7. All the
problems I mentioned were discovered by changing the default protocol
version to 8 and running the RPCSuite.

Vaadin has fixes for those problems in
https://github.com/vaadin/gwt/commit/9c8f102265c032efc575ed3c070334bf57645aa3
and
https://github.com/vaadin/gwt/commit/d3914682163347a9eacee8d3b23e55e62b2a97ea.
John A is currently on vacation and I won't have time to submit a new
version here either because I'm leaving today.