Add Network.setSendRequestIdHeader and use it in headless

Add Network.setSendRequestIdHeader and use it in headless

This lets us correlate fetches seen by a UrlRequest handler with
DevTools network domain events.

BUG=546953
Review-Url: https://codereview.chromium.org/2695923010
Cr-Commit-Position: refs/heads/master@{#453945}
Committed: https://chromium.googlesource.com/chromium/src/+/3aa690e2c0e98d3274c8e988b85f28cf304c0320

[alex clarke (OOO till 29th), 2017-02-16 17:28:46]

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

[alex clarke (OOO till 29th), 2017-02-16 17:28:46]

PTAL

[alex clarke (OOO till 29th), 2017-02-16 17:29:05]

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

[commit-bot: I haz the power, 2017-02-16 17:29: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-02-16 18:01:44]

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

[commit-bot: I haz the power, 2017-02-16 18:01:45]

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

[alex clarke (OOO till 29th), 2017-02-17 10:08:06]

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

[commit-bot: I haz the power, 2017-02-17 10:08: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-17 10:46:57]

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

[commit-bot: I haz the power, 2017-02-17 10:46:57]

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

[alex clarke (OOO till 29th), 2017-02-22 14:41:16]

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

[commit-bot: I haz the power, 2017-02-22 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-22 15:28:42]

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

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

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

[alex clarke (OOO till 29th), 2017-02-23 14:24:04]

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

[commit-bot: I haz the power, 2017-02-23 14:24:19]

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-23 16:05:06]

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

[commit-bot: I haz the power, 2017-02-23 16:05:07]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2017-02-27 12:13:25]

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

[commit-bot: I haz the power, 2017-02-27 12:13:31]

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), 2017-02-27 12:33:40]

PTAL.  I've changed the header to be added unconditionally.

[Sami, 2017-02-27 12:39:07]

Thanks. So if I understood right, the extra header lives from when DevTools
inserts it at the point a new request comes out of Blink and is removed by the
protocol handler before making it to the wire (as well as before DevTools makes
some internal copy of the request?). Assuming the protocol handler does its job
correctly, there's no risk of the id becoming visible to either the web server
or Javascript in the form of responses, right?

https://codereview.chromium.org/2695923010/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://codereview.chromium.org/2695923010/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:25: // Keep in sync with
X_DevTools_Requestt_Id defined in HTTPNames.json5.
typo: request

https://codereview.chromium.org/2695923010/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:120:
kDevToolsEmulateNetworkConditionsClientId);
Should we also check for kDevToolsRequestId?

https://codereview.chromium.org/2695923010/diff/40001/headless/public/util/ge...
File headless/public/util/generic_url_request_job.cc (right):

https://codereview.chromium.org/2695923010/diff/40001/headless/public/util/ge...
headless/public/util/generic_url_request_job.cc:28: // Keep in sync with
X_DevTools_Emulate_Network_Conditions_Client_Id defined in
Comment talks about a different header.

https://codereview.chromium.org/2695923010/diff/40001/headless/public/util/pr...
File headless/public/util/protocol_handler_request_id_browsertest.cc (right):

https://codereview.chromium.org/2695923010/diff/40001/headless/public/util/pr...
headless/public/util/protocol_handler_request_id_browsertest.cc:61: //
MockURLFetcher implementation:
nit: s/Mock//

https://codereview.chromium.org/2695923010/diff/40001/headless/public/util/pr...
headless/public/util/protocol_handler_request_id_browsertest.cc:64: const
net::HttpRequestHeaders& request_headers,
Should we check that the magic header isn't in request_headers?

https://codereview.chromium.org/2695923010/diff/40001/headless/public/util/pr...
headless/public/util/protocol_handler_request_id_browsertest.cc:189:
url_to_devtools_id_[params.GetRequest()->GetUrl()] = params.GetRequestId();
Maybe also check that the magic header isn't included here.

https://codereview.chromium.org/2695923010/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2695923010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:701:
AtomicString(IdentifiersFactory::requestId(identifier)));
This won't be set for blocked requests (didBlockRequest). I'm assuming that's
okay?

[alex clarke (OOO till 29th), 2017-02-27 13:51:16]

https://codereview.chromium.org/2695923010/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://codereview.chromium.org/2695923010/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:25: // Keep in sync with
X_DevTools_Requestt_Id defined in HTTPNames.json5.
On 2017/02/27 12:39:07, Sami wrote:
> typo: request

Done.

https://codereview.chromium.org/2695923010/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.cc:120:
kDevToolsEmulateNetworkConditionsClientId);
On 2017/02/27 12:39:07, Sami wrote:
> Should we also check for kDevToolsRequestId?

Possibly yes. Pavel WDYT?

https://codereview.chromium.org/2695923010/diff/40001/headless/public/util/ge...
File headless/public/util/generic_url_request_job.cc (right):

https://codereview.chromium.org/2695923010/diff/40001/headless/public/util/ge...
headless/public/util/generic_url_request_job.cc:28: // Keep in sync with
X_DevTools_Emulate_Network_Conditions_Client_Id defined in
On 2017/02/27 12:39:07, Sami wrote:
> Comment talks about a different header.

Done.

https://codereview.chromium.org/2695923010/diff/40001/headless/public/util/pr...
File headless/public/util/protocol_handler_request_id_browsertest.cc (right):

https://codereview.chromium.org/2695923010/diff/40001/headless/public/util/pr...
headless/public/util/protocol_handler_request_id_browsertest.cc:61: //
MockURLFetcher implementation:
On 2017/02/27 12:39:07, Sami wrote:
> nit: s/Mock//

Done.

https://codereview.chromium.org/2695923010/diff/40001/headless/public/util/pr...
headless/public/util/protocol_handler_request_id_browsertest.cc:64: const
net::HttpRequestHeaders& request_headers,
On 2017/02/27 12:39:07, Sami wrote:
> Should we check that the magic header isn't in request_headers?

Done.

https://codereview.chromium.org/2695923010/diff/40001/headless/public/util/pr...
headless/public/util/protocol_handler_request_id_browsertest.cc:189:
url_to_devtools_id_[params.GetRequest()->GetUrl()] = params.GetRequestId();
On 2017/02/27 12:39:07, Sami wrote:
> Maybe also check that the magic header isn't included here.

Done.

https://codereview.chromium.org/2695923010/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2695923010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:701:
AtomicString(IdentifiersFactory::requestId(identifier)));
On 2017/02/27 12:39:07, Sami wrote:
> This won't be set for blocked requests (didBlockRequest). I'm assuming that's
> okay?

I think that's fine because the protocol handlers will never see the fetch if
it's canceled by DevTools.

[pfeldman, 2017-02-27 17:30:17]

devtools lgtm

[pfeldman, 2017-02-27 17:30:22]

https://codereview.chromium.org/2695923010/diff/60001/content/browser/devtool...
File content/browser/devtools/devtools_url_request_interceptor.h (right):

https://codereview.chromium.org/2695923010/diff/60001/content/browser/devtool...
content/browser/devtools/devtools_url_request_interceptor.h:13: class
DevtoolsUrlRequestInterceptor : public net::URLRequestInterceptor {
nuke this one.

[alex clarke (OOO till 29th), 2017-02-27 22:20:27]

Sami, PTAL

https://codereview.chromium.org/2695923010/diff/60001/content/browser/devtool...
File content/browser/devtools/devtools_url_request_interceptor.h (right):

https://codereview.chromium.org/2695923010/diff/60001/content/browser/devtool...
content/browser/devtools/devtools_url_request_interceptor.h:13: class
DevtoolsUrlRequestInterceptor : public net::URLRequestInterceptor {
On 2017/02/27 17:30:22, pfeldman wrote:
> nuke this one.

Whoops that was intended for another patch.  It's gone.

[alex clarke (OOO till 29th), 2017-02-27 22:20:33]

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

[commit-bot: I haz the power, 2017-02-27 22:21: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-02-28 00:35:34]

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

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

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, 2017-02-28 13:40:49]

lgtm, thanks!

[alex clarke (OOO till 29th), 2017-02-28 21:35:18]

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

[commit-bot: I haz the power, 2017-02-28 21:35:53]

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-28 23:42:52]

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

[commit-bot: I haz the power, 2017-02-28 23:42:53]

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_...)

[alex clarke (OOO till 29th), 2017-03-01 08:59:29]

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

[commit-bot: I haz the power, 2017-03-01 08:59:47]

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-03-01 10:40:30]

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

[commit-bot: I haz the power, 2017-03-01 10:40:31]

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_...)

[alex clarke (OOO till 29th), 2017-03-01 13:22:55]

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

[commit-bot: I haz the power, 2017-03-01 13:23: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), 2017-03-01 13:23:36]

Made a few changes to fix tests.  PTAL.

[Sami, 2017-03-01 14:50:15]

Still lgtm.

[commit-bot: I haz the power, 2017-03-01 15:06:10]

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

[commit-bot: I haz the power, 2017-03-01 15:06:12]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2017-03-01 16:09:14]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2017-03-01 16:09:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2695923010/#ps120001
(title: "Fix the layout test")

[commit-bot: I haz the power, 2017-03-01 16:09:26]

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-03-01 16:14:03]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488384554185610,
"parent_rev": "54d43255d0c2a680e0ff933e675bc0b0a445809b", "commit_rev":
"3aa690e2c0e98d3274c8e988b85f28cf304c0320"}

[commit-bot: I haz the power, 2017-03-01 16:14:44]

Description was changed from

==========
Add Network.setSendRequestIdHeader and use it in headless

This lets us correlate fetches seen by a UrlRequest handler with
DevTools network domain events.

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

to

==========
Add Network.setSendRequestIdHeader and use it in headless

This lets us correlate fetches seen by a UrlRequest handler with
DevTools network domain events.

BUG=546953

Review-Url: https://codereview.chromium.org/2695923010
Cr-Commit-Position: refs/heads/master@{#453945}
Committed:
https://chromium.googlesource.com/chromium/src/+/3aa690e2c0e98d3274c8e988b85f...
==========

[commit-bot: I haz the power, 2017-03-01 16:14:45]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/3aa690e2c0e98d3274c8e988b85f...

[jam, 2017-03-03 19:38:16]

Description was changed from

==========
Add Network.setSendRequestIdHeader and use it in headless

This lets us correlate fetches seen by a UrlRequest handler with
DevTools network domain events.

BUG=546953

Review-Url: https://codereview.chromium.org/2695923010
Cr-Commit-Position: refs/heads/master@{#453945}
Committed:
https://chromium.googlesource.com/chromium/src/+/3aa690e2c0e98d3274c8e988b85f...
==========

to

==========
Add Network.setSendRequestIdHeader and use it in headless

This lets us correlate fetches seen by a UrlRequest handler with
DevTools network domain events.

BUG=546953

Review-Url: https://codereview.chromium.org/2695923010
Cr-Commit-Position: refs/heads/master@{#453945}
Committed:
https://chromium.googlesource.com/chromium/src/+/3aa690e2c0e98d3274c8e988b85f...
==========

[jam, 2017-03-03 19:38:16]

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

[jam, 2017-03-03 19:38:45]

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

[jam, 2017-03-03 19:38:46]

This broke with PlzNavigate (see my earlier change in
https://codereview.chromium.org/2720543002/ to get it to pass). I'll add this to
the CQ so that it's easier to catch in the future.

In the meantime, I spent some time trying to figure out how to solve this. In
navigation_request.cc's AddAdditionalRequestHeaders, we could add code to add
the new header. The problem is that the request identifier would have to be
generated in the browser since we don't have a renderer yet, and we'd need to
send this to the renderer so that it can replace the generated one there. WDYT?

[dgozman, 2017-03-03 19:54:36]

On 2017/03/03 19:38:46, jam wrote:
> This broke with PlzNavigate (see my earlier change in
> https://codereview.chromium.org/2720543002/ to get it to pass). I'll add this
to
> the CQ so that it's easier to catch in the future.
> 
> In the meantime, I spent some time trying to figure out how to solve this. In
> navigation_request.cc's AddAdditionalRequestHeaders, we could add code to add
> the new header. The problem is that the request identifier would have to be
> generated in the browser since we don't have a renderer yet, and we'd need to
> send this to the renderer so that it can replace the generated one there.
WDYT?

Sounds like the only possible solution, unless we actually notify about the
navigation request from the browser (but that has more problems with redirects
and initiators).

[jam, 2017-03-03 20:36:08]

On 2017/03/03 19:54:36, dgozman wrote:
> On 2017/03/03 19:38:46, jam wrote:
> > This broke with PlzNavigate (see my earlier change in
> > https://codereview.chromium.org/2720543002/ to get it to pass). I'll add
this
> to
> > the CQ so that it's easier to catch in the future.
> > 
> > In the meantime, I spent some time trying to figure out how to solve this.
In
> > navigation_request.cc's AddAdditionalRequestHeaders, we could add code to
add
> > the new header. The problem is that the request identifier would have to be
> > generated in the browser since we don't have a renderer yet, and we'd need
to
> > send this to the renderer so that it can replace the generated one there.
> WDYT?
> 
> Sounds like the only possible solution, unless we actually notify about the
> navigation request from the browser (but that has more problems with redirects
> and initiators).

Alex/Sami: can someone from headless team take care of this?

[alex clarke (OOO till 29th), 2017-03-07 12:53:26]

On 2017/03/03 20:36:08, jam wrote:
> On 2017/03/03 19:54:36, dgozman wrote:
> > On 2017/03/03 19:38:46, jam wrote:
> > > This broke with PlzNavigate (see my earlier change in
> > > https://codereview.chromium.org/2720543002/ to get it to pass). I'll add
> this
> > to
> > > the CQ so that it's easier to catch in the future.
> > > 
> > > In the meantime, I spent some time trying to figure out how to solve this.
> In
> > > navigation_request.cc's AddAdditionalRequestHeaders, we could add code to
> add
> > > the new header. The problem is that the request identifier would have to
be
> > > generated in the browser since we don't have a renderer yet, and we'd need
> to
> > > send this to the renderer so that it can replace the generated one there.
> > WDYT?
> > 
> > Sounds like the only possible solution, unless we actually notify about the
> > navigation request from the browser (but that has more problems with
redirects
> > and initiators).
> 
> Alex/Sami: can someone from headless team take care of this?

This seems quite a complex problem :)  I'll see what we can do.  One idea we had
is to move the generation of the RequestWillBeSent and ResponseReceived events
over to the browser, letting us use a consistent browser generated frame ID. I'm
not entirely sure how feasible that is but it feels like it might dovetail with
the idea of adding devtools protocol command and events to allow programmatic
blocking & rewriting of network fetches and responses.