SafeBrowsing support for WebSocket (post-network-servicification)

SafeBrowsing support for WebSocket (post-network-servicification)

Add WebsocketSBHandshakeThrottle, an implementation of
blink::WebSocketHandshakeThrottle that performs safe browsing checks.

WebSocket handshakes are performed in parallel with the SafeBrowsing
check. The result of the handshake will not be exposed to Javascript before
the SafeBrowsing check passes. While an interstitial is displayed the
WebSocket connection will remain in this pending state. If the connection is
blocked then the WebSocket connection will be closed. Javascript will be
notified of a connection error (without a specific reason) and the fact that
the connection was blocked by SafeBrowsing will be logged to the console.

This implementation requires the network service to be enabled. This means
passing the --enable-network-service flag to chrome and browser_tests. The
unit tests (which are compiled into the unit_tests binary) do not require
the flag.

See the design doc at
https://docs.google.com/document/d/1iR3XMIQukqlXb6ajIHE91apHZAxyF_wvRoB5JGeJYPs/edit#heading=h.1r0zq3g29n7m

BUG=644744
Review-Url: https://codereview.chromium.org/2952583002
Cr-Commit-Position: refs/heads/master@{#482897}
Committed: https://chromium.googlesource.com/chromium/src/+/4ce57e66544e783efe1ae05548c2e00e0fd0ebed

[Adam Rice, 2017-06-20 13:12:35]

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

[commit-bot: I haz the power, 2017-06-20 13:12:50]

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-06-20 13:32:57]

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

[commit-bot: I haz the power, 2017-06-20 13:32:58]

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

[Adam Rice, 2017-06-20 13:46:33]

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

[commit-bot: I haz the power, 2017-06-20 13:46:41]

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-06-20 14:30:40]

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

[commit-bot: I haz the power, 2017-06-20 14:30:40]

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

[Adam Rice, 2017-06-20 15:10:36]

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

[commit-bot: I haz the power, 2017-06-20 15:10:49]

Dry run: CQ is trying da patch.

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

[Adam Rice, 2017-06-20 15:12:43]

ricea@chromium.org changed reviewers:
+ yhirano@chromium.org, yzshen@chromium.org

[Adam Rice, 2017-06-20 15:12:44]

+yhirano for WebSockets
+yzshen for mojo SafeBrowsing

[commit-bot: I haz the power, 2017-06-20 15:46:03]

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

[commit-bot: I haz the power, 2017-06-20 15:46:04]

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

[yzshen1, 2017-06-20 19:52:18]

yzshen@chromium.org changed reviewers:
+ vakh@chromium.org

[yzshen1, 2017-06-20 19:52:19]

+Varun for SafeBrowsing-related change.

Mostly looks good, just a few nits.

https://codereview.chromium.org/2952583002/diff/40001/components/safe_browsin...
File components/safe_browsing/renderer/websocket_sb_handshake_throttle.cc
(right):

https://codereview.chromium.org/2952583002/diff/40001/components/safe_browsin...
components/safe_browsing/renderer/websocket_sb_handshake_throttle.cc:37:
content::RenderThread::Get()->GetConnector()->BindInterface(
You could consider passing in a (not-owned) SafeBrowsing interface pointer, and
let all the WSSBHT instances use the same SafeBrowing interface pointer. That is
what RendererURLLoaderThrottle does.

Creating a SafeBrowsing interface pointer requires an IPC message to the browser
and creating the corresponding impl object. It is nice to avoid this cost, it is
much bigger than the string lookup above. :)

https://codereview.chromium.org/2952583002/diff/40001/components/safe_browsin...
File
components/safe_browsing/renderer/websocket_sb_handshake_throttle_unittest.cc
(right):

https://codereview.chromium.org/2952583002/diff/40001/components/safe_browsin...
components/safe_browsing/renderer/websocket_sb_handshake_throttle_unittest.cc:94:
FakeSafeBrowsing* safe_browsing_;
There seems no need to use a raw pointer and leak. You could use a
FakeSafeBrowsing instance instead, |mojo_binding_| doesn't take ownership.

https://codereview.chromium.org/2952583002/diff/40001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/2952583002/diff/40001/content/public/renderer...
content/public/renderer/content_renderer_client.h:181: virtual
std::unique_ptr<blink::WebSocketHandshakeThrottle>
Out of curiosity: why inserting the method in the middle instead of appending?

[vakh (use Gerrit instead), 2017-06-20 21:40:28]

lgtm for components/safe_browsing/...

https://codereview.chromium.org/2952583002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/2952583002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1481:
IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, MalwareWebSocketBlocked) {
Please add the corresponding tests for V4SafeBrowsingServiceTest class also.
We're going to deprecate SafeBrowsingServiceTest class very very soon.

[Adam Rice, 2017-06-21 06:06:35]

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

[commit-bot: I haz the power, 2017-06-21 06:06:48]

Dry run: CQ is trying da patch.

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

[Adam Rice, 2017-06-21 06:08:36]

https://codereview.chromium.org/2952583002/diff/40001/components/safe_browsin...
File components/safe_browsing/renderer/websocket_sb_handshake_throttle.cc
(right):

https://codereview.chromium.org/2952583002/diff/40001/components/safe_browsin...
components/safe_browsing/renderer/websocket_sb_handshake_throttle.cc:37:
content::RenderThread::Get()->GetConnector()->BindInterface(
On 2017/06/20 19:52:19, yzshen1 wrote:
> You could consider passing in a (not-owned) SafeBrowsing interface pointer,
and
> let all the WSSBHT instances use the same SafeBrowing interface pointer. That
is
> what RendererURLLoaderThrottle does.

Done. I realised I can share the same interface pointer with
RendererURLLoaderThrottle in //chrome for added efficiency. I've kept the
initialisation logic inside MaybeCreate() so that I don't need to duplicate it
for WebView.

https://codereview.chromium.org/2952583002/diff/40001/components/safe_browsin...
File
components/safe_browsing/renderer/websocket_sb_handshake_throttle_unittest.cc
(right):

https://codereview.chromium.org/2952583002/diff/40001/components/safe_browsin...
components/safe_browsing/renderer/websocket_sb_handshake_throttle_unittest.cc:94:
FakeSafeBrowsing* safe_browsing_;
On 2017/06/20 19:52:19, yzshen1 wrote:
> There seems no need to use a raw pointer and leak. You could use a
> FakeSafeBrowsing instance instead, |mojo_binding_| doesn't take ownership.

Thanks! I was confused about the ownership semantics.

https://codereview.chromium.org/2952583002/diff/40001/content/public/renderer...
File content/public/renderer/content_renderer_client.h (right):

https://codereview.chromium.org/2952583002/diff/40001/content/public/renderer...
content/public/renderer/content_renderer_client.h:181: virtual
std::unique_ptr<blink::WebSocketHandshakeThrottle>
On 2017/06/20 19:52:19, yzshen1 wrote:
> Out of curiosity: why inserting the method in the middle instead of appending?

I'm attempting to imitate the order from blink::Profile. Whether this is
actually a good idea or not I don't know.

blink::Profile methods are grouped by the thing they pertain to and then ordered
by the name of that thing. So WebS_o_cket comes before WebS_p_eechSynthesizer.

[kinuko, 2017-06-21 06:42:41]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2017-06-21 06:42:42]

https://codereview.chromium.org/2952583002/diff/60001/components/safe_browsin...
File components/safe_browsing/renderer/websocket_sb_handshake_throttle.cc
(right):

https://codereview.chromium.org/2952583002/diff/60001/components/safe_browsin...
components/safe_browsing/renderer/websocket_sb_handshake_throttle.cc:48:
safe_browsing_(std::move(safe_browsing)),
nit: this std::move seems no-op

https://codereview.chromium.org/2952583002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebSocketHandshakeThrottle.h (left):

https://codereview.chromium.org/2952583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebSocketHandshakeThrottle.h:18: #include
"public/platform/WebCallbacks.h"
I think writing this way was preferable (while not all the code does that yet),
did it make any builds fail?

[Adam Rice, 2017-06-21 07:43:20]

https://codereview.chromium.org/2952583002/diff/60001/components/safe_browsin...
File components/safe_browsing/renderer/websocket_sb_handshake_throttle.cc
(right):

https://codereview.chromium.org/2952583002/diff/60001/components/safe_browsin...
components/safe_browsing/renderer/websocket_sb_handshake_throttle.cc:48:
safe_browsing_(std::move(safe_browsing)),
On 2017/06/21 06:42:41, kinuko wrote:
> nit: this std::move seems no-op

Thanks, removed.

https://codereview.chromium.org/2952583002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebSocketHandshakeThrottle.h (left):

https://codereview.chromium.org/2952583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebSocketHandshakeThrottle.h:18: #include
"public/platform/WebCallbacks.h"
On 2017/06/21 06:42:42, kinuko wrote:
> I think writing this way was preferable (while not all the code does that
yet),
> did it make any builds fail?

Yes. websocket_sb_handshake_throttle.h in //components wouldn't compile because
it doesn't have WebKit in the include path. I couldn't find another way to fix
it.

[kinuko, 2017-06-21 08:59:35]

https://codereview.chromium.org/2952583002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebSocketHandshakeThrottle.h (left):

https://codereview.chromium.org/2952583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebSocketHandshakeThrottle.h:18: #include
"public/platform/WebCallbacks.h"
On 2017/06/21 07:43:20, Adam Rice wrote:
> On 2017/06/21 06:42:42, kinuko wrote:
> > I think writing this way was preferable (while not all the code does that
> yet),
> > did it make any builds fail?
> 
> Yes. websocket_sb_handshake_throttle.h in //components wouldn't compile
because
> it doesn't have WebKit in the include path. I couldn't find another way to fix
> it.

Adding //third_party/WebKit/public:blink_header to the deps of the target
usually helps, could that work?

[Adam Rice, 2017-06-21 09:18:35]

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

[Adam Rice, 2017-06-21 09:18:47]

https://codereview.chromium.org/2952583002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/2952583002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1481:
IN_PROC_BROWSER_TEST_F(SafeBrowsingServiceTest, MalwareWebSocketBlocked) {
On 2017/06/20 21:40:27, vakh (use Gerrit instead) wrote:
> Please add the corresponding tests for V4SafeBrowsingServiceTest class also.
> We're going to deprecate SafeBrowsingServiceTest class very very soon.

Done. They're almost exact copies, but if the old tests are going to be deleted
I don't think that matters.

https://codereview.chromium.org/2952583002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebSocketHandshakeThrottle.h (left):

https://codereview.chromium.org/2952583002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebSocketHandshakeThrottle.h:18: #include
"public/platform/WebCallbacks.h"
On 2017/06/21 08:59:35, kinuko wrote:
> On 2017/06/21 07:43:20, Adam Rice wrote:
> > On 2017/06/21 06:42:42, kinuko wrote:
> > > I think writing this way was preferable (while not all the code does that
> > yet),
> > > did it make any builds fail?
> > 
> > Yes. websocket_sb_handshake_throttle.h in //components wouldn't compile
> because
> > it doesn't have WebKit in the include path. I couldn't find another way to
fix
> > it.
> 
> Adding //third_party/WebKit/public:blink_header to the deps of the target
> usually helps, could that work?

I see. It must have got better when I fixed the DEPS file. I've reverted the
change
to this file now.

[commit-bot: I haz the power, 2017-06-21 09:18:59]

Dry run: CQ is trying da patch.

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

[yhirano, 2017-06-21 09:38:38]

lgtm

https://codereview.chromium.org/2952583002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/2952583002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1483: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
I prefer

if (!HasSwitch())
  return;

[commit-bot: I haz the power, 2017-06-21 10:31:29]

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

[commit-bot: I haz the power, 2017-06-21 10:31:30]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[kinuko, 2017-06-21 13:19:24]

> I see. It must have got better when I fixed the DEPS file. I've reverted the
> change
> to this file now.

Thanks! (lgtm, fwiw)

[yzshen1, 2017-06-21 16:55:06]

https://codereview.chromium.org/2952583002/diff/40001/components/safe_browsin...
File components/safe_browsing/renderer/websocket_sb_handshake_throttle.cc
(right):

https://codereview.chromium.org/2952583002/diff/40001/components/safe_browsin...
components/safe_browsing/renderer/websocket_sb_handshake_throttle.cc:37:
content::RenderThread::Get()->GetConnector()->BindInterface(
On 2017/06/21 06:08:35, Adam Rice wrote:
> On 2017/06/20 19:52:19, yzshen1 wrote:
> > You could consider passing in a (not-owned) SafeBrowsing interface pointer,
> and
> > let all the WSSBHT instances use the same SafeBrowing interface pointer.
That
> is
> > what RendererURLLoaderThrottle does.
> 
> Done. I realised I can share the same interface pointer with
> RendererURLLoaderThrottle in //chrome for added efficiency. I've kept the
> initialisation logic inside MaybeCreate() so that I don't need to duplicate it
> for WebView.
> 

I think it may make more sense to initialize safe_browsing_service in
*ContentRendererClient: it is shared with RendererURLLoaderThrottle. If we do
lazy-init here, we need to duplicate it in RendererURLLoaderThrottle as well.
It feel more natural to let *ContentRendererClient do the work.

https://codereview.chromium.org/2952583002/diff/120001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/2952583002/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:2253: // when
the old database backend stops being used.
optional: You could use parameterized tests.

[Adam Rice, 2017-06-22 12:29:31]

https://codereview.chromium.org/2952583002/diff/120001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/2952583002/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:2253: // when
the old database backend stops being used.
On 2017/06/21 16:55:06, yzshen1 wrote:
> optional: You could use parameterized tests.

I thought you couldn't parameterize on test framework?

Anyway, if the old tests are going to be deleted soon there's no point in
spending a lot of energy on it.

[Adam Rice, 2017-06-22 12:35:04]

https://codereview.chromium.org/2952583002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc (right):

https://codereview.chromium.org/2952583002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc:1483: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2017/06/21 09:38:38, yhirano wrote:
> I prefer
> 
> if (!HasSwitch())
>   return;

Done.

[Adam Rice, 2017-06-22 13:42:44]

https://codereview.chromium.org/2952583002/diff/40001/components/safe_browsin...
File components/safe_browsing/renderer/websocket_sb_handshake_throttle.cc
(right):

https://codereview.chromium.org/2952583002/diff/40001/components/safe_browsin...
components/safe_browsing/renderer/websocket_sb_handshake_throttle.cc:37:
content::RenderThread::Get()->GetConnector()->BindInterface(
On 2017/06/21 16:55:06, yzshen1 wrote:
> I think it may make more sense to initialize safe_browsing_service in
> *ContentRendererClient: it is shared with RendererURLLoaderThrottle. If we do
> lazy-init here, we need to duplicate it in RendererURLLoaderThrottle as well.
> It feel more natural to let *ContentRendererClient do the work.

Done. I factored the shared code out into a new method. We may need to mess it
up again later if we enable the HTTP and WebSocket implementations at different
times.

[Adam Rice, 2017-06-22 13:43:48]

ricea@chromium.org changed reviewers:
+ jam@chromium.org

[Adam Rice, 2017-06-22 13:43:49]

+jam for OWNERS review for the added targets in
components/safe_browsing/renderer/DEPS

[yzshen1, 2017-06-22 15:11:07]

lgtm

[Adam Rice, 2017-06-22 15:12:00]

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

[commit-bot: I haz the power, 2017-06-22 15:12:14]

Dry run: CQ is trying da patch.

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

[Adam Rice, 2017-06-22 15:17:05]

jam, can you also review //android_webview and //chrome/renderer, or would you
like a specific reviewer for those areas?

[commit-bot: I haz the power, 2017-06-22 15:42:50]

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

[commit-bot: I haz the power, 2017-06-22 15:42:51]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jam, 2017-06-22 17:30:01]

On 2017/06/22 15:17:05, Adam Rice wrote:
> jam, can you also review //android_webview and //chrome/renderer, or would you
> like a specific reviewer for those areas?

my only comment is: why block this on network service? i.e. we can turn on the
safe browsing mojo service always, so that websockets can use it independent of
network service.

[yzshen1, 2017-06-22 17:48:35]

On 2017/06/22 17:30:01, jam wrote:
> On 2017/06/22 15:17:05, Adam Rice wrote:
> > jam, can you also review //android_webview and //chrome/renderer, or would
you
> > like a specific reviewer for those areas?
> 
> my only comment is: why block this on network service? i.e. we can turn on the
> safe browsing mojo service always, so that websockets can use it independent
of
> network service.

+1, yeah I don't think there is anything preventing us from doing that.

[Adam Rice, 2017-06-23 10:21:57]

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

[commit-bot: I haz the power, 2017-06-23 10:22:10]

Dry run: CQ is trying da patch.

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

[Adam Rice, 2017-06-23 11:07:31]

message: On 2017/06/22 17:30:01, jam wrote:
> my only comment is: why block this on network service? i.e. we can turn on the
> safe browsing mojo service always, so that websockets can use it independent
of
> network service.

Yes, there's no need to block on network servicification. The reason the
renderer-
side is behind the network service flag is that the browser-side mojo service is
currently only enabled in that case.

The title of the change (post-network-servicification) is just to distinguish it
from the pre-network-servicification design, which it looks like we're not going
to need.

[commit-bot: I haz the power, 2017-06-23 11:50:17]

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

[commit-bot: I haz the power, 2017-06-23 11:50:17]

Dry run: This issue passed the CQ dry run.

[jam, 2017-06-23 16:40:40]

On 2017/06/23 11:07:31, Adam Rice wrote:
> message: On 2017/06/22 17:30:01, jam wrote:
> > my only comment is: why block this on network service? i.e. we can turn on
the
> > safe browsing mojo service always, so that websockets can use it independent
> of
> > network service.
> 
> Yes, there's no need to block on network servicification. The reason the
> renderer-
> side is behind the network service flag is that the browser-side mojo service
is
> currently only enabled in that case.

Can this cl then enable the service always and then it doesn't need the check?

> 
> The title of the change (post-network-servicification) is just to distinguish
it
> from the pre-network-servicification design, which it looks like we're not
going
> to need.

[sgurun-gerrit only, 2017-06-24 01:35:31]

sgurun@chromium.org changed reviewers:
+ ntfschr@chromium.org, sgurun@chromium.org

[sgurun-gerrit only, 2017-06-24 01:35:32]

Nate, FYI

I will take a look at it monday for WebView side.

[Adam Rice, 2017-06-26 02:56:47]

On 2017/06/23 16:40:40, jam wrote:
> Can this cl then enable the service always and then it doesn't need the check?

Not yet. This CL hooks up the android_webview on the renderer side, but the
browser-side code doesn't exist yet. It's trivial to turn it on once the
browser-
side is ready.

[Adam Rice, 2017-06-27 01:01:17]

jam: ping for DEPS and //chrome/renderer

[jam, 2017-06-27 01:36:28]

On 2017/06/26 02:56:47, Adam Rice wrote:
> On 2017/06/23 16:40:40, jam wrote:
> > Can this cl then enable the service always and then it doesn't need the
check?
> 
> Not yet. This CL hooks up the android_webview on the renderer side, but the
> browser-side code doesn't exist yet. It's trivial to turn it on once the
> browser-
> side is ready.

I'm not sure we're talking about the same thing?

i.e. this cl is hooking up webview and chrome.  However it checks that network
service is enabled. What I meant was can we decouple websockets using safe
browsing from the network service? i.e. always enable the safe browsing service.
if you need to temporarily not do SB checks for websocket, then add an if
(false) statement in the code that adds the throttle until the browser side is
ready. does that clear up what I meant? if not please IM me so we can chat high
bandwidth :)

https://codereview.chromium.org/2952583002/diff/180001/android_webview/render...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/2952583002/diff/180001/android_webview/render...
android_webview/renderer/aw_content_renderer_client.cc:344:
DCHECK(safe_browsing_);
ditto

https://codereview.chromium.org/2952583002/diff/180001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2952583002/diff/180001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1617: DCHECK(safe_browsing_);
BindInterface completes asynchronously, but it'll always have one end of a
message pipe here so the DCHECK doesn't do anything.

[jam, 2017-06-27 01:47:31]

lgtm with removing the DCHECKs

(Adam and I chatted; he thought there isn't a point in adding a new flag to
disable this while it's in progress, and just piggybacking on the
disabled-by-default NS flag)

[Adam Rice, 2017-06-27 02:16:57]

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

[Adam Rice, 2017-06-27 02:17:06]

https://codereview.chromium.org/2952583002/diff/180001/android_webview/render...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/2952583002/diff/180001/android_webview/render...
android_webview/renderer/aw_content_renderer_client.cc:344:
DCHECK(safe_browsing_);
On 2017/06/27 01:36:28, jam wrote:
> ditto

Done.

https://codereview.chromium.org/2952583002/diff/180001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2952583002/diff/180001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1617: DCHECK(safe_browsing_);
On 2017/06/27 01:36:28, jam wrote:
> BindInterface completes asynchronously, but it'll always have one end of a
> message pipe here so the DCHECK doesn't do anything.

Removed.

[commit-bot: I haz the power, 2017-06-27 02:17:11]

Dry run: CQ is trying da patch.

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

[Adam Rice, 2017-06-27 02:17:40]

sgurun, have you had a chance to look at this?

[commit-bot: I haz the power, 2017-06-27 03:33:06]

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

[commit-bot: I haz the power, 2017-06-27 03:33:08]

Dry run: This issue passed the CQ dry run.

[sgurun-gerrit only, 2017-06-27 21:05:35]

On 2017/06/27 02:17:40, Adam Rice wrote:
> sgurun, have you had a chance to look at this?

looking now.

[sgurun-gerrit only, 2017-06-27 22:01:57]

On 2017/06/27 21:05:35, sgurun wrote:
> On 2017/06/27 02:17:40, Adam Rice wrote:
> > sgurun, have you had a chance to look at this?
> 
> looking now.

lgtm

[Adam Rice, 2017-06-28 04:09:44]

The CQ bit was checked by ricea@chromium.org

[Adam Rice, 2017-06-28 04:09:44]

The patchset sent to the CQ was uploaded after l-g-t-m from vakh@chromium.org,
kinuko@chromium.org, yhirano@chromium.org, yzshen@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2952583002/#ps200001
(title: "Remove useless DCHECKs")

[commit-bot: I haz the power, 2017-06-28 04:09:54]

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-06-28 05:53:30]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1498622984071880,
"parent_rev": "0cc18dd63714b71b4bb8860d966ff56fedd56afc", "commit_rev":
"4ce57e66544e783efe1ae05548c2e00e0fd0ebed"}

[commit-bot: I haz the power, 2017-06-28 05:53:44]

Description was changed from

==========
SafeBrowsing support for WebSocket (post-network-servicification)

Add WebsocketSBHandshakeThrottle, an implementation of
blink::WebSocketHandshakeThrottle that performs safe browsing checks.

WebSocket handshakes are performed in parallel with the SafeBrowsing
check. The result of the handshake will not be exposed to Javascript before
the SafeBrowsing check passes. While an interstitial is displayed the
WebSocket connection will remain in this pending state. If the connection is
blocked then the WebSocket connection will be closed. Javascript will be
notified of a connection error (without a specific reason) and the fact that
the connection was blocked by SafeBrowsing will be logged to the console.

This implementation requires the network service to be enabled. This means
passing the --enable-network-service flag to chrome and browser_tests. The
unit tests (which are compiled into the unit_tests binary) do not require
the flag.

See the design doc at
https://docs.google.com/document/d/1iR3XMIQukqlXb6ajIHE91apHZAxyF_wvRoB5JGeJY...

BUG=644744
==========

to

==========
SafeBrowsing support for WebSocket (post-network-servicification)

Add WebsocketSBHandshakeThrottle, an implementation of
blink::WebSocketHandshakeThrottle that performs safe browsing checks.

WebSocket handshakes are performed in parallel with the SafeBrowsing
check. The result of the handshake will not be exposed to Javascript before
the SafeBrowsing check passes. While an interstitial is displayed the
WebSocket connection will remain in this pending state. If the connection is
blocked then the WebSocket connection will be closed. Javascript will be
notified of a connection error (without a specific reason) and the fact that
the connection was blocked by SafeBrowsing will be logged to the console.

This implementation requires the network service to be enabled. This means
passing the --enable-network-service flag to chrome and browser_tests. The
unit tests (which are compiled into the unit_tests binary) do not require
the flag.

See the design doc at
https://docs.google.com/document/d/1iR3XMIQukqlXb6ajIHE91apHZAxyF_wvRoB5JGeJY...

BUG=644744

Review-Url: https://codereview.chromium.org/2952583002
Cr-Commit-Position: refs/heads/master@{#482897}
Committed:
https://chromium.googlesource.com/chromium/src/+/4ce57e66544e783efe1ae05548c2...
==========

[commit-bot: I haz the power, 2017-06-28 05:53:46]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/4ce57e66544e783efe1ae05548c2...