Wicket
  1. Wicket
  2. WICKET-5409

wicket-native-websocket does not work with Safari/Safari iOS

    Details

      Description

      I noticed issue when tried this sample app: https://github.com/martin-g/blogs/tree/master/wicket6-websocket-broadcast

      Browsers I tried:
      Firefox v25.0
      Chrome v30.0.1599.101
      Safari v6.1

      Broadcast example seems to be not working with Safari:
      Firefox - works (broadcast messages displayed),
      Chrome - works,
      Safari - no broadcast messages displayed, console shows error: WebSocket connection to 'ws://localhost:8080/feed?0&pageId=0&wicket-ajax-baseurl=feed?0' failed: Error during WebSocket handshake: 'Connection' header value is not 'Upgrade'

      After bunch of different tests, decided to create basic wicket quickstart app - one home page with WebSocketBehaviour added, got same results:
      Firefox - works (no errors in console, websocket connection estabished),
      Chrome - works,
      Safari - websocked connection fails, console shows error: WebSocket connection to 'ws://localhost:8080/?0&pageId=0&wicket-ajax-baseurl=?0' failed: Error during WebSocket handshake: 'Connection' header value is not 'Upgrade'

      Another example (jetty chat app on websockets, unrelated to wicket) that I tested with minor fixes (upgraded to 7.6.9 jetty): http://webtide.intalio.com/wp-content/uploads/2011/08/example-websocket.zip
      Chat example works for all three browsers: Firefox, Chrome and Safari.

      Seems that wicket-native-websocket integration has issues with Safari.

      1. double_upgrade_header.pcapng
        7 kB
        Lorentz Green
      2. Screen Shot 2013-11-07 at 11.41.06 PM.png
        56 kB
        Lorentz Green
      3. wicket-quickstart-websocket.zip
        24 kB
        Lorentz Green

        Activity

        Lorentz Green created issue -
        Hide
        Lorentz Green added a comment -

        Basic quickstart app + websocket behaviour on home page.

        Show
        Lorentz Green added a comment - Basic quickstart app + websocket behaviour on home page.
        Lorentz Green made changes -
        Field Original Value New Value
        Attachment wicket-quickstart-websocket.zip [ 12612710 ]
        Hide
        Lorentz Green added a comment - - edited

        Safari console screenshot with websocket connection errors.

        Show
        Lorentz Green added a comment - - edited Safari console screenshot with websocket connection errors.
        Lorentz Green made changes -
        Hide
        Martin Grigorov added a comment -

        I won't be able to debug this problem.
        AFAIS there is no Safari 6.x for Windows and I don't have Mac or an image for VirtualBox.
        I can test it only with Safari 5.1.x on Windows. Last time I checked it worked.

        Show
        Martin Grigorov added a comment - I won't be able to debug this problem. AFAIS there is no Safari 6.x for Windows and I don't have Mac or an image for VirtualBox. I can test it only with Safari 5.1.x on Windows. Last time I checked it worked.
        Hide
        Lorentz Green added a comment -

        Any suggestions what could be causing this issue? I'm not really into wicket-native-websocket integration implementation, so some introduction would help.

        Show
        Lorentz Green added a comment - Any suggestions what could be causing this issue? I'm not really into wicket-native-websocket integration implementation, so some introduction would help.
        Hide
        Martin Grigorov added a comment -

        This is the docu we have: https://cwiki.apache.org/confluence/display/WICKET/Wicket+Native+WebSockets. It doesn't explain much of the implementation details though.
        To debug the problem you will have to put a breakpoint in the specialization of WicketFilter for the web container that you use.
        https://github.com/apache/wicket/blob/master/wicket-experimental/wicket-native-websocket/wicket-native-websocket-jetty/src/main/java/org/apache/wicket/protocol/ws/jetty/Jetty7WebSocketFilter.java?source=cc#L74

        I guess the browser doesn't send some info for the handshake (request upgrade).
        Try also with newer versions of Jetty - 7.6.9 is a bit old.

        https://github.com/apache/wicket/blob/master/wicket-experimental/wicket-native-websocket/wicket-native-websocket-javax/src/test/java/org/apache/wicket/protocol/ws/javax/Start.java (master branch!) starts Jetty 9.1.x at port 8080 and you can test it without any additional setup.

        Show
        Martin Grigorov added a comment - This is the docu we have: https://cwiki.apache.org/confluence/display/WICKET/Wicket+Native+WebSockets . It doesn't explain much of the implementation details though. To debug the problem you will have to put a breakpoint in the specialization of WicketFilter for the web container that you use. https://github.com/apache/wicket/blob/master/wicket-experimental/wicket-native-websocket/wicket-native-websocket-jetty/src/main/java/org/apache/wicket/protocol/ws/jetty/Jetty7WebSocketFilter.java?source=cc#L74 I guess the browser doesn't send some info for the handshake (request upgrade). Try also with newer versions of Jetty - 7.6.9 is a bit old. https://github.com/apache/wicket/blob/master/wicket-experimental/wicket-native-websocket/wicket-native-websocket-javax/src/test/java/org/apache/wicket/protocol/ws/javax/Start.java (master branch!) starts Jetty 9.1.x at port 8080 and you can test it without any additional setup.
        Hide
        Lorentz Green added a comment - - edited

        I did attach wireshark capture file for req/resp of quickstart websock app with 0.13 native integration lib on jetty 7.6.9 with Safari 6.1.

        What I see that after upgrade request, wicket websock app response had Connection: Upgrade, however - twice. After debugging I see that AbstractUpgradeFilter (line 124 and 125) does:

        124: resp.setHeader("upgrade", "websocket");
        125: resp.setHeader("connection", "upgrade");

        later on Jetty 7.6.9 , WebSocketServletConnectionRFC6455 (lines 44 and 45) does almost the same:

        44: response.setHeader("Upgrade","WebSocket");
        45: response.addHeader("Connection","Upgrade");

        So response now has x2 Connection: Upgrade values. Commenting out lines 124 and 125 in AbstractUpgradeFilter and testing showed that Safari 6.1 does not shows any errors and sample apps work as expected (quickstart websock and websock broadcast).

        So seems that x2 Connection: Upgrade values break Safari 6.1. Currently I assume that fix should be to remove 124 and 125 lines for AbstractUpgradeFilter as this is handled by WebSocketServletConnectionRFC6455 and I would assume that other connection types in Jetty should behave similarly and do setup of headers correctly (however assumption needs to be confirmed).

        Show
        Lorentz Green added a comment - - edited I did attach wireshark capture file for req/resp of quickstart websock app with 0.13 native integration lib on jetty 7.6.9 with Safari 6.1. What I see that after upgrade request, wicket websock app response had Connection: Upgrade, however - twice. After debugging I see that AbstractUpgradeFilter (line 124 and 125) does: 124: resp.setHeader("upgrade", "websocket"); 125: resp.setHeader("connection", "upgrade"); later on Jetty 7.6.9 , WebSocketServletConnectionRFC6455 (lines 44 and 45) does almost the same: 44: response.setHeader("Upgrade","WebSocket"); 45: response.addHeader("Connection","Upgrade"); So response now has x2 Connection: Upgrade values. Commenting out lines 124 and 125 in AbstractUpgradeFilter and testing showed that Safari 6.1 does not shows any errors and sample apps work as expected (quickstart websock and websock broadcast). So seems that x2 Connection: Upgrade values break Safari 6.1. Currently I assume that fix should be to remove 124 and 125 lines for AbstractUpgradeFilter as this is handled by WebSocketServletConnectionRFC6455 and I would assume that other connection types in Jetty should behave similarly and do setup of headers correctly (however assumption needs to be confirmed).
        Lorentz Green made changes -
        Attachment double_upgrade_header.pcapng [ 12612988 ]
        Hide
        Martin Grigorov added a comment -

        Thanks for the investigation!
        I'll take a look soon.

        Can you test with a newer Jetty in the meantime, e.g. 7.6.14 or even better 8.1.14 ?

        Show
        Martin Grigorov added a comment - Thanks for the investigation! I'll take a look soon. Can you test with a newer Jetty in the meantime, e.g. 7.6.14 or even better 8.1.14 ?
        Hide
        Lorentz Green added a comment - - edited

        Skipped jetty-7.6.14 and 8.x, retested previous scenario with : jetty-9.0.6.v20130930, wicket 6.12.0, wicket-native-websocket 0.14 with patched Jetty9UpgradeHttpRequest - replaced usage of org.eclipse.jetty.websocket.server.ServletWebSocketRequest (deprecated) with org.eclipse.jetty.websocket.servlet.ServletUpgradeRequest - otherwise above setup crashes on opening of the page:

        Caused by: java.lang.IllegalStateException: org.eclipse.jetty.websocket.server.ServletWebSocketRequest has no 'req' field!
        at org.apache.wicket.protocol.ws.jetty9.Jetty9UpgradeHttpRequest.<clinit>(Jetty9UpgradeHttpRequest.java:41)
        ... 25 more
        Caused by: java.lang.NoSuchFieldException: req
        at java.lang.Class.getDeclaredField(Class.java:1938)
        at org.apache.wicket.protocol.ws.jetty9.Jetty9UpgradeHttpRequest.<clinit>(Jetty9UpgradeHttpRequest.java:38)
        ... 25 more

        Please, fix this deprecation issue too

        Results are the same as described previously AbstractUpgradeFilter puts dublicate "Connection: Upgrade" entry into header which causes Safari 6.1 to show error "failed: Error during WebSocket handshake: 'Connection' header value is not 'Upgrade' " and disconnect websocket. Removing of line 124 and 125, from abstract upgrade filter resulted in upgrade response with valid header (no duplicate entries) and correct behavior of Safari.

        At the moment do not have jetty 9.1.x setup, did not test it. Also I would rather not use 9.1.x for production - is not officially stable yet.

        Show
        Lorentz Green added a comment - - edited Skipped jetty-7.6.14 and 8.x, retested previous scenario with : jetty-9.0.6.v20130930, wicket 6.12.0, wicket-native-websocket 0.14 with patched Jetty9UpgradeHttpRequest - replaced usage of org.eclipse.jetty.websocket.server.ServletWebSocketRequest (deprecated) with org.eclipse.jetty.websocket.servlet.ServletUpgradeRequest - otherwise above setup crashes on opening of the page: Caused by: java.lang.IllegalStateException: org.eclipse.jetty.websocket.server.ServletWebSocketRequest has no 'req' field! at org.apache.wicket.protocol.ws.jetty9.Jetty9UpgradeHttpRequest.<clinit>(Jetty9UpgradeHttpRequest.java:41) ... 25 more Caused by: java.lang.NoSuchFieldException: req at java.lang.Class.getDeclaredField(Class.java:1938) at org.apache.wicket.protocol.ws.jetty9.Jetty9UpgradeHttpRequest.<clinit>(Jetty9UpgradeHttpRequest.java:38) ... 25 more Please, fix this deprecation issue too Results are the same as described previously AbstractUpgradeFilter puts dublicate "Connection: Upgrade" entry into header which causes Safari 6.1 to show error "failed: Error during WebSocket handshake: 'Connection' header value is not 'Upgrade' " and disconnect websocket. Removing of line 124 and 125, from abstract upgrade filter resulted in upgrade response with valid header (no duplicate entries) and correct behavior of Safari. At the moment do not have jetty 9.1.x setup, did not test it. Also I would rather not use 9.1.x for production - is not officially stable yet.
        Hide
        Martin Grigorov added a comment -

        I am not sure what you have tested and what is actually released with wicket-native-websocket-jetty9:0.14 but I have fixed the deprecation 4 months ago:
        https://github.com/apache/wicket/commit/3b787e14ab1d64fcef83952a706675ad8d697095

        Show
        Martin Grigorov added a comment - I am not sure what you have tested and what is actually released with wicket-native-websocket-jetty9:0.14 but I have fixed the deprecation 4 months ago: https://github.com/apache/wicket/commit/3b787e14ab1d64fcef83952a706675ad8d697095
        Hide
        Martin Grigorov added a comment -

        OK. The problem is located. The fix is not downported to wicket-6.x ...

        Show
        Martin Grigorov added a comment - OK. The problem is located. The fix is not downported to wicket-6.x ...
        Hide
        Martin Grigorov added a comment -

        Fixed.
        The headers are set explicitly only for Tomcat7WebSocketFilter.
        Some improvements have been downported from master branch.

        Please retest with latest 6.13.0-SNAPSHOT if you can. You can use http://wicket.apache.org/start/quickstart.html to generate a quickstart.
        Thanks!

        Show
        Martin Grigorov added a comment - Fixed. The headers are set explicitly only for Tomcat7WebSocketFilter. Some improvements have been downported from master branch. Please retest with latest 6.13.0-SNAPSHOT if you can. You can use http://wicket.apache.org/start/quickstart.html to generate a quickstart. Thanks!
        Martin Grigorov made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 6.13.0 [ 12325564 ]
        Fix Version/s 7.0.0 [ 12322958 ]
        Resolution Fixed [ 1 ]
        Hide
        Lorentz Green added a comment -

        Martin,

        Retested quickstart scenario with : jetty-9.0.6.v20130930, wicket 6.13.0-SNAPSHOT, wicket-native-websocket 0.15-SNAPSHOT - Firefox, Chrome and Safari work as expected, WebSocket connection is established and messaged can be passed. Also wicket-native-websocket 0.15-SNAPSHOT, has no deprecated Jetty class usage issues.

        Thank you for fixing this issue.

        Regards,
        Lorentz

        Show
        Lorentz Green added a comment - Martin, Retested quickstart scenario with : jetty-9.0.6.v20130930, wicket 6.13.0-SNAPSHOT, wicket-native-websocket 0.15-SNAPSHOT - Firefox, Chrome and Safari work as expected, WebSocket connection is established and messaged can be passed. Also wicket-native-websocket 0.15-SNAPSHOT, has no deprecated Jetty class usage issues. Thank you for fixing this issue. Regards, Lorentz
        Hide
        Lorentz Green added a comment -

        works in expected setup and provided scenario

        Show
        Lorentz Green added a comment - works in expected setup and provided scenario
        Lorentz Green made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        5d 12h 58m 1 Martin Grigorov 13/Nov/13 10:36
        Resolved Resolved Closed Closed
        19d 22h 16m 1 Lorentz Green 03/Dec/13 08:53

          People

          • Assignee:
            Martin Grigorov
            Reporter:
            Lorentz Green
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development