nsIWebSocketChannel::asyncOpen silently fails when in offline mode

RESOLVED FIXED in mozilla33

Status

()

Core
Networking: WebSockets
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: mcmanus)

Tracking

({testcase})

Trunk
mozilla33
All
Mac OS X
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Created attachment 8455485 [details] [diff] [review]
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.
(Reporter)

Updated

4 years ago
Keywords: testcase
Version: 33 Branch → Trunk
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.
(Reporter)

Comment 3

4 years ago
(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
> 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 :)
Flags: needinfo?(daniel)
(Assignee)

Comment 5

4 years ago
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).
Assignee: nobody → mcmanus
(Assignee)

Comment 7

4 years ago
update the test a bit - gecko fix is the same

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

Comment 8

4 years ago
Created attachment 8458068 [details] [diff] [review]
0001-bug-1038304-websockets-offline-hang-r-sworkman.patch
Attachment #8458068 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8458068 - Flags: review+ → review?(sworkman)
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.
(Assignee)

Comment 10

4 years ago
(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.
(Assignee)

Comment 11

4 years ago
Created attachment 8458378 [details] [diff] [review]
websockets offline hang
Attachment #8458378 - Flags: review?(sworkman)
(Assignee)

Updated

4 years ago
Attachment #8458068 - Attachment is obsolete: true
Attachment #8458068 - Flags: review?(sworkman)
(Assignee)

Comment 12

4 years ago
Created attachment 8458661 [details] [diff] [review]
[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
Attachment #8458661 - Flags: review?(sworkman)
(Assignee)

Updated

4 years ago
Attachment #8458378 - Attachment is obsolete: true
Attachment #8458378 - Flags: review?(sworkman)
(Assignee)

Comment 13

4 years ago
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 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.
Attachment #8458661 - Flags: review?(sworkman) → review+
https://hg.mozilla.org/mozilla-central/rev/0fd5c666d35c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Flags: needinfo?(daniel)
You need to log in before you can comment on or make changes to this bug.