Isn't there a deeper problem here?  Regardless of success or failure, webRequest is incorrectly reporting the connection lifetime.

Shouldn't the request remain active until the WebSocket connection closes?  That would be consistent with the behavior of (e.g.) a hanging GET.

FTR, the details of the issue is also explained in this comment of the code review:
https://codereview.chromium.org/2449913002/#msg21

+yhirano@. Discussing off the issue with Pavel.

Comment #3: Thanks for pointing it out. Yeah, it's better if we could keep everything close to long poll XHR.

Note that extensions are currently tied to the lifetime of a URLRequest.  The fact that WebSockets kinda use WebSockets has some weird implications for things that expect their lifecycle to mirror that of a URLRequest.

We may be able to prolong the URLRequest lifetime in net/websocket, but we cannot dispatch OK to the associated NetworkDelegate without modifying URLRequest.

URLRequest expects some semantics which websocket doesn't respect, so I don't think it's a good idea to insert websocket-related logic to URLRequest. Some ideas I could think of:

 1: Add a net_error code (ERR_WEBSOCKET_CLOSURE) and cancel the URLRequest with the error code.
 2: Add a method (OnWebSocketClosed) in NetworkDelegate. OnCompleted may be called after that, but an implementation should ignore the OnCompleted call in such a case.
 3: Call NetworkDeletate::OnCompleted from net/websocket. OnCompleted may be called from URLRequest either, but an implementation should accept only the first one.

I don't like 3.

Thoughts?

How about a mixture of 1 and 2? I would call the new net error code ERR_WEBSOCKET_HANDSHAKE_SUCCESS. This would allow implementations of NetworkDelegate to detect when the handshake completed.

Then the new method OnWebSocketClosed(closeCode, message) would permit implementations to see the full WebSocket lifetime.

NetworkDelegate implementations uninterested in WebSockets would get the same behaviour as now (although if they special-case ERR_ABORTED then they would notice the difference). Those that are not interested in the full lifetime could just special-case ERR_WEBSOCKET_HANDSHAKE_SUCCESS.

I'm not sure what the signature of OnWebSocketClosed should be. I think it would be good to guarantee it is always called for all kinds of success or failure, but closeCode is not meaningful for failures that happen during the handshake.

Probably we shouldn't implement OnWebSocketClosed until there is clear demand for it.

Hi everyone,

Some time ago I consulted some relevant people (cbentzel@, mkwst@), and they had thought that at least for WebRequest API showing the full WebSocket lifetime was not critical. Additionally, Adam has just mentioned that there is no clear demand in observing the WebSocketClosed event.

What *is* somewhat critical is proceeding with the long-standing WebSocket+WebRequestAPI CL (https://codereview.chromium.org/2449913002/) which is blocked by this bug.

Taking above arguments into consideration, I would propose to go on with the most simple solution for now. Yutaka, do you think it would be solution 1 from your comment #6?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/96205c442a254bfedf737c97f836c7bfd1973d29

commit 96205c442a254bfedf737c97f836c7bfd1973d29
Author: yhirano <yhirano@chromium.org>
Date: Wed Jan 18 22:45:18 2017

Cancel URLRequest with an appropriate code when WebSocket handshake succeeds

To notify the NetworkDelegate of WebSocket handshake success explicitly, this
CL introduces a new net error and cancels the URLRequest with it.

BUG= 663672 

Review-Url: https://codereview.chromium.org/2628333003
Cr-Commit-Position: refs/heads/master@{#444523}

[modify] https://crrev.com/96205c442a254bfedf737c97f836c7bfd1973d29/net/base/net_error_list.h
[modify] https://crrev.com/96205c442a254bfedf737c97f836c7bfd1973d29/net/websockets/websocket_stream.cc
[modify] https://crrev.com/96205c442a254bfedf737c97f836c7bfd1973d29/net/websockets/websocket_stream_test.cc
[modify] https://crrev.com/96205c442a254bfedf737c97f836c7bfd1973d29/net/websockets/websocket_test_util.h

I just realised we forgot about the need to add the new code to the enum in histograms.xml. It just requires running tools/metrics/histograms/update_net_error_codes.py.

However, I noticed that all the other WebSocket-specific error codes start with WS_, not WEBSOCKET_. I also thought of a better name. Could we change WEBSOCKET_HANDSHAKE_SUCCESS to WS_UPGRADE ?

yhirano, if you don't have time for this let me know and I will handle it.

Hi Adam,

Thanks for pointing this out. I have created a CL (https://codereview.chromium.org/2639933005) and added you both as reviewers, to speed up the process a bit, cause you are in different time zones.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2ff3fe13ff8f01226d36f63e9e695bbaf747dbdb

commit 2ff3fe13ff8f01226d36f63e9e695bbaf747dbdb
Author: pkalinnikov <pkalinnikov@chromium.org>
Date: Thu Jan 19 17:28:51 2017

Rename WS handshake success error code; update enums.

The new error code needs to be added to histograms.xml, as well as some other
values that apparently have been forgotten to be added.

The reason for renaming the error code is that WebSocket error codes start with
a WS_ prefix.

BUG= 663672 

Review-Url: https://codereview.chromium.org/2639933005
Cr-Commit-Position: refs/heads/master@{#444771}

[modify] https://crrev.com/2ff3fe13ff8f01226d36f63e9e695bbaf747dbdb/net/base/net_error_list.h
[modify] https://crrev.com/2ff3fe13ff8f01226d36f63e9e695bbaf747dbdb/net/websockets/websocket_stream.cc
[modify] https://crrev.com/2ff3fe13ff8f01226d36f63e9e695bbaf747dbdb/net/websockets/websocket_stream_test.cc
[modify] https://crrev.com/2ff3fe13ff8f01226d36f63e9e695bbaf747dbdb/tools/metrics/histograms/histograms.xml

pkalinnikov@, do you still want to observe the WebSocketClosed event? Or can I close this bug?

Thanks!

Hello,

I think that observing WebSocketClosed is not in a short-term plan, so the bug can be closed. Moreover, its initial message was fully addressed.

Thank you!