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 <j...@vaadin.com>
Gerrit-Reviewer: Brian Slesinsky <skybr...@google.com>
Gerrit-Reviewer: Colin Alworth <niloc...@gmail.com>
Gerrit-Reviewer: John Ahlroos <j...@vaadin.com>
Gerrit-Reviewer: Leeroy Jenkins <jenk...@gwtproject.org>
Gerrit-Reviewer: Matthew Dempsky <mdemp...@google.com>
Gerrit-Reviewer: Thomas Broyer <t.bro...@gmail.com>
Gerrit-HasComments: Yes

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
--- You received this message because you are subscribed to the Google Groups "GWT Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.


Reply via email to