New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Android] Fix IllegalStateException crash in WebSocketModule.java #6301

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@kevinstumpf
Copy link
Contributor

kevinstumpf commented Mar 4, 2016

  • Motivation: The WebSocket implementation on Android crashes the app when an attempt is made to write on a web socket that was closed due to a spotty connection. We found this issue by using Pusher, which is built on WebSockets. The following stack trace reveals that the WebSocketModule doesn't catch the case of a closed connection, when a consumer attempts to write:
Fatal Exception: java.lang.IllegalStateException: closed
       at com.squareup.okhttp.internal.ws.RealWebSocket.sendMessage(RealWebSocket.java:109)
       at com.facebook.react.modules.websocket.WebSocketModule.send(WebSocketModule.java:176)
       at java.lang.reflect.Method.invoke(Method.java)
       at java.lang.reflect.Method.invoke(Method.java:372)
       at com.facebook.react.bridge.BaseJavaModule$JavaMethod.invoke(BaseJavaModule.java:249)
       at com.facebook.react.bridge.NativeModuleRegistry$ModuleDefinition.call(NativeModuleRegistry.java:158)
       at com.facebook.react.bridge.NativeModuleRegistry.call(NativeModuleRegistry.java:58)
       at com.facebook.react.bridge.CatalystInstanceImpl$NativeModulesReactCallback.call(CatalystInstanceImpl.java:428)
       at com.facebook.react.bridge.queue.NativeRunnableDeprecated.run(NativeRunnableDeprecated.java)
       at android.os.Handler.handleCallback(Handler.java:739)
       at android.os.Handler.dispatchMessage(Handler.java:95)
       at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:31)
       at android.os.Looper.loop(Looper.java:145)
       at com.facebook.react.bridge.queue.MessageQueueThreadImpl$3.run(MessageQueueThreadImpl.java:184)
       at java.lang.Thread.run(Thread.java:818)
  • Test Plan: Create a WebSocket connection and attempt to write after the connection became unavailable due to a spotty connection. An easy way to do this is to establish a Pusher connection with pusher-websocket-iso/react-native and fake or force a spotty connection.
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Mar 4, 2016

By analyzing the blame information on this pull request, we identified @satya164 and @aprock to be potential reviewers.

@@ -178,6 +179,8 @@ public void send(String message, int id) {
new Buffer().writeUtf8(message));
} catch (IOException e) {
notifyWebSocketFailed(id, e.getMessage());
} catch (IllegalStateException e) {
notifyWebSocketFailed(id, e.getMessage());

This comment has been minimized.

@satya164

satya164 Mar 4, 2016

Collaborator

Please combine these two catch blocks like this, (IOException | IllegalStateException e)

@kevinstumpf kevinstumpf force-pushed the kevinstumpf:webSocketModuleCrashFix branch from 8757870 to 1c21177 Mar 4, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Mar 4, 2016

@kevinstumpf updated the pull request.

@kevinstumpf

This comment has been minimized.

Copy link
Contributor

kevinstumpf commented Mar 4, 2016

@satya164 thanks for the quick review. Made the change.

@satya164

This comment has been minimized.

Copy link
Collaborator

satya164 commented Mar 4, 2016

Thank you. Will merge as soon as the tests pass :)

@satya164

This comment has been minimized.

Copy link
Collaborator

satya164 commented Mar 5, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Mar 5, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in a6a89fe Mar 5, 2016

pglotov added a commit to pglotov/react-native that referenced this pull request Mar 15, 2016

Fix IllegalStateException crash in WebSocketModule.java
Summary:- Motivation: The WebSocket implementation on Android crashes the app when an attempt is made to write on a web socket that was closed due to a spotty connection. We found this issue by using Pusher, which is built on WebSockets. The following stack trace reveals that the WebSocketModule doesn't catch the case of a closed connection, when a consumer attempts to write:
```sh
Fatal Exception: java.lang.IllegalStateException: closed
       at com.squareup.okhttp.internal.ws.RealWebSocket.sendMessage(RealWebSocket.java:109)
       at com.facebook.react.modules.websocket.WebSocketModule.send(WebSocketModule.java:176)
       at java.lang.reflect.Method.invoke(Method.java)
       at java.lang.reflect.Method.invoke(Method.java:372)
       at com.facebook.react.bridge.BaseJavaModule$JavaMethod.invoke(BaseJavaModule.java:249)
       at com.facebook.react.bridge.NativeModuleRegistry$ModuleDefinition.call(NativeModuleRegistry.java:158)
       at com.facebook.react.bridge.NativeModuleRegistry.call(NativeModuleReg
Closes facebook#6301

Differential Revision: D3016099

fb-gh-sync-id: 838dd9d2e5e5b7a4e2242fa6de5658dfdaf24f55
shipit-source-id: 838dd9d2e5e5b7a4e2242fa6de5658dfdaf24f55

kipricker pushed a commit to PlexChat/react-native that referenced this pull request Mar 30, 2016

Kevin Stumpf Kip Ricker
Fix IllegalStateException crash in WebSocketModule.java
Summary:- Motivation: The WebSocket implementation on Android crashes the app when an attempt is made to write on a web socket that was closed due to a spotty connection. We found this issue by using Pusher, which is built on WebSockets. The following stack trace reveals that the WebSocketModule doesn't catch the case of a closed connection, when a consumer attempts to write:
```sh
Fatal Exception: java.lang.IllegalStateException: closed
       at com.squareup.okhttp.internal.ws.RealWebSocket.sendMessage(RealWebSocket.java:109)
       at com.facebook.react.modules.websocket.WebSocketModule.send(WebSocketModule.java:176)
       at java.lang.reflect.Method.invoke(Method.java)
       at java.lang.reflect.Method.invoke(Method.java:372)
       at com.facebook.react.bridge.BaseJavaModule$JavaMethod.invoke(BaseJavaModule.java:249)
       at com.facebook.react.bridge.NativeModuleRegistry$ModuleDefinition.call(NativeModuleRegistry.java:158)
       at com.facebook.react.bridge.NativeModuleRegistry.call(NativeModuleReg
Closes facebook#6301

Differential Revision: D3016099

fb-gh-sync-id: 838dd9d2e5e5b7a4e2242fa6de5658dfdaf24f55
shipit-source-id: 838dd9d2e5e5b7a4e2242fa6de5658dfdaf24f55

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment