Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(91)

Issue 2449913002: Support WebSocket in WebRequest API. (Closed)

Created:
1 year, 1 month ago by pkalinnikov
Modified:
9 months, 3 weeks ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, nasko, Adam Rice, lazyboy, battre
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Add WS and WSS schemes to default permissions. #

Patch Set 3 : Add apitest (not yet working, WIP). #

Patch Set 4 : Change the expected events in tests. Add comments. #

Patch Set 5 : Extract process and frame ID from WS requests. #

Total comments: 6

Patch Set 6 : Remove FIXME because the test works. Add TODO for more tests. #

Total comments: 6

Patch Set 7 : Address comments from tyoshino@. #

Patch Set 8 : Report onCompleted instead of onErrorOccurred. #

Patch Set 9 : Rebase. #

Patch Set 10 : Fix build. #

Total comments: 4

Patch Set 11 : Refactor tests; add test; update documentation. #

Total comments: 24

Patch Set 12 : Address comments from Devlin. #

Patch Set 13 : Ignore WS redirects in blocking responses. #

Patch Set 14 : Fix tests; add test; update documentation. #

Patch Set 15 : Fix unittest; add 'websocket' type to WebRequest API. #

Total comments: 4

Patch Set 16 : Address comments; rebase. #

Patch Set 17 : Add tests for onAuthRequired path for WS handshake. #

Total comments: 18

Patch Set 18 : Address comments from Devlin. #

Total comments: 2

Patch Set 19 : Use GURL() instead of EmptyGURL(). #

Patch Set 20 : git cl format #

Unified diffs Side-by-side diffs Delta from patch set Stats (+781 lines, -135 lines) Patch
M chrome/browser/extensions/api/web_request/web_request_api_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 18 chunks +60 lines, -58 lines 0 comments Download
M chrome/browser/extensions/api/web_request/web_request_apitest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +20 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_apitest.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/extensions/extension_apitest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +3 lines, -1 line 0 comments Download
M chrome/common/extensions/docs/templates/intros/webRequest.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +15 lines, -1 line 0 comments Download
M chrome/test/data/extensions/api_test/webrequest/framework.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +18 lines, -9 lines 0 comments Download
M chrome/test/data/extensions/api_test/webrequest/test_types.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +3 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/webrequest/test_websocket.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +10 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/webrequest/test_websocket.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +185 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/webrequest/test_websocket_auth.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +10 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/webrequest/test_websocket_auth.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +332 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/webrequest/websocket.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +40 lines, -0 lines 0 comments Download
M extensions/browser/api/web_request/web_request_api.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +0 lines, -5 lines 0 comments Download
M extensions/browser/api/web_request/web_request_api.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 7 chunks +26 lines, -24 lines 0 comments Download
M extensions/browser/api/web_request/web_request_api_helpers.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +4 lines, -1 line 0 comments Download
M extensions/browser/api/web_request/web_request_api_helpers.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +16 lines, -7 lines 0 comments Download
M extensions/browser/api/web_request/web_request_event_details.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +12 lines, -2 lines 0 comments Download
M extensions/browser/api/web_request/web_request_permissions.cc View 1 2 3 4 5 6 1 chunk +2 lines, -3 lines 0 comments Download
M extensions/common/api/web_request.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -1 line 0 comments Download
M extensions/common/extension.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +4 lines, -5 lines 0 comments Download
M extensions/common/url_pattern.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +10 lines, -8 lines 0 comments Download
M extensions/common/url_pattern.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +7 lines, -8 lines 0 comments Download

Messages

Total messages: 119 (65 generated)
pkalinnikov
Dominic, can you please take a first look? I tested this by running an extension ...
1 year, 1 month ago (2016-10-25 13:59:44 UTC) #3
pkalinnikov
Takeshi, can you please take a look and suggest if you see any issues with ...
1 year, 1 month ago (2016-10-25 14:04:48 UTC) #5
pmarks
Is this supposed to be more than a 2-line change? I tried this with http://demo.kaazing.com/forex/ ...
1 year, 1 month ago (2016-10-26 03:15:01 UTC) #6
tyoshino (SeeGerritForStatus)
Thanks for working on this. Could you please try to write some tests for it ...
1 year, 1 month ago (2016-10-26 10:34:19 UTC) #7
pkalinnikov
On 2016/10/26 03:15:01, pmarks wrote: > Is this supposed to be more than a 2-line ...
1 year, 1 month ago (2016-10-26 11:21:58 UTC) #8
pmarks
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 ...
1 year, 1 month ago (2016-10-26 18:38:26 UTC) #9
pkalinnikov
On 2016/10/26 18:38:26, pmarks wrote: > While running http://demo.kaazing.com/forex/ with the Network debugger open, I ...
1 year, 1 month ago (2016-10-28 11:39:22 UTC) #11
pkalinnikov
mpcomplete@: Hi Matt, can you please take a look? I modified some default permissions for ...
1 year, 1 month ago (2016-10-28 11:43:22 UTC) #12
pkalinnikov
rdevlin.cronin@: Hi Devlin. Your opinion is needed on modifying permissions. Thank you.
1 year, 1 month ago (2016-10-28 11:56:15 UTC) #14
Devlin
+nasko, creis as FYI. In principle, I think this is fine - I see no ...
1 year, 1 month ago (2016-10-28 16:17:02 UTC) #15
pmarks
On 2016/10/28 11:39:22, pkalinnikov wrote: > On 2016/10/26 18:38:26, pmarks wrote: > > While running ...
1 year, 1 month ago (2016-10-29 03:14:06 UTC) #16
pmarks
On 2016/10/29 03:14:06, pmarks wrote: > Sorry, I missed your message because I hadn't CC'd ...
1 year, 1 month ago (2016-10-30 00:06:57 UTC) #17
tyoshino (SeeGerritForStatus)
+mmenke to get input about URLRequest usage. Hi Matt, Currently, the WebSocket stack is designed ...
1 year, 1 month ago (2016-11-08 11:14:49 UTC) #19
mmenke
On 2016/11/08 11:14:49, tyoshino wrote: > +mmenke to get input about URLRequest usage. > > ...
1 year, 1 month ago (2016-11-08 15:25:44 UTC) #20
tyoshino (SeeGerritForStatus)
On 2016/11/08 15:25:44, mmenke wrote: > On 2016/11/08 11:14:49, tyoshino wrote: > > +mmenke to ...
1 year, 1 month ago (2016-11-09 01:29:15 UTC) #21
mmenke
On 2016/11/09 01:29:15, tyoshino wrote: > On 2016/11/08 15:25:44, mmenke wrote: > > On 2016/11/08 ...
1 year, 1 month ago (2016-11-09 15:45:27 UTC) #22
mmenke
On 2016/11/09 15:45:27, mmenke wrote: > On 2016/11/09 01:29:15, tyoshino wrote: > > On 2016/11/08 ...
1 year, 1 month ago (2016-11-09 15:49:16 UTC) #23
pkalinnikov
Hi everyone, Can you take initial looks at this CL? rdevlin.cronin@, mpcomplete@: Please review the ...
1 year, 1 month ago (2016-11-16 18:18:15 UTC) #24
mmenke
https://codereview.chromium.org/2449913002/diff/80001/extensions/browser/api/web_request/web_request_event_details.cc File extensions/browser/api/web_request/web_request_event_details.cc (right): https://codereview.chromium.org/2449913002/diff/80001/extensions/browser/api/web_request/web_request_event_details.cc#newcode48 extensions/browser/api/web_request/web_request_event_details.cc:48: if (request->url().SchemeIsWSOrWSS()) { Why would WebRequestEventDetails be created for ...
1 year, 1 month ago (2016-11-16 18:20:39 UTC) #25
mmenke
https://codereview.chromium.org/2449913002/diff/80001/extensions/browser/api/web_request/web_request_event_details.cc File extensions/browser/api/web_request/web_request_event_details.cc (right): https://codereview.chromium.org/2449913002/diff/80001/extensions/browser/api/web_request/web_request_event_details.cc#newcode48 extensions/browser/api/web_request/web_request_event_details.cc:48: if (request->url().SchemeIsWSOrWSS()) { On 2016/11/16 18:20:39, mmenke wrote: > ...
1 year, 1 month ago (2016-11-16 18:21:28 UTC) #26
battre (please use the other)
https://codereview.chromium.org/2449913002/diff/80001/chrome/test/data/extensions/api_test/webrequest/test_websocket.js File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right): https://codereview.chromium.org/2449913002/diff/80001/chrome/test/data/extensions/api_test/webrequest/test_websocket.js#newcode40 chrome/test/data/extensions/api_test/webrequest/test_websocket.js:40: function webSocketEchoTestImpl(testBody) { I think that ultimately we want ...
1 year, 1 month ago (2016-11-16 21:51:58 UTC) #28
Devlin
A couple high-level comments here: - We'll want either a) more in-depth testing and to ...
1 year, 1 month ago (2016-11-16 22:00:56 UTC) #29
pmarks
onResponseStarted looks fine now: { "frameId": 0, "fromCache": false, "ip": "50.16.199.247", "method": "GET", "parentFrameId": -1, ...
1 year ago (2016-11-17 00:17:27 UTC) #30
pmarks
(I'm adding myself as a reviewer, because I keep getting dropped from the CC list.)
1 year ago (2016-11-17 00:19:23 UTC) #31
pkalinnikov
Hello, pmarks@: As tyoshino@ described above, onErrorOccurred is triggered instead of onCompleted, because current WS ...
1 year ago (2016-11-17 11:34:12 UTC) #32
mmenke
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, ...
1 year ago (2016-11-17 15:31:50 UTC) #34
tyoshino (SeeGerritForStatus)
Sorry for long delay. ps6 lgtm > tyoshino@: Takeshi, does your team work on crbug/663672 ...
1 year ago (2016-12-05 07:32:51 UTC) #35
tyoshino (SeeGerritForStatus)
Forgot to publish the draft comments. https://codereview.chromium.org/2449913002/diff/100001/chrome/test/data/extensions/api_test/webrequest/framework.js File chrome/test/data/extensions/api_test/webrequest/framework.js (right): https://codereview.chromium.org/2449913002/diff/100001/chrome/test/data/extensions/api_test/webrequest/framework.js#newcode210 chrome/test/data/extensions/api_test/webrequest/framework.js:210: // intoduced for ...
1 year ago (2016-12-06 08:05:06 UTC) #36
pkalinnikov
Hi, tyoshino@: Thanks for the comments, I have addressed them. Let's discuss possible solutions for ...
1 year ago (2016-12-13 21:15:59 UTC) #37
pkalinnikov
Hello everyone, With the CL that is about to land (https://codereview.chromium.org/2628333003) and the latest change ...
11 months ago (2017-01-16 12:31:36 UTC) #44
mmenke
On 2017/01/16 12:31:36, pkalinnikov wrote: > Hello everyone, > > With the CL that is ...
11 months ago (2017-01-17 15:54:29 UTC) #46
pmarks
On 2017/01/16 12:31:36, pkalinnikov wrote: > pmarks@: Feel free to try it out to make ...
11 months ago (2017-01-17 21:17:45 UTC) #47
pkalinnikov
On 2017/01/17 21:17:45, pmarks wrote: > On 2017/01/16 12:31:36, pkalinnikov wrote: > > pmarks@: Feel ...
11 months ago (2017-01-19 14:53:40 UTC) #48
pkalinnikov
rdevlin.cronin@: Hi Devlin. A friendly ping for you to review the CL once more. Your ...
11 months ago (2017-01-19 18:27:15 UTC) #51
Charlie Reis
On 2017/01/19 18:27:15, pkalinnikov wrote: > rdevlin.cronin@: Hi Devlin. A friendly ping for you to ...
11 months ago (2017-01-19 21:14:47 UTC) #56
pkalinnikov
Hello, One more friendly reminder to not overlook this CL. Thank you.
10 months, 2 weeks ago (2017-02-01 09:20:42 UTC) #57
mmenke
On 2017/02/01 09:20:42, pkalinnikov wrote: > Hello, > > One more friendly reminder to not ...
10 months, 2 weeks ago (2017-02-01 13:24:09 UTC) #58
pkalinnikov
On 2017/02/01 13:24:09, mmenke (Out Feb 4 to March 5) wrote: > On 2017/02/01 09:20:42, ...
10 months, 2 weeks ago (2017-02-01 14:02:10 UTC) #59
pkalinnikov
battre@: Dominic, I just realized that your review is needed, as you are the owner ...
10 months, 1 week ago (2017-02-07 14:30:19 UTC) #61
battre
How about updating https://developer.chrome.com/extensions/webRequest and explaining the tab ID for the WS scheme if we ...
10 months, 1 week ago (2017-02-07 15:03:02 UTC) #62
pkalinnikov
Addressed comments, thanks! One important case showed up during testing: WebSocket handshake shouldn't be redirected. ...
10 months, 1 week ago (2017-02-09 18:15:05 UTC) #63
Devlin
Very sorry for the extremely, extremely delayed review here. (If that happens in the future, ...
10 months ago (2017-02-14 01:21:39 UTC) #68
pkalinnikov
rdevlin.cronin@: Thanks for the review. There are couple of questions for you. https://codereview.chromium.org/2449913002/diff/200001/chrome/common/extensions/docs/templates/intros/webRequest.html File chrome/common/extensions/docs/templates/intros/webRequest.html ...
10 months ago (2017-02-14 13:49:51 UTC) #69
Devlin
(just responding to questions; will take another look later today) https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/extensions/api_test/webrequest/framework.js File chrome/test/data/extensions/api_test/webrequest/framework.js (right): https://codereview.chromium.org/2449913002/diff/200001/chrome/test/data/extensions/api_test/webrequest/framework.js#newcode215 ...
10 months ago (2017-02-14 15:55:24 UTC) #74
pkalinnikov
rdevlin.cronin@: Devlin, I have addressed your major concerns. Please review once again. Apart from that, ...
10 months ago (2017-02-15 19:15:00 UTC) #87
Devlin
This largely looks good, but I'd like to take another look once the resource type ...
10 months ago (2017-02-15 22:12:58 UTC) #90
pkalinnikov
Addressed. https://codereview.chromium.org/2449913002/diff/280001/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc File chrome/browser/extensions/api/web_request/web_request_api_unittest.cc (right): https://codereview.chromium.org/2449913002/diff/280001/chrome/browser/extensions/api/web_request/web_request_api_unittest.cc#newcode97 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: ...
10 months ago (2017-02-16 21:31:07 UTC) #92
pkalinnikov
rdevlin.cronin@: Devlin, I have covered all the concerns by now, and added more tests. Please ...
9 months, 4 weeks ago (2017-02-20 16:14:28 UTC) #98
Devlin
https://codereview.chromium.org/2449913002/diff/320001/chrome/common/extensions/docs/templates/intros/webRequest.html File chrome/common/extensions/docs/templates/intros/webRequest.html (right): https://codereview.chromium.org/2449913002/diff/320001/chrome/common/extensions/docs/templates/intros/webRequest.html#newcode148 chrome/common/extensions/docs/templates/intros/webRequest.html:148: <span class="availability">Starting from Chrome 58</span>, webRequest API nit: *the* ...
9 months, 4 weeks ago (2017-02-21 16:10:44 UTC) #101
pkalinnikov
Addressed, thank you. Are we close to submit this? https://codereview.chromium.org/2449913002/diff/320001/chrome/common/extensions/docs/templates/intros/webRequest.html File chrome/common/extensions/docs/templates/intros/webRequest.html (right): https://codereview.chromium.org/2449913002/diff/320001/chrome/common/extensions/docs/templates/intros/webRequest.html#newcode148 chrome/common/extensions/docs/templates/intros/webRequest.html:148: ...
9 months, 4 weeks ago (2017-02-21 19:18:16 UTC) #104
Devlin
lgtm. Thanks so much for your work and patience here! https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/extensions/api_test/webrequest/test_websocket.js File chrome/test/data/extensions/api_test/webrequest/test_websocket.js (right): https://codereview.chromium.org/2449913002/diff/320001/chrome/test/data/extensions/api_test/webrequest/test_websocket.js#newcode18 ...
9 months, 4 weeks ago (2017-02-21 20:34:11 UTC) #105
pkalinnikov
Thanks Devlin! Hopefully the "git cl format" change looks good to you as well. Everyone: ...
9 months, 3 weeks ago (2017-02-21 22:28:54 UTC) #111
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2449913002/380001
9 months, 3 weeks ago (2017-02-22 07:32:56 UTC) #116
commit-bot: I haz the power
9 months, 3 weeks ago (2017-02-22 11:12:41 UTC) #119
Message was sent while issue was closed.
Committed patchset #20 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/0f198df6bc8da6fe51b34f11b509...

Powered by Google App Engine
This is Rietveld 0eb02b776