Avoid reusing XMLHttpRequest resources during document load.

Avoid reusing XMLHttpRequest resources during document load.

The determineRevalidationPolicy() predicate decides how an existing
cached resource can be (re)utilized upon another fetch to the same
resource. It will allow reuse of already loaded (and loading)
resources while a document is being loaded.

Such resource reuse isn't safe to do for raw resources requested
through XHR, as there's too much state and header information that an
XHR request can be furnished with. Hence, we exempt raw resources and
delegate to the standard cache control policy to decide on reuse. Like
it would do once the document has loaded.

R=japhet@chromium.org
BUG=314433
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161846

[sof, 2013-11-05 19:27:22]

I'd appreciate it if you could have a look.

[Nate Chapin, 2013-11-05 19:39:24]

https://codereview.chromium.org/59043006/diff/1/Source/core/fetch/ResourceFet...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/59043006/diff/1/Source/core/fetch/ResourceFet...
Source/core/fetch/ResourceFetcher.cpp:864: if (type == Resource::Raw)
We probably want a narrower fix than this. RawResource::canReuse() permits reuse
in a much narrower set of cases than other resources, but it's possible that
it's still too permissive. Regardless, the fix should probably go there.

Based on the test case you provided, perhaps canReuse() should check whether
either the existing or the new request have a url with a query?

[sof, 2013-11-05 21:43:47]

On 2013/11/05 19:39:24, Nate Chapin wrote:
>
https://codereview.chromium.org/59043006/diff/1/Source/core/fetch/ResourceFet...
> File Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/59043006/diff/1/Source/core/fetch/ResourceFet...
> Source/core/fetch/ResourceFetcher.cpp:864: if (type == Resource::Raw)
> We probably want a narrower fix than this. RawResource::canReuse() permits
reuse
> in a much narrower set of cases than other resources, but it's possible that
> it's still too permissive. Regardless, the fix should probably go there.
> 
> Based on the test case you provided, perhaps canReuse() should check whether
> either the existing or the new request have a url with a query?

Thanks, it would then have to return false always to be consistent with this
change, which is pretty strong. Too strong for your liking?

i.e., testing done after having initially filed the issue showed that this isn't
limited to resources having a query portion in their URLs (test added here also
checks this.)

[I just noticed that a fast/workers/ test's expected output hadn't been updated,
as Worker async script loads also use raw resources. My apologies for
overlooking
this, I'll upload a new patchset.]

[Nate Chapin, 2013-11-05 21:47:13]

On 2013/11/05 21:43:47, sof wrote:
> On 2013/11/05 19:39:24, Nate Chapin wrote:
> >
>
https://codereview.chromium.org/59043006/diff/1/Source/core/fetch/ResourceFet...
> > File Source/core/fetch/ResourceFetcher.cpp (right):
> > 
> >
>
https://codereview.chromium.org/59043006/diff/1/Source/core/fetch/ResourceFet...
> > Source/core/fetch/ResourceFetcher.cpp:864: if (type == Resource::Raw)
> > We probably want a narrower fix than this. RawResource::canReuse() permits
> reuse
> > in a much narrower set of cases than other resources, but it's possible that
> > it's still too permissive. Regardless, the fix should probably go there.
> > 
> > Based on the test case you provided, perhaps canReuse() should check whether
> > either the existing or the new request have a url with a query?
> 
> Thanks, it would then have to return false always to be consistent with this
> change, which is pretty strong. Too strong for your liking?

Yep, that's right. :)

> 
> i.e., testing done after having initially filed the issue showed that this
isn't
> limited to resources having a query portion in their URLs (test added here
also
> checks this.)
> 
> [I just noticed that a fast/workers/ test's expected output hadn't been
updated,
> as Worker async script loads also use raw resources. My apologies for
> overlooking
> this, I'll upload a new patchset.]

Thanks for working on this!

[sof, 2013-11-06 07:22:21]

On 2013/11/05 21:47:13, Nate Chapin wrote:
> On 2013/11/05 21:43:47, sof wrote:
> > On 2013/11/05 19:39:24, Nate Chapin wrote:
> > >
> >
>
https://codereview.chromium.org/59043006/diff/1/Source/core/fetch/ResourceFet...
> > > File Source/core/fetch/ResourceFetcher.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/59043006/diff/1/Source/core/fetch/ResourceFet...
> > > Source/core/fetch/ResourceFetcher.cpp:864: if (type == Resource::Raw)
> > > We probably want a narrower fix than this. RawResource::canReuse() permits
> > reuse
> > > in a much narrower set of cases than other resources, but it's possible
that
> > > it's still too permissive. Regardless, the fix should probably go there.
> > > 
> > > Based on the test case you provided, perhaps canReuse() should check
whether
> > > either the existing or the new request have a url with a query?
> > 
> > Thanks, it would then have to return false always to be consistent with this
> > change, which is pretty strong. Too strong for your liking?
> 
> Yep, that's right. :)
> 

Fair enough :) Do you have suggestions for an appropriate way to
proceed?

[I first happened upon this via a failed CORS test (#3 in that list),

 
http://w3c-test.org/webappsec/tests/cors/submitted/opera/staging/credentials-...
]

[Nate Chapin, 2013-11-07 17:28:32]

On 2013/11/06 07:22:21, sof wrote:
> On 2013/11/05 21:47:13, Nate Chapin wrote:
> > On 2013/11/05 21:43:47, sof wrote:
> > > On 2013/11/05 19:39:24, Nate Chapin wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/59043006/diff/1/Source/core/fetch/ResourceFet...
> > > > File Source/core/fetch/ResourceFetcher.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/59043006/diff/1/Source/core/fetch/ResourceFet...
> > > > Source/core/fetch/ResourceFetcher.cpp:864: if (type == Resource::Raw)
> > > > We probably want a narrower fix than this. RawResource::canReuse()
permits
> > > reuse
> > > > in a much narrower set of cases than other resources, but it's possible
> that
> > > > it's still too permissive. Regardless, the fix should probably go there.
> > > > 
> > > > Based on the test case you provided, perhaps canReuse() should check
> whether
> > > > either the existing or the new request have a url with a query?
> > > 
> > > Thanks, it would then have to return false always to be consistent with
this
> > > change, which is pretty strong. Too strong for your liking?
> > 
> > Yep, that's right. :)
> > 
> 
> Fair enough :) Do you have suggestions for an appropriate way to
> proceed?
> 
> [I first happened upon this via a failed CORS test (#3 in that list),
> 
>  
>
http://w3c-test.org/webappsec/tests/cors/submitted/opera/staging/credentials-...
> ]

So looking at RawResource::canReuse()...

For the specific case that is broken on that test page, I would guess that:
if (m_resourceRequest.allowCookies() != newRequest.allowCookies())
    return false;
...is not getting properly triggered. Perhaps we should be plumbing the new
request's ResourceLoaderOptions down to canReuse() and more carefully compare
the new ResourceLoaderOptions to the existing resource's m_options.

[sof, 2013-11-07 19:14:40]

Thanks for looking into this some more, really appreciate it :)

On 2013/11/07 17:28:32, Nate Chapin wrote:
...
> > > > Thanks, it would then have to return false always to be consistent with
> this
> > > > change, which is pretty strong. Too strong for your liking?
> > > 
> > > Yep, that's right. :)
> > > 
> > 
> > Fair enough :) Do you have suggestions for an appropriate way to
> > proceed?
> > 
> > [I first happened upon this via a failed CORS test (#3 in that list),
> > 
> >  
> >
>
http://w3c-test.org/webappsec/tests/cors/submitted/opera/staging/credentials-...
> > ]
> 
> So looking at RawResource::canReuse()...
> 
> For the specific case that is broken on that test page, I would guess that:
> if (m_resourceRequest.allowCookies() != newRequest.allowCookies())
>     return false;
> ...is not getting properly triggered. Perhaps we should be plumbing the new
> request's ResourceLoaderOptions down to canReuse() and more carefully compare
> the new ResourceLoaderOptions to the existing resource's m_options.

Yes, that could be a worthy improvement for raw resources.

However, it wouldn't address that CORS test though, as the 2nd & 3rd CORS
request there are both with 'withCredentials=true', so their setting is
(and would be) the same for the allowCookies() flag.

And for the test attached to this CL, the requests are all "cookie enabled".

[Nate Chapin, 2013-11-07 19:27:29]

On 2013/11/07 19:14:40, sof wrote:
> Thanks for looking into this some more, really appreciate it :)
> 
> On 2013/11/07 17:28:32, Nate Chapin wrote:
> ...
> > > > > Thanks, it would then have to return false always to be consistent
with
> > this
> > > > > change, which is pretty strong. Too strong for your liking?
> > > > 
> > > > Yep, that's right. :)
> > > > 
> > > 
> > > Fair enough :) Do you have suggestions for an appropriate way to
> > > proceed?
> > > 
> > > [I first happened upon this via a failed CORS test (#3 in that list),
> > > 
> > >  
> > >
> >
>
http://w3c-test.org/webappsec/tests/cors/submitted/opera/staging/credentials-...
> > > ]
> > 
> > So looking at RawResource::canReuse()...
> > 
> > For the specific case that is broken on that test page, I would guess that:
> > if (m_resourceRequest.allowCookies() != newRequest.allowCookies())
> >     return false;
> > ...is not getting properly triggered. Perhaps we should be plumbing the new
> > request's ResourceLoaderOptions down to canReuse() and more carefully
compare
> > the new ResourceLoaderOptions to the existing resource's m_options.
> 
> Yes, that could be a worthy improvement for raw resources.
> 
> However, it wouldn't address that CORS test though, as the 2nd & 3rd CORS
> request there are both with 'withCredentials=true', so their setting is
> (and would be) the same for the allowCookies() flag.
> 
> And for the test attached to this CL, the requests are all "cookie enabled".

Hmm....what if we only reuse a resource if both existing and new requests have
options.credentialsRequested == ClientDidNotRequestCredentials? That maps to
withCredentials=false, which is the default case if I'm reading the spec
correctly.

[sof, 2013-11-08 14:54:36]

On 2013/11/07 19:27:29, Nate Chapin wrote:
> On 2013/11/07 19:14:40, sof wrote:
...
> > 
> > Yes, that could be a worthy improvement for raw resources.
> > 
> > However, it wouldn't address that CORS test though, as the 2nd & 3rd CORS
> > request there are both with 'withCredentials=true', so their setting is
> > (and would be) the same for the allowCookies() flag.
> > 
> > And for the test attached to this CL, the requests are all "cookie enabled".
> 
> Hmm....what if we only reuse a resource if both existing and new requests have
> options.credentialsRequested == ClientDidNotRequestCredentials? That maps to
> withCredentials=false, which is the default case if I'm reading the spec
> correctly.

That would solve this particular CORS TC not re-loading/fetching the 2nd
withCredentials=true request. But if it had instead issued a pair of
withCredentials=false requests or been requests of the non-CORS kind
(cf the CL tests), it wouldn't apply.

[sof, 2013-11-08 18:50:07]

The updated patchset simply syncs with what we've been discussing here, at least
that's the intent. (i.e., the medicine may still be too strong :) )

[Nate Chapin, 2013-11-08 21:47:45]

On 2013/11/08 14:54:36, sof wrote:
> On 2013/11/07 19:27:29, Nate Chapin wrote:
> > On 2013/11/07 19:14:40, sof wrote:
> ...
> > > 
> > > Yes, that could be a worthy improvement for raw resources.
> > > 
> > > However, it wouldn't address that CORS test though, as the 2nd & 3rd CORS
> > > request there are both with 'withCredentials=true', so their setting is
> > > (and would be) the same for the allowCookies() flag.
> > > 
> > > And for the test attached to this CL, the requests are all "cookie
enabled".
> > 
> > Hmm....what if we only reuse a resource if both existing and new requests
have
> > options.credentialsRequested == ClientDidNotRequestCredentials? That maps to
> > withCredentials=false, which is the default case if I'm reading the spec
> > correctly.
> 
> That would solve this particular CORS TC not re-loading/fetching the 2nd
> withCredentials=true request. But if it had instead issued a pair of
> withCredentials=false requests or been requests of the non-CORS kind
> (cf the CL tests), it wouldn't apply.

Ok, so it appears there are multiple different mistakes the current code makes.
I can't say I'm surprised :)

Would it cover these cases if we add 2 new rules: never reuse a resource if (1)
the url includes a query, (2) either the existing or the new request is
cross-origin?

[sof, 2013-11-08 22:27:57]

On 2013/11/08 21:47:45, Nate Chapin wrote:
> On 2013/11/08 14:54:36, sof wrote:
> > On 2013/11/07 19:27:29, Nate Chapin wrote:
> > > On 2013/11/07 19:14:40, sof wrote:
> > ...
> > > > 
> > > > Yes, that could be a worthy improvement for raw resources.
> > > > 
> > > > However, it wouldn't address that CORS test though, as the 2nd & 3rd
CORS
> > > > request there are both with 'withCredentials=true', so their setting is
> > > > (and would be) the same for the allowCookies() flag.
> > > > 
> > > > And for the test attached to this CL, the requests are all "cookie
> enabled".
> > > 
> > > Hmm....what if we only reuse a resource if both existing and new requests
> have
> > > options.credentialsRequested == ClientDidNotRequestCredentials? That maps
to
> > > withCredentials=false, which is the default case if I'm reading the spec
> > > correctly.
> > 
> > That would solve this particular CORS TC not re-loading/fetching the 2nd
> > withCredentials=true request. But if it had instead issued a pair of
> > withCredentials=false requests or been requests of the non-CORS kind
> > (cf the CL tests), it wouldn't apply.
> 
> Ok, so it appears there are multiple different mistakes the current code
makes.
> I can't say I'm surprised :)
> 
> Would it cover these cases if we add 2 new rules: never reuse a resource if
(1)
> the url includes a query, (2) either the existing or the new request is
> cross-origin?

Rule 1) would take care of an important class (for GET); RFC 2616 calls that
one out as something to avoid (cf 13.9.) That's not specifically for XHR
though, and implementations do go a bit further there (see attached bug for
some data points.)

Let me think about 2) overnight. My immediate reaction is that the cross-origin
property of a request is orthogonal to its "cacheability".

[sof, 2013-11-11 21:32:33]

On 2013/11/08 22:27:57, sof wrote:
> On 2013/11/08 21:47:45, Nate Chapin wrote:
> > On 2013/11/08 14:54:36, sof wrote:
> > > On 2013/11/07 19:27:29, Nate Chapin wrote:
> > > > On 2013/11/07 19:14:40, sof wrote:
> > > ...
> > > > > 
> > > > > Yes, that could be a worthy improvement for raw resources.
> > > > > 
> > > > > However, it wouldn't address that CORS test though, as the 2nd & 3rd
> CORS
> > > > > request there are both with 'withCredentials=true', so their setting
is
> > > > > (and would be) the same for the allowCookies() flag.
> > > > > 
> > > > > And for the test attached to this CL, the requests are all "cookie
> > enabled".
> > > > 
> > > > Hmm....what if we only reuse a resource if both existing and new
requests
> > have
> > > > options.credentialsRequested == ClientDidNotRequestCredentials? That
maps
> to
> > > > withCredentials=false, which is the default case if I'm reading the spec
> > > > correctly.
> > > 
> > > That would solve this particular CORS TC not re-loading/fetching the 2nd
> > > withCredentials=true request. But if it had instead issued a pair of
> > > withCredentials=false requests or been requests of the non-CORS kind
> > > (cf the CL tests), it wouldn't apply.
> > 
> > Ok, so it appears there are multiple different mistakes the current code
> makes.
> > I can't say I'm surprised :)
> > 
> > Would it cover these cases if we add 2 new rules: never reuse a resource if
> (1)
> > the url includes a query, (2) either the existing or the new request is
> > cross-origin?
> 
> Rule 1) would take care of an important class (for GET); RFC 2616 calls that
> one out as something to avoid (cf 13.9.) That's not specifically for XHR
> though, and implementations do go a bit further there (see attached bug for
> some data points.)
> 
> Let me think about 2) overnight. My immediate reaction is that the
cross-origin
> property of a request is orthogonal to its "cacheability".

Ah, looked into these TCs some more (with the above in mind); the reason why
they're not reloading is due to this condition in determineRevalidationPolicy()

 if (document() && !document()->loadEventFinished() &&
m_validatedURLs.contains(existingResource->url()))
     return Use;

as the TCs run to completion before onload is dispatched. What is its purpose
here? And does it make sense to have in place for all resource types?

If that check is weakened a bit, the revalidation predicate falls into
Resource::isExpired() eventually, which conservatively assumes a freshness
of 0 for resources lacking other cache control information. (=> Reload.)

[Nate Chapin, 2013-11-11 21:39:10]

On 2013/11/11 21:32:33, sof wrote:
> On 2013/11/08 22:27:57, sof wrote:
> > On 2013/11/08 21:47:45, Nate Chapin wrote:
> > > On 2013/11/08 14:54:36, sof wrote:
> > > > On 2013/11/07 19:27:29, Nate Chapin wrote:
> > > > > On 2013/11/07 19:14:40, sof wrote:
> > > > ...
> > > > > > 
> > > > > > Yes, that could be a worthy improvement for raw resources.
> > > > > > 
> > > > > > However, it wouldn't address that CORS test though, as the 2nd & 3rd
> > CORS
> > > > > > request there are both with 'withCredentials=true', so their setting
> is
> > > > > > (and would be) the same for the allowCookies() flag.
> > > > > > 
> > > > > > And for the test attached to this CL, the requests are all "cookie
> > > enabled".
> > > > > 
> > > > > Hmm....what if we only reuse a resource if both existing and new
> requests
> > > have
> > > > > options.credentialsRequested == ClientDidNotRequestCredentials? That
> maps
> > to
> > > > > withCredentials=false, which is the default case if I'm reading the
spec
> > > > > correctly.
> > > > 
> > > > That would solve this particular CORS TC not re-loading/fetching the 2nd
> > > > withCredentials=true request. But if it had instead issued a pair of
> > > > withCredentials=false requests or been requests of the non-CORS kind
> > > > (cf the CL tests), it wouldn't apply.
> > > 
> > > Ok, so it appears there are multiple different mistakes the current code
> > makes.
> > > I can't say I'm surprised :)
> > > 
> > > Would it cover these cases if we add 2 new rules: never reuse a resource
if
> > (1)
> > > the url includes a query, (2) either the existing or the new request is
> > > cross-origin?
> > 
> > Rule 1) would take care of an important class (for GET); RFC 2616 calls that
> > one out as something to avoid (cf 13.9.) That's not specifically for XHR
> > though, and implementations do go a bit further there (see attached bug for
> > some data points.)
> > 
> > Let me think about 2) overnight. My immediate reaction is that the
> cross-origin
> > property of a request is orthogonal to its "cacheability".
> 
> Ah, looked into these TCs some more (with the above in mind); the reason why
> they're not reloading is due to this condition in
determineRevalidationPolicy()
> 
>  if (document() && !document()->loadEventFinished() &&
> m_validatedURLs.contains(existingResource->url()))
>      return Use;
> 
> as the TCs run to completion before onload is dispatched. What is its purpose
> here? And does it make sense to have in place for all resource types?
> 
> If that check is weakened a bit, the revalidation predicate falls into
> Resource::isExpired() eventually, which conservatively assumes a freshness
> of 0 for resources lacking other cache control information. (=> Reload.)

I think that check dates back to a time when XHRs went through a separate code
path and never touched the cache. Before XHRs were cached, only "true"
subresources were put in the MemoryCache (image, scripts, etc.), and those types
don't have to support arbitrary headers. The assumption was that if the same URL
was requested more than once, we could safely assume that reuse was permitted,
but that clearly isn't a safe assumption for XHRs. I wonder if we should add a
"type != Resource::RawResource" check to that if() statement.

[sof, 2013-11-11 22:33:22]

On 2013/11/11 21:39:10, Nate Chapin wrote:
> On 2013/11/11 21:32:33, sof wrote:
> > On 2013/11/08 22:27:57, sof wrote:
> > > On 2013/11/08 21:47:45, Nate Chapin wrote:
> > > > On 2013/11/08 14:54:36, sof wrote:
> > > > > On 2013/11/07 19:27:29, Nate Chapin wrote:
> > > > > > On 2013/11/07 19:14:40, sof wrote:
> > > > > ...
> > > > > > > 
> > > > > > > Yes, that could be a worthy improvement for raw resources.
> > > > > > > 
> > > > > > > However, it wouldn't address that CORS test though, as the 2nd &
3rd
> > > CORS
> > > > > > > request there are both with 'withCredentials=true', so their
setting
> > is
> > > > > > > (and would be) the same for the allowCookies() flag.
> > > > > > > 
> > > > > > > And for the test attached to this CL, the requests are all "cookie
> > > > enabled".
> > > > > > 
> > > > > > Hmm....what if we only reuse a resource if both existing and new
> > requests
> > > > have
> > > > > > options.credentialsRequested == ClientDidNotRequestCredentials? That
> > maps
> > > to
> > > > > > withCredentials=false, which is the default case if I'm reading the
> spec
> > > > > > correctly.
> > > > > 
> > > > > That would solve this particular CORS TC not re-loading/fetching the
2nd
> > > > > withCredentials=true request. But if it had instead issued a pair of
> > > > > withCredentials=false requests or been requests of the non-CORS kind
> > > > > (cf the CL tests), it wouldn't apply.
> > > > 
> > > > Ok, so it appears there are multiple different mistakes the current code
> > > makes.
> > > > I can't say I'm surprised :)
> > > > 
> > > > Would it cover these cases if we add 2 new rules: never reuse a resource
> if
> > > (1)
> > > > the url includes a query, (2) either the existing or the new request is
> > > > cross-origin?
> > > 
> > > Rule 1) would take care of an important class (for GET); RFC 2616 calls
that
> > > one out as something to avoid (cf 13.9.) That's not specifically for XHR
> > > though, and implementations do go a bit further there (see attached bug
for
> > > some data points.)
> > > 
> > > Let me think about 2) overnight. My immediate reaction is that the
> > cross-origin
> > > property of a request is orthogonal to its "cacheability".
> > 
> > Ah, looked into these TCs some more (with the above in mind); the reason why
> > they're not reloading is due to this condition in
> determineRevalidationPolicy()
> > 
> >  if (document() && !document()->loadEventFinished() &&
> > m_validatedURLs.contains(existingResource->url()))
> >      return Use;
> > 
> > as the TCs run to completion before onload is dispatched. What is its
purpose
> > here? And does it make sense to have in place for all resource types?
> > 
> > If that check is weakened a bit, the revalidation predicate falls into
> > Resource::isExpired() eventually, which conservatively assumes a freshness
> > of 0 for resources lacking other cache control information. (=> Reload.)
> 
> I think that check dates back to a time when XHRs went through a separate code
> path and never touched the cache. Before XHRs were cached, only "true"
> subresources were put in the MemoryCache (image, scripts, etc.), and those
types
> don't have to support arbitrary headers. The assumption was that if the same
URL
> was requested more than once, we could safely assume that reuse was permitted,
> but that clearly isn't a safe assumption for XHRs. I wonder if we should add a
> "type != Resource::RawResource" check to that if() statement.

Thanks, great explanation of why it is so. Treating XHR as part of this does
seem a step too far -- I've uploaded a patchset with what you suggest (reverting
the other code skirmishes here in the process..sorry about the untidy patchset
sequence.)

[Nate Chapin, 2013-11-11 22:40:48]

LGTM

Thanks for iterating on this!

[sof, 2013-11-11 22:54:11]

On 2013/11/11 22:40:48, Nate Chapin wrote:
> LGTM
> 
> Thanks for iterating on this!

Ditto (and for the patience :) )

[commit-bot: I haz the power, 2013-11-11 22:54:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/59043006/240001

[commit-bot: I haz the power, 2013-11-12 01:10:34]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[sof, 2013-11-12 14:47:59]

Had to reduce the scope of the test to try to avoid running into a timeout
(triggered for a Win builder). Still looking ok?

(Sorry, don't have trybot access to expedite testing.)

[Nate Chapin, 2013-11-12 17:07:18]

On 2013/11/12 14:47:59, sof wrote:
> Had to reduce the scope of the test to try to avoid running into a timeout
> (triggered for a Win builder). Still looking ok?
> 
> (Sorry, don't have trybot access to expedite testing.)

Sorry, I didn't realize that. Still LGTM

[commit-bot: I haz the power, 2013-11-12 18:07:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/59043006/250005

[commit-bot: I haz the power, 2013-11-12 19:50:40]

Change committed as 161846

[sof, 2013-11-12 19:58:19]

On 2013/11/12 17:07:18, Nate Chapin wrote:
> On 2013/11/12 14:47:59, sof wrote:
> > Had to reduce the scope of the test to try to avoid running into a timeout
> > (triggered for a Win builder). Still looking ok?
> > 
> > (Sorry, don't have trybot access to expedite testing.)
> 
> Sorry, I didn't realize that. Still LGTM

Thanks again, japhet.

I don't quite understand what issue the test ran into on that builder; will keep
an eye on its status.