Add a delay when unlocking WebSocket endpoints.

Add a delay when unlocking WebSocket endpoints.

The synchronous nature of endpoint unlocks for WebSocket throttling permits
denial-of-service attacks. Make it asynchronous, and add a small delay to
make attack timing harder.

BUG=442756
TEST=net_unittests, layout tests

Committed: https://crrev.com/bafe0f29a763c8325690cbdc42c29fc54d487d4f
Cr-Commit-Position: refs/heads/master@{#312774}

[Adam Rice, 2015-01-05 22:05:51]

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

[Adam Rice, 2015-01-20 00:15:59]

ping

[tyoshino (SeeGerritForStatus), 2015-01-20 07:22:07]

lgtm

https://codereview.chromium.org/835623003/diff/40001/net/socket/socket_test_u...
File net/socket/socket_test_util.h (right):

https://codereview.chromium.org/835623003/diff/40001/net/socket/socket_test_u...
net/socket/socket_test_util.h:1325: class WebSocketEndpointZeroUnlockDelay {
[optional] add a prefix "Scoped"?

[Adam Rice, 2015-01-20 07:33:54]

https://codereview.chromium.org/835623003/diff/40001/net/socket/socket_test_u...
File net/socket/socket_test_util.h (right):

https://codereview.chromium.org/835623003/diff/40001/net/socket/socket_test_u...
net/socket/socket_test_util.h:1325: class WebSocketEndpointZeroUnlockDelay {
On 2015/01/20 07:22:07, tyoshino wrote:
> [optional] add a prefix "Scoped"?

Done.

[tyoshino (SeeGerritForStatus), 2015-01-20 07:34:54]

lgtm

[Adam Rice, 2015-01-20 07:36:48]

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

[Adam Rice, 2015-01-20 07:36:49]

+mmenke for net/ OWNERS.

[mmenke, 2015-01-20 15:50:53]

https://codereview.chromium.org/835623003/diff/60001/net/socket/socket_test_u...
File net/socket/socket_test_util.cc (right):

https://codereview.chromium.org/835623003/diff/60001/net/socket/socket_test_u...
net/socket/socket_test_util.cc:1997: old_delay_);
Suggest adding protection against having two of these at once (Or two of these
whose lifetimes aren't nested).

Maybe CHECK/DCHECK/EXPECT that the returned value is base::TimeDelta()?

https://codereview.chromium.org/835623003/diff/60001/net/socket/websocket_end...
File net/socket/websocket_endpoint_lock_manager.h (right):

https://codereview.chromium.org/835623003/diff/60001/net/socket/websocket_end...
net/socket/websocket_endpoint_lock_manager.h:23: class NET_EXPORT_PRIVATE
WebSocketEndpointLockManager {
Mind adding some class level documentation?  I've never heard of this class
before.

https://codereview.chromium.org/835623003/diff/60001/net/socket/websocket_end...
net/socket/websocket_endpoint_lock_manager.h:33: static
WebSocketEndpointLockManager* GetInstance();
Unrelated to this CL, but should this really be a singleton?  Seems weird to be
leaking that information between profiles - don't see leaking that from
incognito as a big issue, but seems a little weird.

Also creates an implicit requirement that all URLRequestContexts that use
WebSockets live on a single thread, or random crashes may ensue.

[mmenke, 2015-01-20 17:46:21]

https://codereview.chromium.org/835623003/diff/60001/net/socket/websocket_end...
File net/socket/websocket_endpoint_lock_manager.h (right):

https://codereview.chromium.org/835623003/diff/60001/net/socket/websocket_end...
net/socket/websocket_endpoint_lock_manager.h:33: static
WebSocketEndpointLockManager* GetInstance();
On 2015/01/20 15:50:53, mmenke wrote:
> Unrelated to this CL, but should this really be a singleton?  Seems weird to
be
> leaking that information between profiles - don't see leaking that from
> incognito as a big issue, but seems a little weird.
> 
> Also creates an implicit requirement that all URLRequestContexts that use
> WebSockets live on a single thread, or random crashes may ensue.

And to elaborte:  This is the case with Chrome, it's not currently the case with
Cronet, where every context has its own thread.  We may want to switch our
threading model at some point, but the reason shouldn't be because of random
non-threadsafe globals peppered throughout the code.

[Adam Rice, 2015-01-21 08:54:02]

https://codereview.chromium.org/835623003/diff/60001/net/socket/socket_test_u...
File net/socket/socket_test_util.cc (right):

https://codereview.chromium.org/835623003/diff/60001/net/socket/socket_test_u...
net/socket/socket_test_util.cc:1997: old_delay_);
On 2015/01/20 15:50:53, mmenke wrote:
> Suggest adding protection against having two of these at once (Or two of these
> whose lifetimes aren't nested).
> 
> Maybe CHECK/DCHECK/EXPECT that the returned value is base::TimeDelta()?

Done.

https://codereview.chromium.org/835623003/diff/60001/net/socket/websocket_end...
File net/socket/websocket_endpoint_lock_manager.h (right):

https://codereview.chromium.org/835623003/diff/60001/net/socket/websocket_end...
net/socket/websocket_endpoint_lock_manager.h:23: class NET_EXPORT_PRIVATE
WebSocketEndpointLockManager {
On 2015/01/20 15:50:53, mmenke wrote:
> Mind adding some class level documentation?  I've never heard of this class
> before.

Wow. Not only did I fail to document it here, I only mentioned it in passing in
the design doc.

I put in a comment of what it does. I'm not sure whether the implementation is
sufficiently interesting to justify adding notes about how it works.

https://codereview.chromium.org/835623003/diff/60001/net/socket/websocket_end...
net/socket/websocket_endpoint_lock_manager.h:33: static
WebSocketEndpointLockManager* GetInstance();
On 2015/01/20 15:50:53, mmenke wrote:
> Unrelated to this CL, but should this really be a singleton?  Seems weird to
be
> leaking that information between profiles - don't see leaking that from
> incognito as a big issue, but seems a little weird.
> 
> Also creates an implicit requirement that all URLRequestContexts that use
> WebSockets live on a single thread, or random crashes may ensue.

Originally I didn't know what its lifetime should be or how to access it from
net/websockets. Now I'm pretty sure it belongs in HttpNetworkSession and can be
accessed via the URLRequestContext.

I have filed http://crbug.com/450518 to fix this.

[mmenke, 2015-01-21 15:20:34]

Thanks for making the changes!  LGTM, deferring to tyoshino on the correctness
of the code.

On 2015/01/21 08:54:02, Adam Rice wrote:
>
https://codereview.chromium.org/835623003/diff/60001/net/socket/socket_test_u...
> File net/socket/socket_test_util.cc (right):
> 
>
https://codereview.chromium.org/835623003/diff/60001/net/socket/socket_test_u...
> net/socket/socket_test_util.cc:1997: old_delay_);
> On 2015/01/20 15:50:53, mmenke wrote:
> > Suggest adding protection against having two of these at once (Or two of
these
> > whose lifetimes aren't nested).
> > 
> > Maybe CHECK/DCHECK/EXPECT that the returned value is base::TimeDelta()?
> 
> Done.
> 
>
https://codereview.chromium.org/835623003/diff/60001/net/socket/websocket_end...
> File net/socket/websocket_endpoint_lock_manager.h (right):
> 
>
https://codereview.chromium.org/835623003/diff/60001/net/socket/websocket_end...
> net/socket/websocket_endpoint_lock_manager.h:23: class NET_EXPORT_PRIVATE
> WebSocketEndpointLockManager {
> On 2015/01/20 15:50:53, mmenke wrote:
> > Mind adding some class level documentation?  I've never heard of this class
> > before.
> 
> Wow. Not only did I fail to document it here, I only mentioned it in passing
in
> the design doc.
> 
> I put in a comment of what it does. I'm not sure whether the implementation is
> sufficiently interesting to justify adding notes about how it works.
> 
>
https://codereview.chromium.org/835623003/diff/60001/net/socket/websocket_end...
> net/socket/websocket_endpoint_lock_manager.h:33: static
> WebSocketEndpointLockManager* GetInstance();
> On 2015/01/20 15:50:53, mmenke wrote:
> > Unrelated to this CL, but should this really be a singleton?  Seems weird to
> be
> > leaking that information between profiles - don't see leaking that from
> > incognito as a big issue, but seems a little weird.
> > 
> > Also creates an implicit requirement that all URLRequestContexts that use
> > WebSockets live on a single thread, or random crashes may ensue.
> 
> Originally I didn't know what its lifetime should be or how to access it from
> net/websockets. Now I'm pretty sure it belongs in HttpNetworkSession and can
be
> accessed via the URLRequestContext.
> 
> I have filed http://crbug.com/450518 to fix this.

[Adam Rice, 2015-01-22 02:02:19]

tyoshino, can you take a quick look at patch set 5?

[tyoshino (SeeGerritForStatus), 2015-01-22 12:57:44]

On 2015/01/22 02:02:19, Adam Rice wrote:
> tyoshino, can you take a quick look at patch set 5?

ps5 lgtm!

[Adam Rice, 2015-01-23 03:11:50]

The CQ bit was checked by ricea@chromium.org

[commit-bot: I haz the power, 2015-01-23 03:13:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/835623003/80001

[commit-bot: I haz the power, 2015-01-23 05:32:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-23 05:32:45]

Patchset 5 (id:??) landed as
https://crrev.com/bafe0f29a763c8325690cbdc42c29fc54d487d4f
Cr-Commit-Position: refs/heads/master@{#312774}