1038304 - nsIWebSocketChannel::asyncOpen silently fails when in offline mode

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Mark Banner (:standard8)]

STR:

1) Put Firefox/Gecko into offline mode (Services.io.offline = true)
2) Set up an nsIWebSocketChannel instance
2) Open the channel in async mode.

Expected Results:

=> Error thrown, or listener called with some sort of failure.

Actual Results:

=> Nothing happens. No errors are thrown, no connections logged on the browser console. Trying to call asyncOpen a second time fails with the fact the socket is already open.


I'll attach an example xpcshell test in a moment.

Comment 1

[Mark Banner (:standard8)]

Attachment: xpcshell example websocket offline mode failure

This is an example xpcshell test that shows the failure. I turned off the disallowing of remote network access as the push server was one I knew I could test against easily.

When Services.io.offline = true, the test just hangs - none of the potential failure or success outputs happen.

Comment 2

[Jason Duell]

Thanks for the report.  We should fix this.  My impression is that offline is used very rarely (though I guess standard8 is using it :) so this isn't super-urgent.

Comment 3

[Mark Banner (:standard8)]

(In reply to Jason Duell (:jduell) from comment #2)
> My impression is that offline
> is used very rarely (though I guess standard8 is using it :) so this isn't
> super-urgent.

FYI: I hadn't realised it until just now, but Firefox for Android has network offline detection by default:

http://mxr.mozilla.org/mozilla-central/search?string=network.manage-offline-status

Comment 4

[Jason Duell]

> Firefox for Android has network offline detection by default

Oh, I didn't know that either.  Hopefully Daniel is aware of it.  Daniel, you might also be a good candidate to look into this bug :)

Comment 5

[Patrick McManus [:mcmanus]]

Thanks Mark.

Websocketchannel::onproxyavailable() isn't checking the return code of the async dns lookup.. and when we are offline that async call throws (and never calls back websockets - which is what it is blocked waiting for).

Comment 6

[Patrick McManus [:mcmanus]]

Your results will be at https://tbpl.mozilla.org/?tree=Try&rev=a3c70f0e1a50

Comment 7

[Patrick McManus [:mcmanus]]

update the test a bit - gecko fix is the same

Your results will be at https://tbpl.mozilla.org/?tree=Try&rev=441b67b22a98

Comment 8

[Patrick McManus [:mcmanus]]

Attachment: 0001-bug-1038304-websockets-offline-hang-r-sworkman.patch

Comment 9

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8458068 [details] [diff] [review]
0001-bug-1038304-websockets-offline-hang-r-sworkman.patch

Review of attachment 8458068 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/test/unit/test_websocket_offline.js
@@ +14,5 @@
> +  onAcknowledge: function(aContext, aSize) {},
> +  onBinaryMessageAvailable: function(aContext, aMsg) {},
> +  onMessageAvailable: function(aContext, aMsg) {},
> +  onServerClose: function(aContext, aCode, aReason) {
> +  },

{},

@@ +16,5 @@
> +  onMessageAvailable: function(aContext, aMsg) {},
> +  onServerClose: function(aContext, aCode, aReason) {
> +  },
> +  onStart: function(aContext) {
> +    chan.close(0,"");

From nsIWebSocketChannelListener, it seems like onStart shouldn't be called in error cases. Should this throw here?

@@ +17,5 @@
> +  onServerClose: function(aContext, aCode, aReason) {
> +  },
> +  onStart: function(aContext) {
> +    chan.close(0,"");
> +},

nit: Indentation.

@@ +20,5 @@
> +    chan.close(0,"");
> +},
> +  onStop: function(aContext, aStatusCode) {
> +    Services.io.offline = offlineStatus;
> +    do_test_finished();

We're expecting this to fail right? Should we check the status code?

@@ +39,5 @@
> +    dump("throwing " + x);
> +    do_throw(x);
> +  }
> +}
> +

nit: Remove extra line.

Comment 10

[Patrick McManus [:mcmanus]]

(In reply to Steve Workman [:sworkman] from comment #9)

> 
> From nsIWebSocketChannelListener, it seems like onStart shouldn't be called
> in error cases. Should this throw here?

good point! The case was written so it would pass if online but hang if offline (without the gecko patch).. but we don't need to do that.

> 
> 
> We're expecting this to fail right? Should we check the status code?
> 

yeah - i was double coding for the online case. removed in the next version. The actual failure code is not defined here but !OK ought to be sufficient.

Comment 11

[Patrick McManus [:mcmanus]]

Attachment: websockets offline hang

Comment 12

[Patrick McManus [:mcmanus]]

Attachment: [PATCH] - websockets offline hang

From 6257da38fdee4ec64467765240a9a8bcaf652a45 Mon Sep 17 00:00:00 2001
---
 netwerk/protocol/websocket/WebSocketChannel.cpp | 11 ++++--
 netwerk/test/unit/test_websocket_offline.js     | 45 +++++++++++++++++++++++++
 netwerk/test/unit/xpcshell.ini                  |  1 +
 3 files changed, 54 insertions(+), 3 deletions(-)
 create mode 100644 netwerk/test/unit/test_websocket_offline.js

Comment 13

[Patrick McManus [:mcmanus]]

this morning I did "git checkout master" and found I had uncommited changes on the branch that I exported the last patch from.. sorry :(

Comment 14

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8458661 [details] [diff] [review]
[PATCH]  - websockets offline hang

Review of attachment 8458661 [details] [diff] [review]:
-----------------------------------------------------------------

Cool - thanks for updating the test. r=me.

Comment 15

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0fd5c666d35c

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0fd5c666d35c