Support WebSocket in WebRequest API.

Support WebSocket in WebRequest API.

This CL makes WebRequest API support intercepting the WebSocket handshake
request. Since the handshake is done by means of an HTTP upgrade request, its
flow fits into HTTP-oriented WebRequest model. Additional restriction applies,
that WS request redirects triggered by extensions are ignored.

Note that WebRequest API does *not intercept*:
 - Individual messages sent over an established WebSocket connection.
 - WebSocket closing connection.

The |details| object reported to extensions uses the newly introduced type of
request: 'websocket'.

BUG=129353
Review-Url: https://codereview.chromium.org/2449913002
Cr-Commit-Position: refs/heads/master@{#451991}
Committed: https://chromium.googlesource.com/chromium/src/+/0f198df6bc8da6fe51b34f11b50936700b6e9353

[pkalinnikov, 2016-10-25 13:55:46]

Description was changed from

==========
Support WebSocket in WebRequest API.

BUG=129353
==========

to

==========
Support WebSocket in WebRequest API.

BUG=129353
==========

[pkalinnikov, 2016-10-25 13:55:46]

pkalinnikov@chromium.org changed reviewers:
+ battre@chromium.org

[pkalinnikov, 2016-10-25 13:59:44]

Dominic, can you please take a first look?

I tested this by running an extension that uses WebRequest API, and checked that
the events are intercepted. Also some ad-hoc debug output showed that the full
chain OnBeforeUrlRequest->OnBeforeSendHeader->...->OnCompleted has been called
in ChromeExtensionsNetworkDelegate.

[pkalinnikov, 2016-10-25 14:04:47]

pkalinnikov@chromium.org changed reviewers:
+ tyoshino@chromium.org

[pkalinnikov, 2016-10-25 14:04:48]

Takeshi, can you please take a look and suggest if you see any issues with this?

[pmarks, 2016-10-26 03:15:01]

Is this supposed to be more than a 2-line change?

I tried this with http://demo.kaazing.com/forex/ and
https://github.com/pmarks-net/ipvfoo,
but I don't see any "wss:" URLs in onBeforeRequest/onResponseStarted.

[tyoshino (SeeGerritForStatus), 2016-10-26 10:34:19]

Thanks for working on this. Could you please try to write some tests for it
(unit tests and ideally some browser tests)?

[pkalinnikov, 2016-10-26 11:21:58]

On 2016/10/26 03:15:01, pmarks wrote:
> Is this supposed to be more than a 2-line change?
> 
> I tried this with http://demo.kaazing.com/forex/ and
> https://github.com/pmarks-net/ipvfoo,
> but I don't see any "wss:" URLs in onBeforeRequest/onResponseStarted.

Hi Paul,

I tested this patch with AdBlock Plus, and blocking WebSocket requests seemed
working. But in fact ABP has a workaround for WebSockets, so the request
apparently had been blocked due to this workaround, and not because the
onBeforeRequest was intercepted. 

Indeed, the request is visible to the ChromeExtensionsNetworkDelegate and
ExtensionWebRequestEventRouter, but from there it doesn't get routed to the
extensions because it doesn't match schemes expected by them: { "http://*/*",
"https://*/*" }. As far as I understand, the "<all_urls>" pattern, stated in the
manifest file, doesn't include WS scheme by default. I will look into how to fix
this.

And yes, Takeshi, we obviously need tests. I will add them too.

[pmarks, 2016-10-26 18:38:26]

While running http://demo.kaazing.com/forex/ with the Network debugger open, I
notice that the connection to wss://demo.kaazing.com/jms?.kl=Y lacks a "Remote
Address" field.

This suggests that additional plumbing may be required to populate details.ip in
onResponseStarted.

[pkalinnikov, 2016-10-28 11:34:53]

pkalinnikov@chromium.org changed reviewers:
+ mpcomplete@chromium.org
- battre@chromium.org

[pkalinnikov, 2016-10-28 11:39:22]

On 2016/10/26 18:38:26, pmarks wrote:
> While running http://demo.kaazing.com/forex/ with the Network debugger open, I
> notice that the connection to wss://demo.kaazing.com/jms?.kl=Y lacks a "Remote
> Address" field.
> 
> This suggests that additional plumbing may be required to populate details.ip
in
> onResponseStarted.

Hi Paul,

Thanks for keeping eye on this CL. I fixed your previous concern, now you can
see "wss:" requests in onBeforeRequest. Can you make sure that it works as you
expect now?

Meanwhile, I will look into the "Remote Address" issue. Do you think it should
be addressed in this CL or in a separate one?

[pkalinnikov, 2016-10-28 11:43:22]

mpcomplete@: Hi Matt, can you please take a look? I modified some default
permissions for WebRequest API. I am not sure if some more similar constants
should be modified as well. I am also not sure about whether changing the
constant in URLPattern::SetScheme is needed.

[pkalinnikov, 2016-10-28 11:56:14]

pkalinnikov@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[pkalinnikov, 2016-10-28 11:56:15]

rdevlin.cronin@: Hi Devlin. Your opinion is needed on modifying permissions.
Thank you.

[Devlin, 2016-10-28 16:17:02]

+nasko, creis as FYI.

In principle, I think this is fine - I see no reason that extensions shouldn't
be able to operate on web socket requests when they intercept everything else.

I'd like creis and/or nasko to weigh in before we carry on too far, but that
discussion can happen on the bug.

[pmarks, 2016-10-29 03:14:06]

On 2016/10/28 11:39:22, pkalinnikov wrote:
> On 2016/10/26 18:38:26, pmarks wrote:
> > While running http://demo.kaazing.com/forex/ with the Network debugger open,
I
> > notice that the connection to wss://demo.kaazing.com/jms?.kl=Y lacks a
"Remote
> > Address" field.
> > 
> > This suggests that additional plumbing may be required to populate
details.ip
> in
> > onResponseStarted.
> 
> Hi Paul,
> 
> Thanks for keeping eye on this CL. I fixed your previous concern, now you can
> see "wss:" requests in onBeforeRequest. Can you make sure that it works as you
> expect now?

Sorry, I missed your message because I hadn't CC'd myself.  I've applied your
new patch, but Chrome takes a while to build, so I'll report back after the
weekend.

> 
> Meanwhile, I will look into the "Remote Address" issue. Do you think it should
> be addressed in this CL or in a separate one?

I have no opinion on the organization of Chrome CLs, but I would like to see
details.ip working properly by the time this feature hits real users.  It's
available for all other network-related webRequests, including ftp:// and cached
HTTP.

[pmarks, 2016-10-30 00:06:57]

On 2016/10/29 03:14:06, pmarks wrote:
> Sorry, I missed your message because I hadn't CC'd myself.  I've applied your
> new patch, but Chrome takes a while to build, so I'll report back after the
> weekend.
> 

Here is a captured onResponseStarted event:

{
  "frameId": -1,
  "fromCache": false,
  "ip": "50.16.199.247",
  "method": "GET",
  "parentFrameId": -1,
  "requestId": "273",
  "statusCode": 101,
  "statusLine": "HTTP/1.1 101 Web Socket Protocol Handshake",
  "tabId": -1,
  "timeStamp": 1477784906834.35,
  "type": "other",
  "url": "wss://demo.kaazing.com/jms?.kl=Y"
}

The IP address looks fine, actually.

But tabId is missing, so you can't tell which page generated the request.
And perhaps the "other" type could be more specific?

[tyoshino (SeeGerritForStatus), 2016-11-08 11:05:51]

tyoshino@chromium.org changed reviewers:
+ mmenke@chromium.org

[tyoshino (SeeGerritForStatus), 2016-11-08 11:14:49]

+mmenke to get input about URLRequest usage.

Hi Matt,

Currently, the WebSocket stack is designed to steal a connection during
processing WebSocket handshake response and just throw away the URLRequest owned
by WebSocketStreamRequestImpl. As a result, the URLRequest Cancel()s during
destruction and is reported to the NetworkDelegate (for the WebRequest API, it's
ChromeExtensionsNetworkDelegate) as abort. I'd like to notify the delegate of
successful WebSocket handshake completion correctly.

One possible solution is that we change the URLRequest to have a method to
accept a signal that this kind of "Upgrade" happened. What do you think? Should
this be addressed outside the URLRequest API?

[mmenke, 2016-11-08 15:25:44]

On 2016/11/08 11:14:49, tyoshino wrote:
> +mmenke to get input about URLRequest usage.
> 
> Hi Matt,
> 
> Currently, the WebSocket stack is designed to steal a connection during
> processing WebSocket handshake response and just throw away the URLRequest
owned
> by WebSocketStreamRequestImpl. As a result, the URLRequest Cancel()s during
> destruction and is reported to the NetworkDelegate (for the WebRequest API,
it's
> ChromeExtensionsNetworkDelegate) as abort. I'd like to notify the delegate of
> successful WebSocket handshake completion correctly.
> 
> One possible solution is that we change the URLRequest to have a method to
> accept a signal that this kind of "Upgrade" happened. What do you think?
Should
> this be addressed outside the URLRequest API?

How is the connection stolen?  I glanced at the code, but wouldn't find the code
for this.

[tyoshino (SeeGerritForStatus), 2016-11-09 01:29:15]

On 2016/11/08 15:25:44, mmenke wrote:
> On 2016/11/08 11:14:49, tyoshino wrote:
> > +mmenke to get input about URLRequest usage.
> > 
> > Hi Matt,
> > 
> > Currently, the WebSocket stack is designed to steal a connection during
> > processing WebSocket handshake response and just throw away the URLRequest
> owned
> > by WebSocketStreamRequestImpl. As a result, the URLRequest Cancel()s during
> > destruction and is reported to the NetworkDelegate (for the WebRequest API,
> it's
> > ChromeExtensionsNetworkDelegate) as abort. I'd like to notify the delegate
of
> > successful WebSocket handshake completion correctly.
> > 
> > One possible solution is that we change the URLRequest to have a method to
> > accept a signal that this kind of "Upgrade" happened. What do you think?
> Should
> > this be addressed outside the URLRequest API?
> 
> How is the connection stolen?  I glanced at the code, but wouldn't find the
code
> for this.

Oops, I forgot to point the very code that detaches the connection. It's
WebSocketBasicHandshakeStream::Upgrade() which calls
HttpBasicState::ReleaseConnection().

After calling WebSocketBasicHandshakeStream::Upgrade(),
WebSocketStreamRequestImpl::PerformUpgrade() calls WebSocketChannel::OnSuccess()
which implements WebSocketStream::ConnectDelegate::OnSuccess(). It calls
WebSocketChannel::OnConnectSuccess() which destroys the
WebSocketStreamRequestImpl instance it holds (in the stream_request_ member
variable), and continues WebSocket message exchange which is performed by
calling ->socket()->Read() on the detached connection held by
WebSocketBasicStream.

[mmenke, 2016-11-09 15:45:27]

On 2016/11/09 01:29:15, tyoshino wrote:
> On 2016/11/08 15:25:44, mmenke wrote:
> > On 2016/11/08 11:14:49, tyoshino wrote:
> > > +mmenke to get input about URLRequest usage.
> > > 
> > > Hi Matt,
> > > 
> > > Currently, the WebSocket stack is designed to steal a connection during
> > > processing WebSocket handshake response and just throw away the URLRequest
> > owned
> > > by WebSocketStreamRequestImpl. As a result, the URLRequest Cancel()s
during
> > > destruction and is reported to the NetworkDelegate (for the WebRequest
API,
> > it's
> > > ChromeExtensionsNetworkDelegate) as abort. I'd like to notify the delegate
> of
> > > successful WebSocket handshake completion correctly.
> > > 
> > > One possible solution is that we change the URLRequest to have a method to
> > > accept a signal that this kind of "Upgrade" happened. What do you think?
> > Should
> > > this be addressed outside the URLRequest API?
> > 
> > How is the connection stolen?  I glanced at the code, but wouldn't find the
> code
> > for this.
> 
> Oops, I forgot to point the very code that detaches the connection. It's
> WebSocketBasicHandshakeStream::Upgrade() which calls
> HttpBasicState::ReleaseConnection().
> 
> After calling WebSocketBasicHandshakeStream::Upgrade(),
> WebSocketStreamRequestImpl::PerformUpgrade() calls
WebSocketChannel::OnSuccess()
> which implements WebSocketStream::ConnectDelegate::OnSuccess(). It calls
> WebSocketChannel::OnConnectSuccess() which destroys the
> WebSocketStreamRequestImpl instance it holds (in the stream_request_ member
> variable), and continues WebSocket message exchange which is performed by
> calling ->socket()->Read() on the detached connection held by
> WebSocketBasicStream.

I don't understand how things are wired up nearly well enough to suggest a path
forward here, but I agree that reporting an ERR_ABORTED to NetworkDelegate is
weird and unexpected, except in the case where we actually did abort the attempt
to establish a WebSocket connection.  It's weird, but for simplicity, does seem
like we should use the same NetworkDelegate methods for earlier events (Sending
the request, getting headers, etc)

[mmenke, 2016-11-09 15:49:16]

On 2016/11/09 15:45:27, mmenke wrote:
> On 2016/11/09 01:29:15, tyoshino wrote:
> > On 2016/11/08 15:25:44, mmenke wrote:
> > > On 2016/11/08 11:14:49, tyoshino wrote:
> > > > +mmenke to get input about URLRequest usage.
> > > > 
> > > > Hi Matt,
> > > > 
> > > > Currently, the WebSocket stack is designed to steal a connection during
> > > > processing WebSocket handshake response and just throw away the
URLRequest
> > > owned
> > > > by WebSocketStreamRequestImpl. As a result, the URLRequest Cancel()s
> during
> > > > destruction and is reported to the NetworkDelegate (for the WebRequest
> API,
> > > it's
> > > > ChromeExtensionsNetworkDelegate) as abort. I'd like to notify the
delegate
> > of
> > > > successful WebSocket handshake completion correctly.
> > > > 
> > > > One possible solution is that we change the URLRequest to have a method
to
> > > > accept a signal that this kind of "Upgrade" happened. What do you think?
> > > Should
> > > > this be addressed outside the URLRequest API?
> > > 
> > > How is the connection stolen?  I glanced at the code, but wouldn't find
the
> > code
> > > for this.
> > 
> > Oops, I forgot to point the very code that detaches the connection. It's
> > WebSocketBasicHandshakeStream::Upgrade() which calls
> > HttpBasicState::ReleaseConnection().
> > 
> > After calling WebSocketBasicHandshakeStream::Upgrade(),
> > WebSocketStreamRequestImpl::PerformUpgrade() calls
> WebSocketChannel::OnSuccess()
> > which implements WebSocketStream::ConnectDelegate::OnSuccess(). It calls
> > WebSocketChannel::OnConnectSuccess() which destroys the
> > WebSocketStreamRequestImpl instance it holds (in the stream_request_ member
> > variable), and continues WebSocket message exchange which is performed by
> > calling ->socket()->Read() on the detached connection held by
> > WebSocketBasicStream.
> 
> I don't understand how things are wired up nearly well enough to suggest a
path
> forward here, but I agree that reporting an ERR_ABORTED to NetworkDelegate is
> weird and unexpected, except in the case where we actually did abort the
attempt
> to establish a WebSocket connection.  It's weird, but for simplicity, does
seem
> like we should use the same NetworkDelegate methods for earlier events
(Sending
> the request, getting headers, etc)

I'm reluctant to add yet more stuff to URLRequest - the fact is the interface
has way too many contracts it doesn't respect already, but I just don't
understand how this works well enough to suggest an alternative.

[pkalinnikov, 2016-11-16 18:18:15]

Hi everyone,

Can you take initial looks at this CL?

rdevlin.cronin@, mpcomplete@: Please review the whole CL.

mmenke@, tyoshino@: Please see the
"extensions/browser/api/web_request/web_request_event_details.cc", where
render_process_id/tab_id are obtained for WS requests.

tyoshino@: Feel free to skim through the rest of CL.

pmarks@: Will you be able to make another round of testing with your extension
to make sure the API works as you expect? Note, that the "other" type is
unlikely to be changed to something more specific in this CL.

[mmenke, 2016-11-16 18:20:39]

https://codereview.chromium.org/2449913002/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_event_details.cc (right):

https://codereview.chromium.org/2449913002/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_event_details.cc:48: if
(request->url().SchemeIsWSOrWSS()) {
Why would WebRequestEventDetails be created for requests that aren't WS or WSS?

[mmenke, 2016-11-16 18:21:28]

https://codereview.chromium.org/2449913002/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_event_details.cc (right):

https://codereview.chromium.org/2449913002/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_event_details.cc:48: if
(request->url().SchemeIsWSOrWSS()) {
On 2016/11/16 18:20:39, mmenke wrote:
> Why would WebRequestEventDetails be created for requests that aren't WS or
WSS?

Oh, WebRequest...was thinking WebSocketRequest...Sorry.

[battre (please use the other), 2016-11-16 21:51:57]

battre@google.com changed reviewers:
+ battre@google.com

[battre (please use the other), 2016-11-16 21:51:58]

https://codereview.chromium.org/2449913002/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:40: function
webSocketEchoTestImpl(testBody) {
I think that ultimately we want more tests that exercise redirects and blocking.
Add a TODO?

https://codereview.chromium.org/2449913002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:116: //
FIXME(pkalinnikov): Make the tests work. Now they hang forever.
Did you fix this?

[Devlin, 2016-11-16 22:00:56]

A couple high-level comments here:
- We'll want either a) more in-depth testing and to fix all the known failures
or b) to implement this incrementally and disable it by default until we're
ready.  It's hard to review this without knowing whether or not this is meant to
implement full support or is only a step along the way.
- There's an internal thread that had some security questions that seem to have
gone unanswered.  Before we land support (or part of support), we need security
signoff.  It seems like people are mostly on board, but we need a concrete plan
and to iron out any details.

[pmarks, 2016-11-17 00:17:27]

onResponseStarted looks fine now:
{
  "frameId": 0,
  "fromCache": false,
  "ip": "50.16.199.247",
  "method": "GET",
  "parentFrameId": -1,
  "requestId": "590",
  "statusCode": 101,
  "statusLine": "HTTP/1.1 101 Web Socket Protocol Handshake",
  "tabId": 11,
  "timeStamp": 1479341275420.0562,
  "type": "other",
  "url": "wss://demo.kaazing.com/jms?.kl=Y"
}

But a millisecond later, I'm seeing onErrorOccurred, even though the data is
still streaming in:
{
  "error": "net::ERR_ABORTED",
  "frameId": 0,
  "fromCache": false,
  "method": "GET",
  "parentFrameId": -1,
  "requestId": "590",
  "tabId": 11,
  "timeStamp": 1479341275421.714,
  "type": "other",
  "url": "wss://demo.kaazing.com/jms?.kl=Y"
}

[pmarks, 2016-11-17 00:19:23]

(I'm adding myself as a reviewer, because I keep getting dropped from the CC
list.)

[pkalinnikov, 2016-11-17 11:34:12]

Hello,

pmarks@: As tyoshino@ described above, onErrorOccurred is triggered instead of
onCompleted, because current WS implementation gets rid of URLRequest after WS
handshake in a way that doesn't report success. I filed a crbug/663672 for this
issue.

tyoshino@: Takeshi, does your team work on crbug/663672 ?

rdevlin.cronin@: This CL does not provide full support. Plan (b) seems more
feasible, because 2 of the present issues can be factored out of this CL:
1. crbug/663672
2. Add a new content::ResourceType for WebRequest, and then in WebRequest API
map it to some "webrequest" resource type instead of "other".

However, the current CL is kinda consistent. WebRequest API promises to call
either onCompleted or onErrorOccurred for every request. WS now consistently
triggers onErrorOccurred.

Devlin, what do you mean by disabling? Not landing the CL until all issues are
addressed, or landing it with some disabling code?

https://codereview.chromium.org/2449913002/diff/80001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:40: function
webSocketEchoTestImpl(testBody) {
On 2016/11/16 21:51:58, battre (please use the other) wrote:
> I think that ultimately we want more tests that exercise redirects and
blocking.
> Add a TODO?

Added a TODO.

https://codereview.chromium.org/2449913002/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:116: //
FIXME(pkalinnikov): Make the tests work. Now they hang forever.
On 2016/11/16 21:51:58, battre (please use the other) wrote:
> Did you fix this?

Yes. Forgot to delete the comment.

[pkalinnikov, 2016-11-17 11:38:39]

Description was changed from

==========
Support WebSocket in WebRequest API.

BUG=129353
==========

to

==========
Support WebSocket in WebRequest API.

Trigger all WebRequest API event along WebSocket handshake requests. Report WS
requests as "other". The last event in the pipeline is onErrorOccurred instead
of onCompleted, until the issue crbug/663672 gets fixed.

BUG=129353
==========

[mmenke, 2016-11-17 15:31:50]

extensions/browser/api/web_request/web_request_event_details.cc LGTM, but do
make sure you get tyoshino's signoff.  I'm no expert on websockets, and defer to
him.

[tyoshino (SeeGerritForStatus), 2016-12-05 07:32:51]

Sorry for long delay. ps6 lgtm

> tyoshino@: Takeshi, does your team work on crbug/663672 ?

Thank you for filing the bug. Sorry but not yet.

Thanks mmenke@ for the opinions about NetworkDelegate at #22 and #23.

Let's figure out the right way to fix this.

[tyoshino (SeeGerritForStatus), 2016-12-06 08:05:06]

Forgot to publish the draft comments.

https://codereview.chromium.org/2449913002/diff/100001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

https://codereview.chromium.org/2449913002/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/framework.js:210: // intoduced
for the WebSocket requests.
introduced

https://codereview.chromium.org/2449913002/diff/100001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:41:
webSocketUrl = getEchoTestURL(testWebSocketPort);
var

https://codereview.chromium.org/2449913002/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:116: //
TODO(pkalinnikov): Add tests excercising redirects and blocking.
exercise

[pkalinnikov, 2016-12-13 21:15:59]

Hi,

tyoshino@: Thanks for the comments, I have addressed them. Let's discuss
possible solutions for the ERR_ABORTED problem offline/in the bug.

https://codereview.chromium.org/2449913002/diff/100001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

https://codereview.chromium.org/2449913002/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/framework.js:210: // intoduced
for the WebSocket requests.
On 2016/12/06 08:05:06, tyoshino wrote:
> introduced

Done.

https://codereview.chromium.org/2449913002/diff/100001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:41:
webSocketUrl = getEchoTestURL(testWebSocketPort);
On 2016/12/06 08:05:06, tyoshino wrote:
> var

Done.

https://codereview.chromium.org/2449913002/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:116: //
TODO(pkalinnikov): Add tests excercising redirects and blocking.
On 2016/12/06 08:05:06, tyoshino wrote:
> exercise

Done.

[pkalinnikov, 2017-01-16 10:29:34]

Description was changed from

==========
Support WebSocket in WebRequest API.

Trigger all WebRequest API event along WebSocket handshake requests. Report WS
requests as "other". The last event in the pipeline is onErrorOccurred instead
of onCompleted, until the issue crbug/663672 gets fixed.

BUG=129353
==========

to

==========
Support WebSocket in WebRequest API.

Trigger WebRequest API events corresponding to various phases of processing
WebSocket handshake requests. Use "other" type in the details of the request.

BUG=129353
==========

[pkalinnikov, 2017-01-16 10:49:46]

pkalinnikov@chromium.org changed reviewers:
+ battre@chromium.org
- battre@google.com, mpcomplete@chromium.org

[pkalinnikov, 2017-01-16 10:53:22]

The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 10:53:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 12:13:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 12:13:35]

Dry run: This issue passed the CQ dry run.

[pkalinnikov, 2017-01-16 12:31:36]

Hello everyone,

With the CL that is about to land (https://codereview.chromium.org/2628333003)
and the latest change to the current CL, onCompleted will be reported through
WebRequest API when WebSocket handshake request completes.

rdevlin.cronin@: Devlin, please review this version.

pmarks@: Feel free to try it out to make sure you get the right event in your
extension. Note that the type is going to remain "other" in this CL.

[pkalinnikov, 2017-01-16 17:19:17]

pkalinnikov@chromium.org changed reviewers:
- battre@chromium.org

[mmenke, 2017-01-17 15:54:29]

On 2017/01/16 12:31:36, pkalinnikov wrote:
> Hello everyone,
> 
> With the CL that is about to land (https://codereview.chromium.org/2628333003)
> and the latest change to the current CL, onCompleted will be reported through
> WebRequest API when WebSocket handshake request completes.
> 
> rdevlin.cronin@: Devlin, please review this version.
> 
> pmarks@: Feel free to try it out to make sure you get the right event in your
> extension. Note that the type is going to remain "other" in this CL.

Still LGTM (Sorry, too many reviews, and no other way to make this one non-bold
than another signoff.  At least I can not send out an email)

[pmarks, 2017-01-17 21:17:45]

On 2017/01/16 12:31:36, pkalinnikov wrote:
> pmarks@: Feel free to try it out to make sure you get the right event in your
> extension. Note that the type is going to remain "other" in this CL.

The events appear to be firing as you described (I see onCompleted after the
handshake.)

That's better than nothing, but extensions that track open connections will need
ugly hacks, like assuming that ws/wss sessions remain active until the next
navigation event.

[pkalinnikov, 2017-01-19 14:53:40]

On 2017/01/17 21:17:45, pmarks wrote:
> On 2017/01/16 12:31:36, pkalinnikov wrote:
> > pmarks@: Feel free to try it out to make sure you get the right event in
your
> > extension. Note that the type is going to remain "other" in this CL.
> 
> The events appear to be firing as you described (I see onCompleted after the
> handshake.)
> 
> That's better than nothing, but extensions that track open connections will
need
> ugly hacks, like assuming that ws/wss sessions remain active until the next
> navigation event.

Hi Paul,

I agree with you, that this would be ideal to observe the full WebSocket
lifetime. However, this would require significantly more work, including
WebRequest API's refactoring, because the WS flow is a bit different from
HTTP-like one that the API was build for.

Some time ago, together with cbentzel@, mkwst@, tyoshino@, we have reached an
agreement via email, that intercepting just a WS handshake is a good start.
Sorry for not letting you know, it's a bit tough to keep all the people involved
in this process informed :)

[pkalinnikov, 2017-01-19 18:24:36]

The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 18:25:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pkalinnikov, 2017-01-19 18:27:15]

rdevlin.cronin@: Hi Devlin. A friendly ping for you to review the CL once more.
Your sign-off is very important.

[pkalinnikov, 2017-01-19 18:33:56]

The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 18:34:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 19:45:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 19:45:20]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-01-19 21:14:47]

On 2017/01/19 18:27:15, pkalinnikov wrote:
> rdevlin.cronin@: Hi Devlin. A friendly ping for you to review the CL once
more.
> Your sign-off is very important.

Just FYI, I think he's busy at an event this week, but hopefully he'll get a
chance to look when he gets back.

[pkalinnikov, 2017-02-01 09:20:42]

Hello,

One more friendly reminder to not overlook this CL. Thank you.

[mmenke, 2017-02-01 13:24:09]

On 2017/02/01 09:20:42, pkalinnikov wrote:
> Hello,
> 
> One more friendly reminder to not overlook this CL. Thank you.

Just a friendly tip:  Be clear who you're reminding.  This makes it more likely
that you'll get that person's attention, and prevent you from getting the wrong
people's attention.

[pkalinnikov, 2017-02-01 14:02:10]

On 2017/02/01 13:24:09, mmenke (Out Feb 4 to March 5) wrote:
> On 2017/02/01 09:20:42, pkalinnikov wrote:
> > Hello,
> > 
> > One more friendly reminder to not overlook this CL. Thank you.
> 
> Just a friendly tip:  Be clear who you're reminding.  This makes it more
likely
> that you'll get that person's attention, and prevent you from getting the
wrong
> people's attention.

Thanks for the tip. I was addressing rdevlin.cronin@.

[pkalinnikov, 2017-02-07 14:30:18]

pkalinnikov@chromium.org changed reviewers:
+ battre@chromium.org

[pkalinnikov, 2017-02-07 14:30:19]

battre@: Dominic, I just realized that your review is needed, as you are the
owner of "chrome/browser/extensions/api/web_request/".

rdevlin.cronin@: Devlin, please review the CL.

[battre, 2017-02-07 15:03:02]

How about updating https://developer.chrome.com/extensions/webRequest and
explaining the tab ID for the WS scheme if we cannot fix that?

https://codereview.chromium.org/2449913002/diff/180001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:50: tabId: 1,
Hm, do you think this can be fixed?

https://codereview.chromium.org/2449913002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:116: //
TODO(pkalinnikov): Add tests exercising redirects and blocking.
yes, please.

[pkalinnikov, 2017-02-09 18:15:05]

Addressed comments, thanks!

One important case showed up during testing: WebSocket handshake shouldn't be
redirected. This probably requires adding some restrictions for WS scheme in the
code of API to prevent extensions from doing that. Will look into that.

https://codereview.chromium.org/2449913002/diff/180001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:50: tabId: 1,
TL;DR: This is not a bug. See
chrome/test/data/extensions/api_test/webrequest/test_types.js. A comments says:
  // tabId 0 = tab opened by test runner;
  // tabId 1 = this tab.

In details, what happens here:

WebRequestApiTest creates a tab and navigates it to "test_websocket.html". When
"test_websocket.js" gets run, it calls framework.js:runTests, which creates a
new about:blank tab. The newly created tab is recorded as tab number 0 in the
numeration system of framework.js (see |tabIdMap|). Right after that
chrome.test.runTests(tests) is executed, and this is when tests from
"test_websocket.js" are actually started. Only when the first WS request comes
back to framework.js in a form of onBeforeRequest callback, |tabIdMap| records a
new entry (numbered 1) associated with the initial tab.

Although it is not a bug, the new tab is created for nothing. I factored out the
pure test-running code to framework.js:runTestsForTab to avoid this. Now all the
tabId's in this test are 0 (default, so could be omitted).

https://codereview.chromium.org/2449913002/diff/180001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:116: //
TODO(pkalinnikov): Add tests exercising redirects and blocking.
On 2017/02/07 15:03:02, battre (OOO) wrote:
> yes, please.

Added a test with blocking.

I also tried adding a test with a redirect in onBeforeRequest, but it had caused
a fatal error in
net/websockets/websocket_stream.cc:Delegate::OnReceivedRedirect. Apparently,
WebSocket handshake is not permitted to be redirected, and WebRequest API should
prevent this.

[pkalinnikov, 2017-02-10 10:23:24]

The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 10:23:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 11:29:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 11:29:24]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-02-14 01:21:39]

Very sorry for the extremely, extremely delayed review here.  (If that happens
in the future, please feel free to ping me on hangouts.)

https://codereview.chromium.org/2449913002/diff/200001/chrome/common/extensio...
File chrome/common/extensions/docs/templates/intros/webRequest.html (right):

https://codereview.chromium.org/2449913002/diff/200001/chrome/common/extensio...
chrome/common/extensions/docs/templates/intros/webRequest.html:149: supports
intercepting WebSocket handshake request. Since the handshake is done
nit:
intercepting *the* WebSocket handshake...

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/framework.js:215: //
TODO(pkalinnikov): Remove the startsWith('ws') check once a new type gets
When will this be?

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.html
(right):

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.html:2: *
Copyright 2016 The Chromium Authors. All rights reserved.
not 2016

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:14: var
MESSAGE = "test message";
single quotes in js.  Also, typically prefer kMessage instead of MESSAGE.

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:16: var
connectionOpenExpected;
why not just something like:
var keepalive = chrome.test.callbackAdded();

and then use assert[False|True](expectedToConnect)?

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:24:
chrome.test.log("WebSocket error: " + error);
single quotes (whole file)

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:151: },
redirect test?

https://codereview.chromium.org/2449913002/diff/200001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2449913002/diff/200001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:982:
OnCompleted(browser_context, extension_info_map, request, net_error);
Document why we do this?

https://codereview.chromium.org/2449913002/diff/200001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_event_details.cc (right):

https://codereview.chromium.org/2449913002/diff/200001/extensions/browser/api...
extensions/browser/api/web_request/web_request_event_details.cc:48: if
(request->url().SchemeIsWSOrWSS()) {
Can we put this if on line 45?
if (info) {
  ...
} else if (request->url().SchemeIsWSOrWSS()) {
  ...
} else {
  // Fallback for requests...
}

https://codereview.chromium.org/2449913002/diff/200001/extensions/common/url_...
File extensions/common/url_pattern.cc (right):

https://codereview.chromium.org/2449913002/diff/200001/extensions/common/url_...
extensions/common/url_pattern.cc:325: valid_schemes_ &= (SCHEME_HTTP |
SCHEME_HTTPS | SCHEME_WS | SCHEME_WSS);
Hmm... I'm not sure this is preferable.  We don't want most APIs to be able to
(try to) do anything with ws:/wss: schemes.  I'd be inclined to say we should
have them specify them explicitly separately, as we do for other "unusual"
schemes.

[pkalinnikov, 2017-02-14 13:49:51]

rdevlin.cronin@: Thanks for the review. There are couple of questions for you.

https://codereview.chromium.org/2449913002/diff/200001/chrome/common/extensio...
File chrome/common/extensions/docs/templates/intros/webRequest.html (right):

https://codereview.chromium.org/2449913002/diff/200001/chrome/common/extensio...
chrome/common/extensions/docs/templates/intros/webRequest.html:149: supports
intercepting WebSocket handshake request. Since the handshake is done
On 2017/02/14 01:21:38, Devlin wrote:
> nit:
> intercepting *the* WebSocket handshake...

Done.

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/framework.js:215: //
TODO(pkalinnikov): Remove the startsWith('ws') check once a new type gets
On 2017/02/14 01:21:38, Devlin wrote:
> When will this be?

I was thinking to address this in a follow-up CL. Do you think it should be done
in this one?

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.html
(right):

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.html:2: *
Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/02/14 01:21:38, Devlin wrote:
> not 2016

Yeah, it's been a while :)

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:14: var
MESSAGE = "test message";
On 2017/02/14 01:21:38, Devlin wrote:
> single quotes in js.  Also, typically prefer kMessage instead of MESSAGE.

Done.

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:16: var
connectionOpenExpected;
On 2017/02/14 01:21:38, Devlin wrote:
> why not just something like:
> var keepalive = chrome.test.callbackAdded();
> 
> and then use assert[False|True](expectedToConnect)?

Indeed.

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:24:
chrome.test.log("WebSocket error: " + error);
On 2017/02/14 01:21:38, Devlin wrote:
> single quotes (whole file)

Done.

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:151: },
On 2017/02/14 01:21:38, Devlin wrote:
> redirect test?

I tried doing redirect, and it didn't work (see my previous response to
Dominic's comments).

I see 2 options:
1. Disallow redirecting WS from WebRequest API.
2. As Adam Rice pointed out, although server redirects of WS requests are
prohibited for security reasons, it would be possible to make an exception for
synthetic redirects from extensions.

What do you think is the right way to go? I would probably go with whatever is
the simplest, which is probably the option 1.

https://codereview.chromium.org/2449913002/diff/200001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2449913002/diff/200001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:982:
OnCompleted(browser_context, extension_info_map, request, net_error);
On 2017/02/14 01:21:38, Devlin wrote:
> Document why we do this?

Done.

https://codereview.chromium.org/2449913002/diff/200001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_event_details.cc (right):

https://codereview.chromium.org/2449913002/diff/200001/extensions/browser/api...
extensions/browser/api/web_request/web_request_event_details.cc:48: if
(request->url().SchemeIsWSOrWSS()) {
On 2017/02/14 01:21:38, Devlin wrote:
> Can we put this if on line 45?
> if (info) {
>   ...
> } else if (request->url().SchemeIsWSOrWSS()) {
>   ...
> } else {
>   // Fallback for requests...
> }

Done.

https://codereview.chromium.org/2449913002/diff/200001/extensions/common/url_...
File extensions/common/url_pattern.cc (right):

https://codereview.chromium.org/2449913002/diff/200001/extensions/common/url_...
extensions/common/url_pattern.cc:325: valid_schemes_ &= (SCHEME_HTTP |
SCHEME_HTTPS | SCHEME_WS | SCHEME_WSS);
On 2017/02/14 01:21:38, Devlin wrote:
> Hmm... I'm not sure this is preferable.  We don't want most APIs to be able to
> (try to) do anything with ws:/wss: schemes.  I'd be inclined to say we should
> have them specify them explicitly separately, as we do for other "unusual"
> schemes.

SGTM.

[pkalinnikov, 2017-02-14 13:51:09]

The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 13:51:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 15:04:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 15:04:30]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-02-14 15:55:24]

(just responding to questions; will take another look later today)

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/framework.js:215: //
TODO(pkalinnikov): Remove the startsWith('ws') check once a new type gets
On 2017/02/14 13:49:51, pkalinnikov wrote:
> On 2017/02/14 01:21:38, Devlin wrote:
> > When will this be?
> 
> I was thinking to address this in a follow-up CL. Do you think it should be
done
> in this one?

I don't think this should be too complicated, and I'd rather do it at the same
time.  If we separate adding the functionality and differentiating, it means
that any extensions that try to use this have to code, like this, details.type
== 'other' && details.url.startsWith('ws') - and that kind of code has a nasty
habit of staying around.

If this ends up being painful, lemme know, but I think it should be
straightforward and should be addressed here.

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:151: },
On 2017/02/14 13:49:51, pkalinnikov wrote:
> On 2017/02/14 01:21:38, Devlin wrote:
> > redirect test?
> 
> I tried doing redirect, and it didn't work (see my previous response to
> Dominic's comments).
> 
> I see 2 options:
> 1. Disallow redirecting WS from WebRequest API.
> 2. As Adam Rice pointed out, although server redirects of WS requests are
> prohibited for security reasons, it would be possible to make an exception for
> synthetic redirects from extensions.
> 
> What do you think is the right way to go? I would probably go with whatever is
> the simplest, which is probably the option 1.

I think that disallowing redirect is fine, especially since we're not sure
there's a demand for it and it's more inline with the traditional web socket
behavior.  But I think we should explicitly test that behavior so that it's
deterministic, and add a comment explaining it.

[pkalinnikov, 2017-02-15 14:41:16]

The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 14:41:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 15:00:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 15:00:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pkalinnikov, 2017-02-15 15:38:21]

The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 15:38:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 16:28:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 16:28:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pkalinnikov, 2017-02-15 18:49:25]

The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 18:50:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pkalinnikov, 2017-02-15 18:56:03]

Description was changed from

==========
Support WebSocket in WebRequest API.

Trigger WebRequest API events corresponding to various phases of processing
WebSocket handshake requests. Use "other" type in the details of the request.

BUG=129353
==========

to

==========
Support WebSocket in WebRequest API.

Trigger WebRequest API events corresponding to various phases of processing
WebSocket handshake requests. Introduce new 'websocket' type, which is now
added to the details of the request. Redirects of WS are ignored.

BUG=129353
==========

[pkalinnikov, 2017-02-15 19:05:28]

Description was changed from

==========
Support WebSocket in WebRequest API.

Trigger WebRequest API events corresponding to various phases of processing
WebSocket handshake requests. Introduce new 'websocket' type, which is now
added to the details of the request. Redirects of WS are ignored.

BUG=129353
==========

to

==========
Support WebSocket in WebRequest API.

This CL makes WebRequest API support intercepting the WebSocket handshake
request. Since the handshake is done by means of an HTTP upgrade request, its
flow fits into HTTP-oriented WebRequest model. Additional restriction applies,
that WS request redirects triggered by extensions are ignored.

Note that WebRequest API does *not intercept*:
 - Individual messages sent over an established WebSocket connection.
 - WebSocket closing connection.

The |details| object reported to extensions uses the newly introduced type of
request: 'websocket'.

BUG=129353
==========

[pkalinnikov, 2017-02-15 19:15:00]

rdevlin.cronin@:

Devlin, I have addressed your major concerns. Please review once again.

Apart from that, I am going to add more js tests, e.g., a test exercising the
onAuthRequired flow.

According to chrome/common/extensions/docs/README, there should be a way to open
the modified documentation following a CL-specific link:
https://chrome-apps-doc.appspot.com/_patch/2449913002/extensions/webRequest
Do you know why it doesn't work?

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/framework.js:215: //
TODO(pkalinnikov): Remove the startsWith('ws') check once a new type gets
On 2017/02/14 15:55:24, Devlin wrote:
> On 2017/02/14 13:49:51, pkalinnikov wrote:
> > On 2017/02/14 01:21:38, Devlin wrote:
> > > When will this be?
> > 
> > I was thinking to address this in a follow-up CL. Do you think it should be
> done
> > in this one?
> 
> I don't think this should be too complicated, and I'd rather do it at the same
> time.  If we separate adding the functionality and differentiating, it means
> that any extensions that try to use this have to code, like this, details.type
> == 'other' && details.url.startsWith('ws') - and that kind of code has a nasty
> habit of staying around.
> 
> If this ends up being painful, lemme know, but I think it should be
> straightforward and should be addressed here.

Added new type in a separate CL.

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:151: },
On 2017/02/14 15:55:24, Devlin wrote:
> On 2017/02/14 13:49:51, pkalinnikov wrote:
> > On 2017/02/14 01:21:38, Devlin wrote:
> > > redirect test?
> > 
> > I tried doing redirect, and it didn't work (see my previous response to
> > Dominic's comments).
> > 
> > I see 2 options:
> > 1. Disallow redirecting WS from WebRequest API.
> > 2. As Adam Rice pointed out, although server redirects of WS requests are
> > prohibited for security reasons, it would be possible to make an exception
for
> > synthetic redirects from extensions.
> > 
> > What do you think is the right way to go? I would probably go with whatever
is
> > the simplest, which is probably the option 1.
> 
> I think that disallowing redirect is fine, especially since we're not sure
> there's a demand for it and it's more inline with the traditional web socket
> behavior.  But I think we should explicitly test that behavior so that it's
> deterministic, and add a comment explaining it.

Added redirect test.

[commit-bot: I haz the power, 2017-02-15 20:53:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 20:53:27]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-02-15 22:12:58]

This largely looks good, but I'd like to take another look once the resource
type discussion is resolved in https://codereview.chromium.org/2700553002/.

On 2017/02/15 19:15:00, pkalinnikov wrote:
> According to chrome/common/extensions/docs/README, there should be a way to
open
> the modified documentation following a CL-specific link:
> https://chrome-apps-doc.appspot.com/_patch/2449913002/extensions/webRequest
> Do you know why it doesn't work?

lazyboy@ has been poking the docserver lately.  I've never actually used that
feature, so I'm not sure if it used to work, or when it stopped.

Generally, the way to preview changes locally is to run 
$ python chrome/common/extensions/docs/server2/preview.py
which will spin up a local server for the docs, and then you can visit the
article.

https://codereview.chromium.org/2449913002/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
(right):

https://codereview.chromium.org/2449913002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:97: const
GURL kExampleUrl("http://example.com");
afaik, static members must be POD, even in tests.

https://codereview.chromium.org/2449913002/diff/280001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/280001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:12: function
testWebSocketConnection(url, expectedToConnect = true) {
nitty nit: I don't think the default argument here is worthwhile, since we only
call it 3 times.  I'd just pass true/false in each case.

[pkalinnikov, 2017-02-16 21:30:46]

The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run

[pkalinnikov, 2017-02-16 21:31:07]

Addressed.

https://codereview.chromium.org/2449913002/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
(right):

https://codereview.chromium.org/2449913002/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:97: const
GURL kExampleUrl("http://example.com");
On 2017/02/15 22:12:58, Devlin wrote:
> afaik, static members must be POD, even in tests.

Done.

https://codereview.chromium.org/2449913002/diff/280001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/280001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:12: function
testWebSocketConnection(url, expectedToConnect = true) {
On 2017/02/15 22:12:58, Devlin wrote:
> nitty nit: I don't think the default argument here is worthwhile, since we
only
> call it 3 times.  I'd just pass true/false in each case.

Done.

[commit-bot: I haz the power, 2017-02-16 21:33:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 22:16:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 22:16:16]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[pkalinnikov, 2017-02-20 15:35:01]

The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 15:35:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pkalinnikov, 2017-02-20 16:14:28]

rdevlin.cronin@: Devlin, I have covered all the concerns by now, and added more
tests. Please review it once again. Thank you.

[commit-bot: I haz the power, 2017-02-20 17:00:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 17:00:08]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-02-21 16:10:44]

https://codereview.chromium.org/2449913002/diff/320001/chrome/common/extensio...
File chrome/common/extensions/docs/templates/intros/webRequest.html (right):

https://codereview.chromium.org/2449913002/diff/320001/chrome/common/extensio...
chrome/common/extensions/docs/templates/intros/webRequest.html:148: <span
class="availability">Starting from Chrome 58</span>, webRequest API
nit: *the* webRequest API

https://codereview.chromium.org/2449913002/diff/320001/chrome/common/extensio...
chrome/common/extensions/docs/templates/intros/webRequest.html:156: </p>
should we also mention that redirects are not supported?

edit: I see that we do this on the method, so that's probably sufficient.  It
might still be worth calling out explicitly here, but I don't feel strongly, so
up to you.

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/framework.js:216: // introduced
for the WebSocket requests.
This is done, right?

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:18: frameUrl:
'unknown frame URL',
Can we not determine the url that initiated the request?  Should there be a TODO
for it?

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:73:
testWebSocketConnection(url, true);
document what 'true' is (same for line 108, 181)

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket_auth.js
(right):

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket_auth.js:298:
retval: {authCredentials: {username: 'test', password: 'test'}}
Do we verify anywhere if the authCredentials were correctly used?

https://codereview.chromium.org/2449913002/diff/320001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2449913002/diff/320001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:943: DCHECK(net_error ==
net::OK || net_error == net::ERR_WS_UPGRADE);
nit: Can we add
// See comment in OnErrorOccurred re net::ERR_WS_UPGRADE.

https://codereview.chromium.org/2449913002/diff/320001/extensions/common/api/...
File extensions/common/api/web_request.json (right):

https://codereview.chromium.org/2449913002/diff/320001/extensions/common/api/...
extensions/common/api/web_request.json:114: "description": "Only used as a
response to the onBeforeRequest and onHeadersReceived events. If set, the
original request is prevented from being sent/completed and is instead
redirected to the given URL. Redirections to non-HTTP schemes such as
<code>data://</code> are allowed. Redirects initiated by a redirect action use
the original request method for the redirect, with one exception: If the
redirect is initiated at the onHeadersReceived stage, then the redirect will be
issued using the GET method. Redirects from URLs with <code>ws://</code> and
<code>wss://</code> schemes are <b>ignored</b>."
nit: While data: is technically a scheme, I don't think I've ever seen it with
the trailing '//'.  Let's keep this as the more common 'data:'.  (Adding the
<code> tags is good, though!)

[pkalinnikov, 2017-02-21 19:17:04]

The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 19:17:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pkalinnikov, 2017-02-21 19:18:16]

Addressed, thank you. Are we close to submit this?

https://codereview.chromium.org/2449913002/diff/320001/chrome/common/extensio...
File chrome/common/extensions/docs/templates/intros/webRequest.html (right):

https://codereview.chromium.org/2449913002/diff/320001/chrome/common/extensio...
chrome/common/extensions/docs/templates/intros/webRequest.html:148: <span
class="availability">Starting from Chrome 58</span>, webRequest API
On 2017/02/21 16:10:44, Devlin (ooo Feb20-US Holiday) wrote:
> nit: *the* webRequest API

Done.

https://codereview.chromium.org/2449913002/diff/320001/chrome/common/extensio...
chrome/common/extensions/docs/templates/intros/webRequest.html:156: </p>
On 2017/02/21 16:10:44, Devlin (ooo Feb20-US Holiday) wrote:
> should we also mention that redirects are not supported?
> 
> edit: I see that we do this on the method, so that's probably sufficient.  It
> might still be worth calling out explicitly here, but I don't feel strongly,
so
> up to you.

Done.

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/framework.js:216: // introduced
for the WebSocket requests.
On 2017/02/21 16:10:44, Devlin (ooo Feb20-US Holiday) wrote:
> This is done, right?

Done.

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:18: frameUrl:
'unknown frame URL',
On 2017/02/21 16:10:44, Devlin (ooo Feb20-US Holiday) wrote:
> Can we not determine the url that initiated the request?  Should there be a
TODO
> for it?

This field is not the API's thing, it is added by framework.js when captureEvent
is invoked for an "onBeforeRequest". The absence of URL means that captureEvent
was not called for the frame initiated the WS request. I think this is an
unrelated issue, if issue at all. Other tests don't intercept flow of the
documents that were loaded by the browsertest, only the flow of documents
navigated to *from js*. The WS tests don't navigate from JS, so maybe this is
the reason.

I will leave a TODO if you don't object.

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:73:
testWebSocketConnection(url, true);
On 2017/02/21 16:10:44, Devlin (ooo Feb20-US Holiday) wrote:
> document what 'true' is (same for line 108, 181)

Done.

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket_auth.js
(right):

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket_auth.js:298:
retval: {authCredentials: {username: 'test', password: 'test'}}
On 2017/02/21 16:10:44, Devlin (ooo Feb20-US Holiday) wrote:
> Do we verify anywhere if the authCredentials were correctly used?

As discussed offline, I put a comment elaborating that these are the only
credentials accepted by the test server.

https://codereview.chromium.org/2449913002/diff/320001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2449913002/diff/320001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:943: DCHECK(net_error ==
net::OK || net_error == net::ERR_WS_UPGRADE);
On 2017/02/21 16:10:44, Devlin (ooo Feb20-US Holiday) wrote:
> nit: Can we add
> // See comment in OnErrorOccurred re net::ERR_WS_UPGRADE.

What is "re"? I put "regarding", looks good?

https://codereview.chromium.org/2449913002/diff/320001/extensions/common/api/...
File extensions/common/api/web_request.json (right):

https://codereview.chromium.org/2449913002/diff/320001/extensions/common/api/...
extensions/common/api/web_request.json:114: "description": "Only used as a
response to the onBeforeRequest and onHeadersReceived events. If set, the
original request is prevented from being sent/completed and is instead
redirected to the given URL. Redirections to non-HTTP schemes such as
<code>data://</code> are allowed. Redirects initiated by a redirect action use
the original request method for the redirect, with one exception: If the
redirect is initiated at the onHeadersReceived stage, then the redirect will be
issued using the GET method. Redirects from URLs with <code>ws://</code> and
<code>wss://</code> schemes are <b>ignored</b>."
On 2017/02/21 16:10:44, Devlin (ooo Feb20-US Holiday) wrote:
> nit: While data: is technically a scheme, I don't think I've ever seen it with
> the trailing '//'.  Let's keep this as the more common 'data:'.  (Adding the
> <code> tags is good, though!)

Done.

[Devlin, 2017-02-21 20:34:11]

lgtm.  Thanks so much for your work and patience here!

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right):

https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/test_websocket.js:18: frameUrl:
'unknown frame URL',
On 2017/02/21 19:18:16, pkalinnikov wrote:
> On 2017/02/21 16:10:44, Devlin (ooo Feb20-US Holiday) wrote:
> > Can we not determine the url that initiated the request?  Should there be a
> TODO
> > for it?
> 
> This field is not the API's thing, it is added by framework.js when
captureEvent
> is invoked for an "onBeforeRequest". The absence of URL means that
captureEvent
> was not called for the frame initiated the WS request. I think this is an
> unrelated issue, if issue at all. Other tests don't intercept flow of the
> documents that were loaded by the browsertest, only the flow of documents
> navigated to *from js*. The WS tests don't navigate from JS, so maybe this is
> the reason.
> 
> I will leave a TODO if you don't object.

Whoops, mistook this for url from the API. Not an issue.

https://codereview.chromium.org/2449913002/diff/320001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2449913002/diff/320001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:943: DCHECK(net_error ==
net::OK || net_error == net::ERR_WS_UPGRADE);
On 2017/02/21 19:18:16, pkalinnikov wrote:
> On 2017/02/21 16:10:44, Devlin (ooo Feb20-US Holiday) wrote:
> > nit: Can we add
> > // See comment in OnErrorOccurred re net::ERR_WS_UPGRADE.
> 
> What is "re"? I put "regarding", looks good?

Looks good.  Yep, re == regarding. [1]

[1]
https://www.google.com/search?q=define%3Are&oq=define%3Are&aqs=chrome..69i57j...

https://codereview.chromium.org/2449913002/diff/340001/chrome/browser/extensi...
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
(right):

https://codereview.chromium.org/2449913002/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:1762:
EXPECT_EQ(GURL::EmptyGURL(), effective_new_url);
nit: just use GURL() instead of EmptyGURL()

[commit-bot: I haz the power, 2017-02-21 21:20:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 21:20:22]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[pkalinnikov, 2017-02-21 22:24:29]

The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 22:25:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pkalinnikov, 2017-02-21 22:28:53]

pkalinnikov@chromium.org changed reviewers:
- battre@chromium.org

[pkalinnikov, 2017-02-21 22:28:54]

Thanks Devlin! Hopefully the "git cl format" change looks good to you as well.

Everyone: I will leave this CL to pass trybots and commit it in about 10 hours
if everything is fine.

https://codereview.chromium.org/2449913002/diff/340001/chrome/browser/extensi...
File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc
(right):

https://codereview.chromium.org/2449913002/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_api_unittest.cc:1762:
EXPECT_EQ(GURL::EmptyGURL(), effective_new_url);
On 2017/02/21 20:34:11, Devlin wrote:
> nit: just use GURL() instead of EmptyGURL()

Done.

[commit-bot: I haz the power, 2017-02-22 00:30:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 00:30:29]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[pkalinnikov, 2017-02-22 07:31:31]

The CQ bit was checked by pkalinnikov@chromium.org

[pkalinnikov, 2017-02-22 07:31:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org, mmenke@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2449913002/#ps380001
(title: "git cl format")

[commit-bot: I haz the power, 2017-02-22 07:32:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 11:11:28]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1487748691579760,
"parent_rev": "e42e559eb580c5eefecdf2868b615bda2e9a487b", "commit_rev":
"0f198df6bc8da6fe51b34f11b50936700b6e9353"}

[commit-bot: I haz the power, 2017-02-22 11:12:40]

Description was changed from

==========
Support WebSocket in WebRequest API.

This CL makes WebRequest API support intercepting the WebSocket handshake
request. Since the handshake is done by means of an HTTP upgrade request, its
flow fits into HTTP-oriented WebRequest model. Additional restriction applies,
that WS request redirects triggered by extensions are ignored.

Note that WebRequest API does *not intercept*:
 - Individual messages sent over an established WebSocket connection.
 - WebSocket closing connection.

The |details| object reported to extensions uses the newly introduced type of
request: 'websocket'.

BUG=129353
==========

to

==========
Support WebSocket in WebRequest API.

This CL makes WebRequest API support intercepting the WebSocket handshake
request. Since the handshake is done by means of an HTTP upgrade request, its
flow fits into HTTP-oriented WebRequest model. Additional restriction applies,
that WS request redirects triggered by extensions are ignored.

Note that WebRequest API does *not intercept*:
 - Individual messages sent over an established WebSocket connection.
 - WebSocket closing connection.

The |details| object reported to extensions uses the newly introduced type of
request: 'websocket'.

BUG=129353

Review-Url: https://codereview.chromium.org/2449913002
Cr-Commit-Position: refs/heads/master@{#451991}
Committed:
https://chromium.googlesource.com/chromium/src/+/0f198df6bc8da6fe51b34f11b509...
==========

[commit-bot: I haz the power, 2017-02-22 11:12:41]

Committed patchset #20 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/0f198df6bc8da6fe51b34f11b509...