Handle unparsable JSON as invalid content (#11733)

Change-Id: I67ed5e78b93ff0fe20d861105a0aa01ae6ccb2e6

9 files changed, 164 insertions, 72 deletions

diff --git a/client/src/com/vaadin/client/ApplicationConnection.java b/client/src/com/vaadin/client/ApplicationConnection.java
index 65afa9b08b..568c728671 100644
--- a/client/src/com/vaadin/client/ApplicationConnection.java
+++ b/client/src/com/vaadin/client/ApplicationConnection.java
@@ -440,7 +440,8 @@ public class ApplicationConnection implements HasHandlers {
 
             // Hack to avoid logging an error in endRequest()
             getServerCommunicationHandler().startRequest();
-            getServerMessageHandler().handleMessage(jsonText);
+            getServerMessageHandler().handleMessage(
+                    ServerMessageHandler.parseJson(jsonText));
         }
 
         // Tooltip can't be created earlier because the
diff --git a/client/src/com/vaadin/client/communication/AtmospherePushConnection.java b/client/src/com/vaadin/client/communication/AtmospherePushConnection.java
index ee2a35fcc2..d36dccdc8b 100644
--- a/client/src/com/vaadin/client/communication/AtmospherePushConnection.java
+++ b/client/src/com/vaadin/client/communication/AtmospherePushConnection.java
@@ -29,6 +29,7 @@ import com.vaadin.client.ApplicationConnection.ApplicationStoppedHandler;
 import com.vaadin.client.ResourceLoader;
 import com.vaadin.client.ResourceLoader.ResourceLoadEvent;
 import com.vaadin.client.ResourceLoader.ResourceLoadListener;
+import com.vaadin.client.ValueMap;
 import com.vaadin.shared.ApplicationConstants;
 import com.vaadin.shared.Version;
 import com.vaadin.shared.communication.PushConstants;
@@ -354,7 +355,7 @@ public class AtmospherePushConnection implements PushConnection {
 
     protected void onMessage(AtmosphereResponse response) {
         String message = response.getResponseBody();
-        String json = ServerCommunicationHandler.stripJSONWrapping(message);
+        ValueMap json = ServerMessageHandler.parseWrappedJson(message);
         if (json == null) {
             // Invalid string (not wrapped as expected)
             getCommunicationProblemHandler().pushInvalidContent(this, message);
@@ -362,7 +363,7 @@ public class AtmospherePushConnection implements PushConnection {
         } else {
             getLogger().info(
                     "Received push (" + getTransportType() + ") message: "
-                            + json);
+                            + message);
             connection.getServerMessageHandler().handleMessage(json);
         }
     }
diff --git a/client/src/com/vaadin/client/communication/ReconnectingCommunicationProblemHandler.java b/client/src/com/vaadin/client/communication/ReconnectingCommunicationProblemHandler.java
index 2c08b0cfd0..36fba675f6 100644
--- a/client/src/com/vaadin/client/communication/ReconnectingCommunicationProblemHandler.java
+++ b/client/src/com/vaadin/client/communication/ReconnectingCommunicationProblemHandler.java
@@ -425,7 +425,19 @@ public class ReconnectingCommunicationProblemHandler implements
 
     @Override
     public void pushInvalidContent(PushConnection pushConnection, String message) {
-        // Do nothing for now. Should likely do the same as xhrInvalidContent
+        debug("pushInvalidContent");
+        if (pushConnection.isBidirectional()) {
+            // We can't be sure that what was pushed was actually a response but
+            // at this point it should not really matter, as something is
+            // seriously broken.
+            endRequest();
+        }
+
+        // Do nothing special for now. Should likely do the same as
+        // xhrInvalidContent
+        handleUnrecoverableCommunicationError("Invalid JSON from server: "
+                + message, null);
+
     }
 
     @Override
@@ -474,10 +486,12 @@ public class ReconnectingCommunicationProblemHandler implements
 
     private void handleUnrecoverableCommunicationError(String details,
             CommunicationProblemEvent event) {
-        Response response = event.getResponse();
         int statusCode = -1;
-        if (response != null) {
-            statusCode = response.getStatusCode();
+        if (event != null) {
+            Response response = event.getResponse();
+            if (response != null) {
+                statusCode = response.getStatusCode();
+            }
         }
         connection.handleCommunicationError(details, statusCode);
 
diff --git a/client/src/com/vaadin/client/communication/ServerCommunicationHandler.java b/client/src/com/vaadin/client/communication/ServerCommunicationHandler.java
index fb7e5bb83b..0e96eafd3d 100644
--- a/client/src/com/vaadin/client/communication/ServerCommunicationHandler.java
+++ b/client/src/com/vaadin/client/communication/ServerCommunicationHandler.java
@@ -48,9 +48,6 @@ import elemental.json.JsonObject;
  */
 public class ServerCommunicationHandler {
 
-    public static final String JSON_COMMUNICATION_PREFIX = "for(;;);[";
-    public static final String JSON_COMMUNICATION_SUFFIX = "]";
-
     private ApplicationConnection connection;
     private boolean hasActiveRequest = false;
 
@@ -102,6 +99,7 @@ public class ServerCommunicationHandler {
      * 
      */
     private void doSendInvocationsToServer() {
+
         ServerRpcQueue serverRpcQueue = getServerRpcQueue();
         if (serverRpcQueue.isEmpty()) {
             return;
@@ -367,28 +365,4 @@ public class ServerCommunicationHandler {
         }
     }
 
-    /**
-     * Strips the JSON wrapping from the given json string with wrapping.
-     * 
-     * If the given string is not wrapped as expected, returns null
-     * 
-     * @since
-     * @param jsonWithWrapping
-     *            the JSON received from the server
-     * @return an unwrapped JSON string or null if the given string was not
-     *         wrapped
-     */
-    public static String stripJSONWrapping(String jsonWithWrapping) {
-        if (!jsonWithWrapping
-                .startsWith(ServerCommunicationHandler.JSON_COMMUNICATION_PREFIX)
-                || !jsonWithWrapping
-                        .endsWith(ServerCommunicationHandler.JSON_COMMUNICATION_SUFFIX)) {
-            return null;
-        }
-        return jsonWithWrapping.substring(
-                ServerCommunicationHandler.JSON_COMMUNICATION_PREFIX.length(),
-                jsonWithWrapping.length()
-                        - ServerCommunicationHandler.JSON_COMMUNICATION_SUFFIX
-                                .length());
-    }
 }
diff --git a/client/src/com/vaadin/client/communication/ServerMessageHandler.java b/client/src/com/vaadin/client/communication/ServerMessageHandler.java
index 1fffcfa4b4..3c986079a1 100644
--- a/client/src/com/vaadin/client/communication/ServerMessageHandler.java
+++ b/client/src/com/vaadin/client/communication/ServerMessageHandler.java
@@ -85,6 +85,9 @@ import elemental.json.JsonObject;
  */
 public class ServerMessageHandler {
 
+    public static final String JSON_COMMUNICATION_PREFIX = "for(;;);[";
+    public static final String JSON_COMMUNICATION_SUFFIX = "]";
+
     /**
      * Helper used to return two values when updating the connector hierarchy.
      */
@@ -222,28 +225,11 @@ public class ServerMessageHandler {
      * @param jsonText
      *            The JSON to handle
      */
-    public void handleMessage(String jsonText) {
-        final Date start = new Date();
-        final ValueMap json;
-        try {
-            json = parseJSONResponse(jsonText);
-        } catch (final Exception e) {
-            // Should not call endRequest for a asynchronous push message
-            // but there is currently no way of knowing if this is an async
-            // message if we get invalid JSON.
-
-            // TODO Move parsing out from this method and handle the error the
-            // same way as if we do not receive the expected prefix and suffix
-            getServerCommunicationHandler().endRequest();
-
-            connection.showCommunicationError(e.getMessage()
-                    + " - Original JSON-text:" + jsonText, 200);
-            return;
+    public void handleMessage(final ValueMap json) {
+        if (json == null) {
+            throw new IllegalArgumentException(
+                    "The json to handle cannot be null");
         }
-        getLogger().info(
-                "JSON parsing took " + (new Date().getTime() - start.getTime())
-                        + "ms");
-
         if (getServerId(json) == -1) {
             getLogger()
                     .severe("Response didn't contain a server id. "
@@ -269,11 +255,6 @@ public class ServerMessageHandler {
         }
     }
 
-    private static native ValueMap parseJSONResponse(String jsonText)
-    /*-{
-       return JSON.parse(jsonText);
-    }-*/;
-
     protected void handleJSON(final ValueMap json) {
         final int serverId = getServerId(json);
 
@@ -1696,4 +1677,70 @@ public class ServerMessageHandler {
         return connection.getServerCommunicationHandler();
     }
 
+    /**
+     * Strips the JSON wrapping from the given json string with wrapping.
+     * 
+     * If the given string is not wrapped as expected, returns null
+     * 
+     * @since
+     * @param jsonWithWrapping
+     *            the JSON received from the server
+     * @return an unwrapped JSON string or null if the given string was not
+     *         wrapped
+     */
+    public static String stripJSONWrapping(String jsonWithWrapping) {
+        if (jsonWithWrapping == null) {
+            return null;
+        }
+
+        if (!jsonWithWrapping.startsWith(JSON_COMMUNICATION_PREFIX)
+                || !jsonWithWrapping.endsWith(JSON_COMMUNICATION_SUFFIX)) {
+            return null;
+        }
+        return jsonWithWrapping.substring(JSON_COMMUNICATION_PREFIX.length(),
+                jsonWithWrapping.length() - JSON_COMMUNICATION_SUFFIX.length());
+    }
+
+    /**
+     * Unwraps and parses the given JSON, originating from the server
+     * 
+     * @param jsonText
+     *            the json from the server
+     * @return A parsed ValueMap or null if the input could not be parsed (or
+     *         was null)
+     */
+    public static ValueMap parseJson(String jsonText) {
+        if (jsonText == null) {
+            return null;
+        }
+        final Date start = new Date();
+        try {
+            ValueMap json = parseJSONResponse(jsonText);
+            getLogger().info(
+                    "JSON parsing took "
+                            + (new Date().getTime() - start.getTime()) + "ms");
+            return json;
+        } catch (final Exception e) {
+            getLogger().severe("Unable to parse JSON: " + jsonText);
+            return null;
+        }
+    }
+
+    private static native ValueMap parseJSONResponse(String jsonText)
+    /*-{
+       return JSON.parse(jsonText);
+    }-*/;
+
+    /**
+     * Parse the given wrapped JSON, received from the server, to a ValueMap
+     * 
+     * @param wrappedJsonText
+     *            the json, wrapped as done by the server
+     * @return a ValueMap, or null if the wrapping was incorrect or json could
+     *         not be parsed
+     */
+    public static ValueMap parseWrappedJson(String wrappedJsonText) {
+        return parseJson(stripJSONWrapping(wrappedJsonText));
+    }
+
 }
diff --git a/client/src/com/vaadin/client/communication/XhrConnection.java b/client/src/com/vaadin/client/communication/XhrConnection.java
index 2ba6ac0873..7930560aae 100644
--- a/client/src/com/vaadin/client/communication/XhrConnection.java
+++ b/client/src/com/vaadin/client/communication/XhrConnection.java
@@ -33,6 +33,7 @@ import com.vaadin.client.ApplicationConnection.RequestStartingEvent;
 import com.vaadin.client.ApplicationConnection.ResponseHandlingEndedEvent;
 import com.vaadin.client.ApplicationConnection.ResponseHandlingStartedEvent;
 import com.vaadin.client.BrowserInfo;
+import com.vaadin.client.ValueMap;
 import com.vaadin.shared.ApplicationConstants;
 import com.vaadin.shared.JsonConstants;
 import com.vaadin.shared.ui.ui.UIConstants;
@@ -165,10 +166,9 @@ public class XhrConnection {
             // for(;;);["+ realJson +"]"
             String responseText = response.getText();
 
-            final String jsonText = ServerCommunicationHandler
-                    .stripJSONWrapping(responseText);
-            if (jsonText == null) {
-                // Invalid string (not wrapped as expected)
+            ValueMap json = ServerMessageHandler.parseWrappedJson(responseText);
+            if (json == null) {
+                // Invalid string (not wrapped as expected or can't parse)
                 getCommunicationProblemHandler().xhrInvalidContent(
                         new CommunicationProblemEvent(request, payload,
                                 response));
@@ -176,8 +176,8 @@ public class XhrConnection {
             }
 
             getCommunicationProblemHandler().xhrOk();
-            getLogger().info("Received xhr message: " + jsonText);
-            getServerMessageHandler().handleMessage(jsonText);
+            getLogger().info("Received xhr message: " + responseText);
+            getServerMessageHandler().handleMessage(json);
         }
 
         /**
diff --git a/client/tests/src/com/vaadin/client/communication/ServerMessageHandlerTest.java b/client/tests/src/com/vaadin/client/communication/ServerMessageHandlerTest.java
new file mode 100644
index 0000000000..4b788d06cc
--- /dev/null
+++ b/client/tests/src/com/vaadin/client/communication/ServerMessageHandlerTest.java
@@ -0,0 +1,56 @@
+/*
+ * Copyright 2000-2014 Vaadin Ltd.
+ * 
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.vaadin.client.communication;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * 
+ * @since
+ * @author Vaadin Ltd
+ */
+public class ServerMessageHandlerTest {
+
+    @Test
+    public void unwrapValidJson() {
+        String payload = "{'foo': 'bar'}";
+        Assert.assertEquals(
+                payload,
+                ServerMessageHandler.stripJSONWrapping("for(;;);[" + payload
+                        + "]"));
+
+    }
+
+    @Test
+    public void unwrapUnwrappedJson() {
+        String payload = "{'foo': 'bar'}";
+        Assert.assertNull(ServerMessageHandler.stripJSONWrapping(payload));
+
+    }
+
+    @Test
+    public void unwrapNull() {
+        Assert.assertNull(ServerMessageHandler.stripJSONWrapping(null));
+
+    }
+
+    @Test
+    public void unwrapEmpty() {
+        Assert.assertNull(ServerMessageHandler.stripJSONWrapping(""));
+
+    }
+}
diff --git a/uitest/src/com/vaadin/tests/application/CommErrorEmulatorUI.java b/uitest/src/com/vaadin/tests/application/CommErrorEmulatorUI.java
index da03e78f1f..080d36fa48 100644
--- a/uitest/src/com/vaadin/tests/application/CommErrorEmulatorUI.java
+++ b/uitest/src/com/vaadin/tests/application/CommErrorEmulatorUI.java
@@ -20,8 +20,6 @@ import com.vaadin.data.Property.ValueChangeEvent;
 import com.vaadin.data.Property.ValueChangeListener;
 import com.vaadin.server.VaadinRequest;
 import com.vaadin.server.VaadinServlet;
-import com.vaadin.shared.communication.PushMode;
-import com.vaadin.shared.ui.ui.Transport;
 import com.vaadin.tests.components.AbstractTestUIWithLog;
 import com.vaadin.ui.Alignment;
 import com.vaadin.ui.Button;
@@ -69,12 +67,10 @@ public class CommErrorEmulatorUI extends AbstractTestUIWithLog {
         String transport = request.getParameter("transport");
 
         if ("websocket".equalsIgnoreCase(transport)) {
-            getPushConfiguration().setPushMode(PushMode.AUTOMATIC);
-            getPushConfiguration().setTransport(Transport.WEBSOCKET);
             log("Using websocket");
+        } else if ("websocket-xhr".equalsIgnoreCase(transport)) {
+            log("Using websocket for push only");
         } else if ("long-polling".equalsIgnoreCase(transport)) {
-            getPushConfiguration().setPushMode(PushMode.AUTOMATIC);
-            getPushConfiguration().setTransport(Transport.LONG_POLLING);
             log("Using long-polling");
         } else {
             log("Using XHR");
diff --git a/uitest/src/com/vaadin/tests/components/AbstractTestUI.java b/uitest/src/com/vaadin/tests/components/AbstractTestUI.java
index 98b0f63ce1..296e68008e 100644
--- a/uitest/src/com/vaadin/tests/components/AbstractTestUI.java
+++ b/uitest/src/com/vaadin/tests/components/AbstractTestUI.java
@@ -124,6 +124,9 @@ public abstract class AbstractTestUI extends UI {
             config.setPushMode(PushMode.DISABLED);
         } else if ("websocket".equals(transport)) {
             enablePush(Transport.WEBSOCKET);
+        } else if ("websocket-xhr".equals(transport)) {
+            enablePush(Transport.WEBSOCKET);
+            getPushConfiguration().setAlwaysUseXhrForServerRequests(true);
         } else if ("streaming".equals(transport)) {
             enablePush(Transport.STREAMING);
         } else if ("long-polling".equals(transport)) {