[Net] Assert that URLRequests with LOAD_IGNORE_LIMITS have MAXIMUM_PRIORITY

[Net] Assert that URLRequests with LOAD_IGNORE_LIMITS have MAXIMUM_PRIORITY

Also remove the priority juggling in ClientSocketPoolBaseHelper, and instead
assert that any requests with ignore_limits = true also has priority =
MAXIMUM_PRIORITY.

BUG=166689
R=asanka@chromium.org, gene@chromium.org, jamesr@chromium.org, joaodasilva@chromium.org, mmenke@chromium.org, simonjam@chromium.org
TBR=benm@chromium.org, jamesr@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232802

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234964

[akalin, 2013-10-29 23:27:59]

+mmenke for overall review
+mnissler for chrome/browser/policy OWNERS
+scottbyer for cloud_print OWNERS
+asanka for content/browser/download OWNERS
+simonjam for content/browser/loader OWNERS
+jamesr for webkit/ OWNERS

[akalin, 2013-10-29 23:27:59]

+mmenke for overall review
+mnissler for chrome/browser/policy OWNERS
+scottbyer for cloud_print OWNERS
+asanka for content/browser/download OWNERS
+simonjam for content/browser/loader OWNERS
+jamesr for webkit/ OWNERS

[akalin, 2013-10-29 23:28:39]

-mnissler, +joaodasilva

[James Simonsen, 2013-10-29 23:33:12]

I'm curious what motivated this. We already have code in Blink that makes
synchronous requests have highest priority. I think synchronous requests are the
only source of ignore limits requests. Why do we want this logic in the network
stack too?

[akalin, 2013-10-29 23:37:21]

On 2013/10/29 23:33:12, James Simonsen wrote:
> I'm curious what motivated this. We already have code in Blink that makes
> synchronous requests have highest priority. I think synchronous requests are
the
> only source of ignore limits requests. Why do we want this logic in the
network
> stack too?

The logic was already in the network stack, buried deep in
client_socket_pool_base.cc. This CL is to move that logic up into the URLRequest
layer. If we can assume that any URLRequest with IGNORE_LIMITS will have
MAXIMUM_PRIORITY, then that's even better -- I can just replace the logic with a
bunch of DCHECKs!

So in ResourceDispatcherHostImpl::BeginRequest, if sync_result is non-NULL, does
that then mean that request_data.priority == MAXIMUM_PRIORITY?

[mmenke, 2013-10-29 23:44:17]

On 2013/10/29 23:37:21, akalin wrote:
> On 2013/10/29 23:33:12, James Simonsen wrote:
> > I'm curious what motivated this. We already have code in Blink that makes
> > synchronous requests have highest priority. I think synchronous requests are
> the
> > only source of ignore limits requests. Why do we want this logic in the
> network
> > stack too?
> 
> The logic was already in the network stack, buried deep in
> client_socket_pool_base.cc. This CL is to move that logic up into the
URLRequest
> layer. If we can assume that any URLRequest with IGNORE_LIMITS will have
> MAXIMUM_PRIORITY, then that's even better -- I can just replace the logic with
a
> bunch of DCHECKs!
> 
> So in ResourceDispatcherHostImpl::BeginRequest, if sync_result is non-NULL,
does
> that then mean that request_data.priority == MAXIMUM_PRIORITY?

To elaborate a bit:  We have to increase the priority of ignore_limits requests
to the extent of going above MAXIMUM_PRIORITY requests in the socket pool.  We
also currently have to remember their *real* priority, to pass down to lower
level sockets.  This makes things weird and complicated, particularly since at
other levels, their real priority may not by MAXIMUM_PRIORITY, as far as the
network stack knows.

Life is simpler, and the world is just a much more swell place if we can assume
ignore_limits requests have MAXIMUM_PRIORITY.  Rather than add it as a DCHECK
when setting load flags, I'd rather enforce it when the network stack receives
the load flags (In this case, I prefer enforcing it, rather than having
correctness at the bottom layer of the network stack depend on a debug-only
failure at the top level...Or at the bottom level, for that matter).

[James Simonsen, 2013-10-30 00:12:02]

On 2013/10/29 23:37:21, akalin wrote:
> So in ResourceDispatcherHostImpl::BeginRequest, if sync_result is non-NULL,
does
> that then mean that request_data.priority == MAXIMUM_PRIORITY?

To the best of my knowledge, yes.

For Blink, all of the sync requests go through here:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Raw is XHR and XSLT is already highest. I'm not aware of any other sources of
sync requests.

Besides Blink, I'm not aware of any other sources of sync requests. They're
evil, so I'd imagine we'd avoid them everywhere else.

[mmenke, 2013-10-30 15:54:59]

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
net/socket/client_socket_pool_base.cc:1276:
pending_requests_.Insert(request.release(), priority);
Shouldn't we be calling InsertAtFront if ignore_limits is set?

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
File net/socket/client_socket_pool_base_unittest.cc (right):

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
net/socket/client_socket_pool_base_unittest.cc:106: bool ignore_limits() {
return behavior_ == IGNORE_LIMITS; }
Why the change?  Not strongly opposed, just doesn't seem to get anything, and
adds a new value to be familiar with (RESPECT_LIMITS).

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
net/socket/client_socket_pool_base_unittest.cc:4045: EXPECT_EQ(ERR_IO_PENDING,
StartRequestWithParams("a", HIGHEST, params_));
These should be MAXIMUM_PRIORITY now.

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
net/socket/client_socket_pool_base_unittest.cc:4057: EXPECT_EQ(OK,
request(2)->WaitForResult());
WAit...Why is this passing?  I don't see where we put the ignore_limits request
in front of the other request.

[akalin, 2013-10-30 23:32:16]

PTAL

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
net/socket/client_socket_pool_base.cc:1276:
pending_requests_.Insert(request.release(), priority);
On 2013/10/30 15:54:59, mmenke wrote:
> Shouldn't we be calling InsertAtFront if ignore_limits is set?

Done.

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
File net/socket/client_socket_pool_base_unittest.cc (right):

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
net/socket/client_socket_pool_base_unittest.cc:106: bool ignore_limits() {
return behavior_ == IGNORE_LIMITS; }
On 2013/10/30 15:54:59, mmenke wrote:
> Why the change?  Not strongly opposed, just doesn't seem to get anything, and
> adds a new value to be familiar with (RESPECT_LIMITS).

Enum params are preferred over bool params in the style guide, I believe. I
dislike passing around just 'true' and 'false' values anyway.

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
net/socket/client_socket_pool_base_unittest.cc:4045: EXPECT_EQ(ERR_IO_PENDING,
StartRequestWithParams("a", HIGHEST, params_));
On 2013/10/30 15:54:59, mmenke wrote:
> These should be MAXIMUM_PRIORITY now.

Done.

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
net/socket/client_socket_pool_base_unittest.cc:4057: EXPECT_EQ(OK,
request(2)->WaitForResult());
On 2013/10/30 15:54:59, mmenke wrote:
> WAit...Why is this passing?  I don't see where we put the ignore_limits
request
> in front of the other request.

Did you m ean the previous test (IgnoreLimits)? I'm not exactly sure what this
test (IgnoreLimitsCancelOtherJob) is supposed to be testing, to be honest.

https://codereview.chromium.org/51683002/diff/160001/content/browser/loader/r...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/51683002/diff/160001/content/browser/loader/r...
content/browser/loader/resource_scheduler.cc:293:
client->pending_requests.Insert(request,
should this also do InsertAtFront if ignore_limits is set?

[akalin, 2013-10-30 23:32:40]

On 2013/10/30 00:12:02, James Simonsen wrote:
> On 2013/10/29 23:37:21, akalin wrote:
> > So in ResourceDispatcherHostImpl::BeginRequest, if sync_result is non-NULL,
> does
> > that then mean that request_data.priority == MAXIMUM_PRIORITY?
> 
> To the best of my knowledge, yes.
> 
> For Blink, all of the sync requests go through here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Raw is XHR and XSLT is already highest. I'm not aware of any other sources of
> sync requests.
> 
> Besides Blink, I'm not aware of any other sources of sync requests. They're
> evil, so I'd imagine we'd avoid them everywhere else.

Cool! Okay, added a bunch of DCHECKs in ResourceDispatcherHost for that.

[James Simonsen, 2013-10-30 23:43:53]

On 2013/10/30 23:32:40, akalin wrote:
> On 2013/10/30 00:12:02, James Simonsen wrote:
> > On 2013/10/29 23:37:21, akalin wrote:
> > > So in ResourceDispatcherHostImpl::BeginRequest, if sync_result is
non-NULL,
> > does
> > > that then mean that request_data.priority == MAXIMUM_PRIORITY?
> > 
> > To the best of my knowledge, yes.
> > 
> > For Blink, all of the sync requests go through here:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Raw is XHR and XSLT is already highest. I'm not aware of any other sources
of
> > sync requests.
> > 
> > Besides Blink, I'm not aware of any other sources of sync requests. They're
> > evil, so I'd imagine we'd avoid them everywhere else.
> 
> Cool! Okay, added a bunch of DCHECKs in ResourceDispatcherHost for that.

Do me a favor and run the Blink layout trybots here too, just to be sure.

[akalin, 2013-10-31 00:13:07]

On 2013/10/30 23:43:53, James Simonsen wrote:
> On 2013/10/30 23:32:40, akalin wrote:
> > On 2013/10/30 00:12:02, James Simonsen wrote:
> > > On 2013/10/29 23:37:21, akalin wrote:
> > > > So in ResourceDispatcherHostImpl::BeginRequest, if sync_result is
> non-NULL,
> > > does
> > > > that then mean that request_data.priority == MAXIMUM_PRIORITY?
> > > 
> > > To the best of my knowledge, yes.
> > > 
> > > For Blink, all of the sync requests go through here:
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > Raw is XHR and XSLT is already highest. I'm not aware of any other sources
> of
> > > sync requests.
> > > 
> > > Besides Blink, I'm not aware of any other sources of sync requests.
They're
> > > evil, so I'd imagine we'd avoid them everywhere else.
> > 
> > Cool! Okay, added a bunch of DCHECKs in ResourceDispatcherHost for that.
> 
> Do me a favor and run the Blink layout trybots here too, just to be sure.

Done.

[akalin, 2013-10-31 00:13:42]

also +benm for android_webview/ OWNERS
also +gene for cloud_print/ OWNERS

[gene, 2013-10-31 00:52:31]

On 2013/10/31 00:13:42, akalin wrote:
> also +benm for android_webview/ OWNERS
> also +gene for cloud_print/ OWNERS

lgtm for cloud_print/

[Joao da Silva, 2013-10-31 07:26:03]

policy lgtm

[asanka, 2013-10-31 15:17:58]

/download/ lgtm

[mmenke, 2013-10-31 15:37:26]

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
File net/socket/client_socket_pool_base_unittest.cc (right):

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
net/socket/client_socket_pool_base_unittest.cc:106: bool ignore_limits() {
return behavior_ == IGNORE_LIMITS; }
On 2013/10/30 23:32:16, akalin wrote:
> On 2013/10/30 15:54:59, mmenke wrote:
> > Why the change?  Not strongly opposed, just doesn't seem to get anything,
and
> > adds a new value to be familiar with (RESPECT_LIMITS).
> 
> Enum params are preferred over bool params in the style guide, I believe. I
> dislike passing around just 'true' and 'false' values anyway.

But we weren't passing around raw bools anywhere.  I can understand that if it
were TestSocketParams(true, true, false, KINDA, SORTA, MAYBE, false, INDEED,
VERILY);

But set_ignore_limits(true) is pretty explicit.  And I think RESPECT_LIMITS
everywhere is actually more confusing - adds low value data everywhere.

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
net/socket/client_socket_pool_base_unittest.cc:4057: EXPECT_EQ(OK,
request(2)->WaitForResult());
On 2013/10/30 23:32:16, akalin wrote:
> On 2013/10/30 15:54:59, mmenke wrote:
> > WAit...Why is this passing?  I don't see where we put the ignore_limits
> request
> > in front of the other request.
> 
> Did you m ean the previous test (IgnoreLimits)? I'm not exactly sure what this
> test (IgnoreLimitsCancelOtherJob) is supposed to be testing, to be honest.

Ah!  I missed the cancel.  Yea, I was thinking this was just like the other
test, except with HIGHEST priority everywhere.

Purpose of this test:  When we cancel requests, in some cases we cancel
ConnectJobs.  We currently just do this in the case there are more requests than
ConnectJobs, but the logic used to be quite different, I believe.  If we end up
cancelling a ConnectJob that is intended to service an ignore_limits request,
that would be bad.

[akalin, 2013-11-01 07:02:46]

PTAL!

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
File net/socket/client_socket_pool_base_unittest.cc (right):

https://codereview.chromium.org/51683002/diff/1/net/socket/client_socket_pool...
net/socket/client_socket_pool_base_unittest.cc:106: bool ignore_limits() {
return behavior_ == IGNORE_LIMITS; }
On 2013/10/31 15:37:26, mmenke wrote:
> On 2013/10/30 23:32:16, akalin wrote:
> > On 2013/10/30 15:54:59, mmenke wrote:
> > > Why the change?  Not strongly opposed, just doesn't seem to get anything,
> and
> > > adds a new value to be familiar with (RESPECT_LIMITS).
> > 
> > Enum params are preferred over bool params in the style guide, I believe. I
> > dislike passing around just 'true' and 'false' values anyway.
> 
> But we weren't passing around raw bools anywhere.  I can understand that if it
> were TestSocketParams(true, true, false, KINDA, SORTA, MAYBE, false, INDEED,
> VERILY);
> 
> But set_ignore_limits(true) is pretty explicit.  And I think RESPECT_LIMITS
> everywhere is actually more confusing - adds low value data everywhere.

Okay, done.

https://codereview.chromium.org/51683002/diff/160001/content/browser/loader/r...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/51683002/diff/160001/content/browser/loader/r...
content/browser/loader/resource_scheduler.cc:293:
client->pending_requests.Insert(request,
On 2013/10/30 23:32:16, akalin wrote:
> should this also do InsertAtFront if ignore_limits is set?

Matt, any opinion here? Should I add a TODO?

[mmenke, 2013-11-01 15:47:59]

So...there's still at least one way to set IGNORE_LIMITS on a lower priority
request:  InitSocketHandleForRawConnect in client_socket_pool_manager.h is
exposed publicly.  It takes in load_flags, but always uses MEDIUM priority.

https://codereview.chromium.org/51683002/diff/160001/content/browser/loader/r...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/51683002/diff/160001/content/browser/loader/r...
content/browser/loader/resource_scheduler.cc:293:
client->pending_requests.Insert(request,
On 2013/11/01 07:02:47, akalin wrote:
> On 2013/10/30 23:32:16, akalin wrote:
> > should this also do InsertAtFront if ignore_limits is set?
> 
> Matt, any opinion here? Should I add a TODO?

We shouldn't be reprioritizing those requests in the first place.  Could make
this class more aware of those requests, but I don't think that's the way to go
about it.

[akalin, 2013-11-01 18:50:14]

Per our chat, Init...RawConnect always uses 0 for load flags, and the other
functions are already checked by the checks in PoolBaseHelper.

PTAL!

https://codereview.chromium.org/51683002/diff/160001/content/browser/loader/r...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/51683002/diff/160001/content/browser/loader/r...
content/browser/loader/resource_scheduler.cc:293:
client->pending_requests.Insert(request,
On 2013/11/01 15:48:00, mmenke wrote:
> On 2013/11/01 07:02:47, akalin wrote:
> > On 2013/10/30 23:32:16, akalin wrote:
> > > should this also do InsertAtFront if ignore_limits is set?
> > 
> > Matt, any opinion here? Should I add a TODO?
> 
> We shouldn't be reprioritizing those requests in the first place.  Could make
> this class more aware of those requests, but I don't think that's the way to
go
> about it.

Ah, right. Added a check instead.

[akalin, 2013-11-01 18:50:30]

simonjam, ping? Do the layout tests look okay?

[mmenke, 2013-11-01 19:06:38]

https://codereview.chromium.org/51683002/diff/1130001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/51683002/diff/1130001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:276: if
((request->url_request()->load_flags() & net::LOAD_IGNORE_LIMITS) != 0) {
optional nit:  I think !(request->url_request()->load_flags() &
net::LOAD_IGNORE_LIMITS) is a little more intuitive.

https://codereview.chromium.org/51683002/diff/1130001/net/socket/client_socke...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/51683002/diff/1130001/net/socket/client_socke...
net/socket/client_socket_pool_base.cc:1274: // We must cache this value before
we release |request|.
nit:  --We

https://codereview.chromium.org/51683002/diff/1130001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/51683002/diff/1130001/net/url_request/url_req...
net/url_request/url_request.cc:504: DCHECK(flags & LOAD_IGNORE_LIMITS);
So the IGNORE_LIMITS flag can be set, but not unset?  We DCHECK when we override
the priority due to MAX_LIMITS, anyways, so I think it's fine to clear the
flag...Isn't it?

I think the only case where we really have to DCHECK here is when setting the
flag, when there's either a job or priority is not MAXIMUM_PRIORITY.

https://codereview.chromium.org/51683002/diff/1130001/net/url_request/url_req...
net/url_request/url_request.cc:978: if (((load_flags_ & LOAD_IGNORE_LIMITS) !=
0) &&
optional nit:  Think !(load_flags_ & LOAD_IGNORE_LIMITS) is more intuitive

https://codereview.chromium.org/51683002/diff/1130001/net/url_request/url_req...
net/url_request/url_request.cc:979: (priority != MAXIMUM_PRIORITY)) {
nit:  Parens not needed.

https://codereview.chromium.org/51683002/diff/1130001/net/url_request/url_req...
net/url_request/url_request.cc:983: priority = MAXIMUM_PRIORITY;
optional nit:  In this case, priority_ will now be priority, so we can just
return early.  Doesn't save us anything, just think it makes the logic simpler.

https://codereview.chromium.org/51683002/diff/1130001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/51683002/diff/1130001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1914: EXPECT_EQ(MAXIMUM_PRIORITY,
req.priority());
Should we make sure req.SetPriority(MAXIMUM_PRIORITY) also works?

[James Simonsen, 2013-11-01 20:28:38]

lgtm

Yeah, they're all red, but clicking through they seem to all be flakes.

[akalin, 2013-11-01 23:15:51]

ptal

https://codereview.chromium.org/51683002/diff/1130001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/51683002/diff/1130001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:276: if
((request->url_request()->load_flags() & net::LOAD_IGNORE_LIMITS) != 0) {
On 2013/11/01 19:06:39, mmenke wrote:
> optional nit:  I think !(request->url_request()->load_flags() &
> net::LOAD_IGNORE_LIMITS) is a little more intuitive.

Done.

https://codereview.chromium.org/51683002/diff/1130001/net/socket/client_socke...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/51683002/diff/1130001/net/socket/client_socke...
net/socket/client_socket_pool_base.cc:1274: // We must cache this value before
we release |request|.
On 2013/11/01 19:06:39, mmenke wrote:
> nit:  --We

Done.

https://codereview.chromium.org/51683002/diff/1130001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/51683002/diff/1130001/net/url_request/url_req...
net/url_request/url_request.cc:504: DCHECK(flags & LOAD_IGNORE_LIMITS);
On 2013/11/01 19:06:39, mmenke wrote:
> So the IGNORE_LIMITS flag can be set, but not unset?  We DCHECK when we
override
> the priority due to MAX_LIMITS, anyways, so I think it's fine to clear the
> flag...Isn't it?
> 
> I think the only case where we really have to DCHECK here is when setting the
> flag, when there's either a job or priority is not MAXIMUM_PRIORITY.

It's fine, but the only use case doesn't ever clear the flag, right? I'd like to
keep the API simple until we actually need to relax the restriction.

https://codereview.chromium.org/51683002/diff/1130001/net/url_request/url_req...
net/url_request/url_request.cc:978: if (((load_flags_ & LOAD_IGNORE_LIMITS) !=
0) &&
On 2013/11/01 19:06:39, mmenke wrote:
> optional nit:  Think !(load_flags_ & LOAD_IGNORE_LIMITS) is more intuitive

Done.

https://codereview.chromium.org/51683002/diff/1130001/net/url_request/url_req...
net/url_request/url_request.cc:979: (priority != MAXIMUM_PRIORITY)) {
On 2013/11/01 19:06:39, mmenke wrote:
> nit:  Parens not needed.

Not needed, but I think it's clearer.

https://codereview.chromium.org/51683002/diff/1130001/net/url_request/url_req...
net/url_request/url_request.cc:983: priority = MAXIMUM_PRIORITY;
On 2013/11/01 19:06:39, mmenke wrote:
> optional nit:  In this case, priority_ will now be priority, so we can just
> return early.  Doesn't save us anything, just think it makes the logic
simpler.

Done.

https://codereview.chromium.org/51683002/diff/1130001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/51683002/diff/1130001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:1914: EXPECT_EQ(MAXIMUM_PRIORITY,
req.priority());
On 2013/11/01 19:06:39, mmenke wrote:
> Should we make sure req.SetPriority(MAXIMUM_PRIORITY) also works?

Done.

[mmenke, 2013-11-04 15:47:41]

LGTM

[akalin, 2013-11-04 21:16:03]

tbring

[commit-bot: I haz the power, 2013-11-04 21:18:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/51683002/180016

[akalin, 2013-11-04 21:18:36]

Committed patchset #6 manually as r232802 (presubmit successful).

[commit-bot: I haz the power, 2013-11-04 21:18:43]

Failed to apply patch for
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
  Hunk #1 FAILED at 43.
  1 out of 1 hunk FAILED -- saving rejects to file
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc.rej

Patch:      
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
Index:
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
diff --git
a/android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
b/android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
index
0b5312a0da89b94e054dcedb6682fb2aa26e75b5..80fdad483a486a82b372a4639983e0f7edc95bcb
100644
---
a/android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
+++
b/android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
@@ -43,7 +43,7 @@ void SetCacheControlFlag(
   int load_flags = request->load_flags();
   load_flags &= ~all_cache_control_flags;
   load_flags |= flag;
-  request->set_load_flags(load_flags);
+  request->SetLoadFlags(load_flags);
 }
 
 }  // namespace

[jamesr, 2013-11-04 22:34:18]

webkit/ lgtm

[dglazkov, 2013-11-04 23:11:38]

I am pretty sure this is causing DCHECKs on Canaries:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

Please look into this,
   Smokey the Bear.

[akalin, 2013-11-04 23:33:55]

On 2013/11/04 23:11:38, Dimitri Glazkov wrote:
> I am pretty sure this is causing DCHECKs on Canaries:
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...
> 
> Please look into this,
>    Smokey the Bear.

Reverted and looking, sorry

[akalin, 2013-11-04 23:45:27]

On 2013/11/04 23:33:55, akalin wrote:
> On 2013/11/04 23:11:38, Dimitri Glazkov wrote:
> > I am pretty sure this is causing DCHECKs on Canaries:
> > 
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...
> > 
> > Please look into this,
> >    Smokey the Bear.
> 
> Reverted and looking, sorry

Looks like there are some cases where we try to change the priority of a sync
XHR:

[7401:7426:1104/151900:1655224203858:FATAL:url_request.cc(979)] Check failed:
false. 
 [0x000001ea81ce] base::debug::StackTrace::StackTrace()
 [0x000001ef41cf] logging::LogMessage::~LogMessage()
 [0x0000022d6297] net::URLRequest::SetPriority()
 [0x0000013b7fa6] ChromeResourceDispatcherHostDelegate::RequestBeginning()
 [0x00000620c950] content::ResourceDispatcherHostImpl::BeginRequest()
 [0x00000620b728] content::ResourceDispatcherHostImpl::OnSyncLoad()
 [0x0000062274bc] DispatchToMethod<>()
 [0x00000622733b] IPC::SyncMessageSchema<>::DispatchDelayReplyWithSendParams<>()
 [0x000006213eae] ResourceHostMsg_SyncLoad::DispatchDelayReply<>()
 [0x00000620b171] content::ResourceDispatcherHostImpl::OnMessageReceived()
 [0x000006237645] content::ResourceMessageFilter::OnMessageReceived()
 [0x00000609e0c1] content::BrowserMessageFilter::Internal::DispatchMessage()
 [0x00000609ddee] content::BrowserMessageFilter::Internal::OnMessageReceived()
...

This is in the PrerenderDeferredSynchronousXHR browsertest.

[James Simonsen, 2013-11-04 23:56:43]

On 2013/11/04 23:45:27, akalin wrote:
> On 2013/11/04 23:33:55, akalin wrote:
> > On 2013/11/04 23:11:38, Dimitri Glazkov wrote:
> > > I am pretty sure this is causing DCHECKs on Canaries:
> > > 
> > >
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...
> > > 
> > > Please look into this,
> > >    Smokey the Bear.
> > 
> > Reverted and looking, sorry
> 
> Looks like there are some cases where we try to change the priority of a sync
> XHR:
> 
> [7401:7426:1104/151900:1655224203858:FATAL:url_request.cc(979)] Check failed:
> false. 
>  [0x000001ea81ce] base::debug::StackTrace::StackTrace()
>  [0x000001ef41cf] logging::LogMessage::~LogMessage()
>  [0x0000022d6297] net::URLRequest::SetPriority()
>  [0x0000013b7fa6] ChromeResourceDispatcherHostDelegate::RequestBeginning()
>  [0x00000620c950] content::ResourceDispatcherHostImpl::BeginRequest()
>  [0x00000620b728] content::ResourceDispatcherHostImpl::OnSyncLoad()
>  [0x0000062274bc] DispatchToMethod<>()
>  [0x00000622733b]
IPC::SyncMessageSchema<>::DispatchDelayReplyWithSendParams<>()
>  [0x000006213eae] ResourceHostMsg_SyncLoad::DispatchDelayReply<>()
>  [0x00000620b171] content::ResourceDispatcherHostImpl::OnMessageReceived()
>  [0x000006237645] content::ResourceMessageFilter::OnMessageReceived()
>  [0x00000609e0c1] content::BrowserMessageFilter::Internal::DispatchMessage()
>  [0x00000609ddee] content::BrowserMessageFilter::Internal::OnMessageReceived()
> ...
> 
> This is in the PrerenderDeferredSynchronousXHR browsertest.

Yep. That case makes sense. Sorry, I wasn't aware of it.

I'm curious what's going on in the Blink case. I'm not exactly sure what happens
with XHRs and whether we de-duplicate them. If so, I'd expect a
didChangePriority message to come across. Maybe that happens after your CHECK,
or maybe there's a bug and it's not sent.

[Joao da Silva, 2013-11-13 08:31:50]

Fred, should this issue be closed? Looks like it was already committed but it's
still open in Rietveld.

[akalin, 2013-11-13 08:32:18]

On 2013/11/13 08:31:50, Joao da Silva wrote:
> Fred, should this issue be closed? Looks like it was already committed but
it's
> still open in Rietveld.

No, I still need to reland it.

[akalin, 2013-11-14 00:00:14]

Committed patchset #7 manually as r234964 (presubmit successful).