Adds a --deterministic-fetch flag to headless_shell

Adds a --deterministic-fetch flag to headless_shell

Adds a -deterministic-fetch flag to headless_shell which causes network
reuests to complete in the same order they're created in.  This removes
a significant source of network related non-determinism at the cost of
slower page loads.

BUG=546953
Committed: https://crrev.com/ad54054fcdee1ede7d009413d3577cea0f0e08f6
Cr-Commit-Position: refs/heads/master@{#420657}

[alex clarke (OOO till 29th), 2016-09-19 14:53:47]

alexclarke@chromium.org changed reviewers:
+ skyostil@chromium.org

[alex clarke (OOO till 29th), 2016-09-19 14:53:53]

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

[commit-bot: I haz the power, 2016-09-19 14:54:05]

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

[alex clarke (OOO till 29th), 2016-09-19 15:01:05]

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

[commit-bot: I haz the power, 2016-09-19 15:01:42]

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, 2016-09-19 15:20:02]

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

[commit-bot: I haz the power, 2016-09-19 15:20:03]

Dry run: This issue passed the CQ dry run.

[Sami, 2016-09-22 12:09:40]

This will be really useful to have!

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/de...
File headless/public/util/deterministic_http_protocol_handler.cc (right):

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/de...
headless/public/util/deterministic_http_protocol_handler.cc:39: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/de...
headless/public/util/deterministic_http_protocol_handler.cc:52:
std::unique_ptr<net::URLRequestContext> url_request_context(
This is a pretty big object. Could we construct it once and have all requests
share it like we do normally?

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/de...
headless/public/util/deterministic_http_protocol_handler.cc:55:
url_request_context->set_http_transaction_factory(
Should we use CopyFrom instead? Otherwise the context can miss a lot of subtle
things.

https://cs.chromium.org/chromium/src/net/url_request/url_request_context.h?l=...

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/de...
File headless/public/util/deterministic_http_protocol_handler.h (right):

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/de...
headless/public/util/deterministic_http_protocol_handler.h:37: void
set_browser_context(HeadlessBrowserContext* browser_context) {
I wonder if we could avoid the dependency on the browser context by calling
URLRequestJobManager::GetInstance()->CreateJob() from MaybeCreateJob() (and
making sure to not do that re-entrantly)?

Alternatively should we structure this so that it uses another
net::URLRequestJobFactory::ProtocolHandler to implement fetching so that you
could hook this up to your custom protocol handler too?

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/ht...
File headless/public/util/http_url_fetcher.cc (right):

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/ht...
headless/public/util/http_url_fetcher.cc:70: request_->set_method("GET");
Could we add a DCHECK for this somewhere? I think silently turning POSTs into
GETs will be surprising to say the least.

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/ht...
headless/public/util/http_url_fetcher.cc:97: LOG(WARNING) << "SSL certificate
error when fetching PAC script, aborting.";
I'm not sure how this is related to PAC scripts?

[alex clarke (OOO till 29th), 2016-09-23 13:29:13]

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

[alex clarke (OOO till 29th), 2016-09-23 13:29:22]

PTAL

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/de...
File headless/public/util/deterministic_http_protocol_handler.cc (right):

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/de...
headless/public/util/deterministic_http_protocol_handler.cc:39: };
On 2016/09/22 12:09:40, Sami wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/de...
headless/public/util/deterministic_http_protocol_handler.cc:52:
std::unique_ptr<net::URLRequestContext> url_request_context(
On 2016/09/22 12:09:40, Sami wrote:
> This is a pretty big object. Could we construct it once and have all requests
> share it like we do normally?

It's shared at the ProtocolHandler level now.

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/de...
headless/public/util/deterministic_http_protocol_handler.cc:55:
url_request_context->set_http_transaction_factory(
On 2016/09/22 12:09:40, Sami wrote:
> Should we use CopyFrom instead? Otherwise the context can miss a lot of subtle
> things.
> 
>
https://cs.chromium.org/chromium/src/net/url_request/url_request_context.h?l=...

Done.

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/de...
File headless/public/util/deterministic_http_protocol_handler.h (right):

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/de...
headless/public/util/deterministic_http_protocol_handler.h:37: void
set_browser_context(HeadlessBrowserContext* browser_context) {
On 2016/09/22 12:09:40, Sami wrote:
> I wonder if we could avoid the dependency on the browser context by calling
> URLRequestJobManager::GetInstance()->CreateJob() from MaybeCreateJob() (and
> making sure to not do that re-entrantly)?
> 
> Alternatively should we structure this so that it uses another
> net::URLRequestJobFactory::ProtocolHandler to implement fetching so that you
> could hook this up to your custom protocol handler too?

We can get the context from the request itself.

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/ht...
File headless/public/util/http_url_fetcher.cc (right):

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/ht...
headless/public/util/http_url_fetcher.cc:70: request_->set_method("GET");
On 2016/09/22 12:09:40, Sami wrote:
> Could we add a DCHECK for this somewhere? I think silently turning POSTs into
> GETs will be surprising to say the least.

I went ahead and added support for the verb.

https://codereview.chromium.org/2352663003/diff/20001/headless/public/util/ht...
headless/public/util/http_url_fetcher.cc:97: LOG(WARNING) << "SSL certificate
error when fetching PAC script, aborting.";
On 2016/09/22 12:09:40, Sami wrote:
> I'm not sure how this is related to PAC scripts?

Whoops cut & paste fail.

[commit-bot: I haz the power, 2016-09-23 13:29:24]

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

[alex clarke (OOO till 29th), 2016-09-23 13:34:45]

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

[commit-bot: I haz the power, 2016-09-23 13:34:58]

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, 2016-09-23 13:37:10]

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

[commit-bot: I haz the power, 2016-09-23 13:37:11]

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

[Sami, 2016-09-23 14:27:29]

https://codereview.chromium.org/2352663003/diff/60001/headless/public/headles...
File headless/public/headless_browser_context.h (right):

https://codereview.chromium.org/2352663003/diff/60001/headless/public/headles...
headless/public/headless_browser_context.h:67: virtual net::URLRequestContext*
GetRequestContext() const = 0;
Not needed anymore?

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/de...
File headless/public/util/deterministic_http_protocol_handler.cc (right):

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/de...
headless/public/util/deterministic_http_protocol_handler.cc:65: if
(!url_request_context_) {
DCHECK(io_task_runner_->BelongsToCurrentThread())?

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/de...
File headless/public/util/deterministic_http_protocol_handler.h (right):

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/de...
headless/public/util/deterministic_http_protocol_handler.h:22: // A
deterministic protocol handler.  Requests make to this protocol handler
s/make/made/

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/de...
headless/public/util/deterministic_http_protocol_handler.h:48: // the IO thread.
The URLRequestContextis setup to bypassing any
s/Contextis/Context is/

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/ht...
File headless/public/util/http_url_fetcher.cc (right):

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/ht...
headless/public/util/http_url_fetcher.cc:86: bool is_hsts_ok) {
Do we need to care about is_hsts_ok?

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/ht...
File headless/public/util/http_url_fetcher.h (right):

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/ht...
headless/public/util/http_url_fetcher.h:18: // A simple URLFetcher that wraps a
net::URLRequest.
net::URLRequestContext?

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/ht...
headless/public/util/http_url_fetcher.h:19: class HttpUrlFetcher : public
URLFetcher {
nit: "URL" to match the interface. Maybe we should call this ContextURLFetcher
or something since the implementation doesn't seem to have much to do with http
per se?

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/ht...
headless/public/util/http_url_fetcher.h:34: std::unique_ptr<Delegate> delagate_;
typo: delegate_

[alex clarke (OOO till 29th), 2016-09-23 16:40:16]

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

[alex clarke (OOO till 29th), 2016-09-23 16:40:27]

PTAL

https://codereview.chromium.org/2352663003/diff/60001/headless/public/headles...
File headless/public/headless_browser_context.h (right):

https://codereview.chromium.org/2352663003/diff/60001/headless/public/headles...
headless/public/headless_browser_context.h:67: virtual net::URLRequestContext*
GetRequestContext() const = 0;
On 2016/09/23 14:27:29, Sami wrote:
> Not needed anymore?

Good point, it's gone.

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/de...
File headless/public/util/deterministic_http_protocol_handler.cc (right):

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/de...
headless/public/util/deterministic_http_protocol_handler.cc:65: if
(!url_request_context_) {
On 2016/09/23 14:27:29, Sami wrote:
> DCHECK(io_task_runner_->BelongsToCurrentThread())?

Done.

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/de...
File headless/public/util/deterministic_http_protocol_handler.h (right):

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/de...
headless/public/util/deterministic_http_protocol_handler.h:22: // A
deterministic protocol handler.  Requests make to this protocol handler
On 2016/09/23 14:27:29, Sami wrote:
> s/make/made/

Done.

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/de...
headless/public/util/deterministic_http_protocol_handler.h:48: // the IO thread.
The URLRequestContextis setup to bypassing any
On 2016/09/23 14:27:29, Sami wrote:
> s/Contextis/Context is/

Done.

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/ht...
File headless/public/util/http_url_fetcher.cc (right):

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/ht...
headless/public/util/http_url_fetcher.cc:86: bool is_hsts_ok) {
On 2016/09/23 14:27:29, Sami wrote:
> Do we need to care about is_hsts_ok?

It turns out this is actually called |fatal| by most implementations, and is
ignored by the majority of them.  I.e. we, like most implementations, don't make
any attempt to recover so we can ignore it.

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/ht...
File headless/public/util/http_url_fetcher.h (right):

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/ht...
headless/public/util/http_url_fetcher.h:18: // A simple URLFetcher that wraps a
net::URLRequest.
On 2016/09/23 14:27:29, Sami wrote:
> net::URLRequestContext?

Done.

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/ht...
headless/public/util/http_url_fetcher.h:19: class HttpUrlFetcher : public
URLFetcher {
On 2016/09/23 14:27:29, Sami wrote:
> nit: "URL" to match the interface. Maybe we should call this ContextURLFetcher
> or something since the implementation doesn't seem to have much to do with
http
> per se?

Done.

https://codereview.chromium.org/2352663003/diff/60001/headless/public/util/ht...
headless/public/util/http_url_fetcher.h:34: std::unique_ptr<Delegate> delagate_;
On 2016/09/23 14:27:29, Sami wrote:
> typo: delegate_

Done.

[commit-bot: I haz the power, 2016-09-23 16:40:31]

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

[Sami, 2016-09-23 16:49:06]

lgtm

[alex clarke (OOO till 29th), 2016-09-23 16:50:42]

The CQ bit was unchecked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2016-09-23 16:50:43]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2016-09-23 16:50:59]

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, 2016-09-23 18:04:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-23 18:05:28]

Description was changed from

==========
Adds a --deterministic-fetch flag to headless_shell

Adds a -deterministic-fetch flag to headless_shell which causes network
reuests to complete in the same order they're created in.  This removes
a significant source of network related non-determinism at the cost of
slower page loads.

BUG=546953
==========

to

==========
Adds a --deterministic-fetch flag to headless_shell

Adds a -deterministic-fetch flag to headless_shell which causes network
reuests to complete in the same order they're created in.  This removes
a significant source of network related non-determinism at the cost of
slower page loads.

BUG=546953

Committed: https://crrev.com/ad54054fcdee1ede7d009413d3577cea0f0e08f6
Cr-Commit-Position: refs/heads/master@{#420657}
==========

[commit-bot: I haz the power, 2016-09-23 18:05:29]

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