Do not revalidate when ResourceLoaderOptions.synchronousPolicy is different

Do not revalidate when ResourceLoaderOptions.synchronousPolicy is different

This CL adds a check of synchronousPolicy to
ResourceLoaderOptions::canReuseRequest() to avoid Resources to be reused
between sync and async requests.
This CL also removes a condition from
ResourceFetcher::determineRevalidationPolicy() that is covered by the newly
added check.

CachingCorrectnessTest is updated so that it doesn't use synchronous
RawResource requests.

BUG=652172
Committed: https://crrev.com/733a476495c43c1f472e5770c9d8aeeb94c5caca
Cr-Commit-Position: refs/heads/master@{#422725}

[hiroshige, 2016-10-03 07:34:12]

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

[commit-bot: I haz the power, 2016-10-03 07:34:28]

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

[hiroshige, 2016-10-03 07:48:53]

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

[commit-bot: I haz the power, 2016-10-03 07:49:09]

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-10-03 07:53:16]

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

[commit-bot: I haz the power, 2016-10-03 07:53:17]

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

[hiroshige, 2016-10-03 09:02:26]

Description was changed from

==========
Do not revalidate when ResourceLoaderOptions.synchronousPolicy is different

BUG=652172
==========

to

==========
Do not revalidate when ResourceLoaderOptions.synchronousPolicy is different

CachingCorrectnessTest is updated so that it doesn't use synchronous
RawResource requests.

BUG=652172
==========

[hiroshige, 2016-10-03 09:03:27]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org, tyoshino@chromium.org, yhirano@chromium.org

[hiroshige, 2016-10-03 09:03:28]

Could you take a look?

Another option would be to update/overwrite ResourceLoaderOptions on
revalidation.

[ThomasJones, 2016-10-03 09:14:04]

On 2016/10/03 09:03:28, hiroshige wrote:
> Could you take a look?
> 
> Another option would be to update/overwrite ResourceLoaderOptions on
> revalidation.

I came  across the same issues when I was working on my project at
http://essaydune.com/buy-essay. So how can I overwrite ResourceLoaderOptions on
revalidation? If I cannot to it myself, who may I refer to get some help with
that? This really affects the efficiency of my working progress.

[hiroshige, 2016-10-03 09:21:19]

On 2016/10/03 09:14:04, ThomasJones wrote:
> On 2016/10/03 09:03:28, hiroshige wrote:
> > Could you take a look?
> > 
> > Another option would be to update/overwrite ResourceLoaderOptions on
> > revalidation.
> 
> I came  across the same issues when I was working on my project at
> http://essaydune.com/buy-essay. So how can I overwrite ResourceLoaderOptions
on
> revalidation? If I cannot to it myself, who may I refer to get some help with
> that? This really affects the efficiency of my working progress.

Probably your case is a different issue from this CL, if you want to overwrite
ResourceLoaderOptions in general.
Could you file a new bug at https://crbug.com/, cc/assign to me, and provide
your case in more detail?

Thanks!

[yhirano, 2016-10-03 10:53:55]

lgtm

[yhirano, 2016-10-03 11:18:31]

By the way, can we remove

  // Don't try to reuse an in-progress async request for a new sync request.
  if (fetchRequest.options().synchronousPolicy == RequestSynchronously &&
      existingResource->isLoading())
    return Reload;

from ResourceFetcher::determineRevalidationPolicy?

[hiroshige, 2016-10-03 12:53:53]

Description was changed from

==========
Do not revalidate when ResourceLoaderOptions.synchronousPolicy is different

CachingCorrectnessTest is updated so that it doesn't use synchronous
RawResource requests.

BUG=652172
==========

to

==========
Do not revalidate when ResourceLoaderOptions.synchronousPolicy is different

This CL adds a check of synchronousPolicy to
ResourceLoaderOptions::canReuseRequest() to avoid Resources to be reused
between sync and async requests.
This CL also removes a condition from
ResourceFetcher::determineRevalidationPolicy() that is covered by the newly
added check.

CachingCorrectnessTest is updated so that it doesn't use synchronous
RawResource requests.

BUG=652172
==========

[hiroshige, 2016-10-03 12:54:48]

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

[hiroshige, 2016-10-03 12:54:51]

On 2016/10/03 11:18:31, yhirano wrote:
> By the way, can we remove
> 
>   // Don't try to reuse an in-progress async request for a new sync request.
>   if (fetchRequest.options().synchronousPolicy == RequestSynchronously &&
>       existingResource->isLoading())
>     return Reload;
> 
> from ResourceFetcher::determineRevalidationPolicy?

Done in Patch Set 3.

[commit-bot: I haz the power, 2016-10-03 12:54: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, 2016-10-03 13:03:30]

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

[commit-bot: I haz the power, 2016-10-03 13:03:31]

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

[hiroshige, 2016-10-04 05:37:20]

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

[commit-bot: I haz the power, 2016-10-04 05:37:33]

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

[hiroshige, 2016-10-04 05:43:40]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2016-10-04 05:43:45]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2016-10-04 05:43:45]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2391523002/#ps60001
(title: "git cl format")

[commit-bot: I haz the power, 2016-10-04 05:44:03]

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-10-04 06:55:44]

Description was changed from

==========
Do not revalidate when ResourceLoaderOptions.synchronousPolicy is different

This CL adds a check of synchronousPolicy to
ResourceLoaderOptions::canReuseRequest() to avoid Resources to be reused
between sync and async requests.
This CL also removes a condition from
ResourceFetcher::determineRevalidationPolicy() that is covered by the newly
added check.

CachingCorrectnessTest is updated so that it doesn't use synchronous
RawResource requests.

BUG=652172
==========

to

==========
Do not revalidate when ResourceLoaderOptions.synchronousPolicy is different

This CL adds a check of synchronousPolicy to
ResourceLoaderOptions::canReuseRequest() to avoid Resources to be reused
between sync and async requests.
This CL also removes a condition from
ResourceFetcher::determineRevalidationPolicy() that is covered by the newly
added check.

CachingCorrectnessTest is updated so that it doesn't use synchronous
RawResource requests.

BUG=652172
==========

[commit-bot: I haz the power, 2016-10-04 06:55:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-04 06:58:14]

Description was changed from

==========
Do not revalidate when ResourceLoaderOptions.synchronousPolicy is different

This CL adds a check of synchronousPolicy to
ResourceLoaderOptions::canReuseRequest() to avoid Resources to be reused
between sync and async requests.
This CL also removes a condition from
ResourceFetcher::determineRevalidationPolicy() that is covered by the newly
added check.

CachingCorrectnessTest is updated so that it doesn't use synchronous
RawResource requests.

BUG=652172
==========

to

==========
Do not revalidate when ResourceLoaderOptions.synchronousPolicy is different

This CL adds a check of synchronousPolicy to
ResourceLoaderOptions::canReuseRequest() to avoid Resources to be reused
between sync and async requests.
This CL also removes a condition from
ResourceFetcher::determineRevalidationPolicy() that is covered by the newly
added check.

CachingCorrectnessTest is updated so that it doesn't use synchronous
RawResource requests.

BUG=652172

Committed: https://crrev.com/733a476495c43c1f472e5770c9d8aeeb94c5caca
Cr-Commit-Position: refs/heads/master@{#422725}
==========

[commit-bot: I haz the power, 2016-10-04 06:58:15]

Patchset 4 (id:??) landed as
https://crrev.com/733a476495c43c1f472e5770c9d8aeeb94c5caca
Cr-Commit-Position: refs/heads/master@{#422725}

[pcc1, 2016-10-05 00:10:43]

On 2016/10/04 06:58:15, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/733a476495c43c1f472e5770c9d8aeeb94c5caca
> Cr-Commit-Position: refs/heads/master@{#422725}

I believe this change caused a regression on the "CFI Linux ToT" bot as a result
of introducing a bad cast. See e.g.
https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux%20ToT/builds/4446

Specifically, each of the failing tests contains code that looks like this:

  Resource* firstResource =
      Resource::create(ResourceRequest(redirectUrl), Resource::Raw);
  [...]
  Resource* fetched = fetch();
  EXPECT_EQ(firstResource, fetched);

This CL changed CachingCorrectnessTest::fetch() to call RawResource::fetch()
instead of RawResource::fetchSynchronously(). The former function casts what
ends up being firstResource (which is a Resource) from Resource to RawResource
(see toRawResource), which fails the bad cast check.

Can you please take a look?

[hiroshige, 2016-10-05 04:48:36]

On 2016/10/05 00:10:43, pcc1 wrote:
> On 2016/10/04 06:58:15, commit-bot: I haz the power wrote:
> > Patchset 4 (id:??) landed as
> > https://crrev.com/733a476495c43c1f472e5770c9d8aeeb94c5caca
> > Cr-Commit-Position: refs/heads/master@{#422725}
> 
> I believe this change caused a regression on the "CFI Linux ToT" bot as a
result
> of introducing a bad cast. See e.g.
>
https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux%20ToT/builds/4446
> 
> Specifically, each of the failing tests contains code that looks like this:
> 
>   Resource* firstResource =
>       Resource::create(ResourceRequest(redirectUrl), Resource::Raw);
>   [...]
>   Resource* fetched = fetch();
>   EXPECT_EQ(firstResource, fetched);
> 
> This CL changed CachingCorrectnessTest::fetch() to call RawResource::fetch()
> instead of RawResource::fetchSynchronously(). The former function casts what
> ends up being firstResource (which is a Resource) from Resource to RawResource
> (see toRawResource), which fails the bad cast check.
> 
> Can you please take a look?

Er, Resource::create() creates Resource (not RawResource) with type
Resource::Raw. MemoryCache etc. assumes Resource with type Resource::Raw is
RawResource, so causing wrong cast. Thanks, fixing.

[hiroshige, 2016-10-06 07:43:58]

On 2016/10/05 04:48:36, hiroshige wrote:
> On 2016/10/05 00:10:43, pcc1 wrote:
> > On 2016/10/04 06:58:15, commit-bot: I haz the power wrote:
> > > Patchset 4 (id:??) landed as
> > > https://crrev.com/733a476495c43c1f472e5770c9d8aeeb94c5caca
> > > Cr-Commit-Position: refs/heads/master@{#422725}
> > 
> > I believe this change caused a regression on the "CFI Linux ToT" bot as a
> result
> > of introducing a bad cast. See e.g.
> >
>
https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux%20ToT/builds/4446
> > 
> > Specifically, each of the failing tests contains code that looks like this:
> > 
> >   Resource* firstResource =
> >       Resource::create(ResourceRequest(redirectUrl), Resource::Raw);
> >   [...]
> >   Resource* fetched = fetch();
> >   EXPECT_EQ(firstResource, fetched);
> > 
> > This CL changed CachingCorrectnessTest::fetch() to call RawResource::fetch()
> > instead of RawResource::fetchSynchronously(). The former function casts what
> > ends up being firstResource (which is a Resource) from Resource to
RawResource
> > (see toRawResource), which fails the bad cast check.
> > 
> > Can you please take a look?
> 
> Er, Resource::create() creates Resource (not RawResource) with type
> Resource::Raw. MemoryCache etc. assumes Resource with type Resource::Raw is
> RawResource, so causing wrong cast. Thanks, fixing.

Tracked by https://crbug.com/652999, landed a fix, waiting for bot results.

[pcc1, 2016-10-06 17:18:34]

On 2016/10/06 07:43:58, hiroshige wrote:
> On 2016/10/05 04:48:36, hiroshige wrote:
> > On 2016/10/05 00:10:43, pcc1 wrote:
> > > On 2016/10/04 06:58:15, commit-bot: I haz the power wrote:
> > > > Patchset 4 (id:??) landed as
> > > > https://crrev.com/733a476495c43c1f472e5770c9d8aeeb94c5caca
> > > > Cr-Commit-Position: refs/heads/master@{#422725}
> > > 
> > > I believe this change caused a regression on the "CFI Linux ToT" bot as a
> > result
> > > of introducing a bad cast. See e.g.
> > >
> >
>
https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux%20ToT/builds/4446
> > > 
> > > Specifically, each of the failing tests contains code that looks like
this:
> > > 
> > >   Resource* firstResource =
> > >       Resource::create(ResourceRequest(redirectUrl), Resource::Raw);
> > >   [...]
> > >   Resource* fetched = fetch();
> > >   EXPECT_EQ(firstResource, fetched);
> > > 
> > > This CL changed CachingCorrectnessTest::fetch() to call
RawResource::fetch()
> > > instead of RawResource::fetchSynchronously(). The former function casts
what
> > > ends up being firstResource (which is a Resource) from Resource to
> RawResource
> > > (see toRawResource), which fails the bad cast check.
> > > 
> > > Can you please take a look?
> > 
> > Er, Resource::create() creates Resource (not RawResource) with type
> > Resource::Raw. MemoryCache etc. assumes Resource with type Resource::Raw is
> > RawResource, so causing wrong cast. Thanks, fixing.
> 
> Tracked by https://crbug.com/652999, landed a fix, waiting for bot results.

Bot is green now, thanks Hiroshige!

[awesome.agouti, 2017-01-13 12:04:25]

On 2016/10/06 17:18:34, pcc1 wrote:
> On 2016/10/06 07:43:58, hiroshige wrote:
> > On 2016/10/05 04:48:36, hiroshige wrote:
> > > On 2016/10/05 00:10:43, pcc1 wrote:
> > > > On 2016/10/04 06:58:15, commit-bot: I haz the power wrote:
> > > > > Patchset 4 (id:??) landed as
> > > > > https://crrev.com/733a476495c43c1f472e5770c9d8aeeb94c5caca
> > > > > Cr-Commit-Position: refs/heads/master@{#422725}
> > > > 
> > > > I believe this change caused a regression on the "CFI Linux ToT" bot as
a
> > > result
> > > > of introducing a bad cast. See e.g.
> > > >
> > >
> >
>
https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux%20ToT/builds/4446
> > > > 
> > > > Specifically, each of the failing tests contains code that looks like
> this:
> > > > 
> > > >   Resource* firstResource =
> > > >       Resource::create(ResourceRequest(redirectUrl), Resource::Raw);
> > > >   [...]
> > > >   Resource* fetched = fetch();
> > > >   EXPECT_EQ(firstResource, fetched);
> > > > 
> > > > This CL changed CachingCorrectnessTest::fetch() to call
> RawResource::fetch()
> > > > instead of RawResource::fetchSynchronously(). The former function casts
> what
> > > > ends up being firstResource (which is a Resource) from Resource to
> > RawResource
> > > > (see toRawResource), which fails the bad cast check.
> > > > 
> > > > Can you please take a look?
> > > 
> > > Er, Resource::create() creates Resource (not RawResource) with type
> > > Resource::Raw. MemoryCache etc. assumes Resource with type Resource::Raw
is
> > > RawResource, so causing wrong cast. Thanks, fixing.
> > 
> > Tracked by https://crbug.com/652999, landed a fix, waiting for bot results.
> 
> Bot is green now, thanks Hiroshige!


> Could you take a look?
> 
> Another option would be to update/overwrite ResourceLoaderOptions on
> revalidation.

I came  across the same issues when I was working on my project at
http://www.mobi-spy.net/. So how can I overwrite ResourceLoaderOptions on
revalidation? If I cannot to it myself, who may I refer to get some help with
that? This really affects the efficiency of my working progress.

[hiroshige, 2017-01-13 17:02:32]

On 2017/01/13 12:04:25, awesome.agouti wrote:
> On 2016/10/06 17:18:34, pcc1 wrote:
> > On 2016/10/06 07:43:58, hiroshige wrote:
> > > On 2016/10/05 04:48:36, hiroshige wrote:
> > > > On 2016/10/05 00:10:43, pcc1 wrote:
> > > > > On 2016/10/04 06:58:15, commit-bot: I haz the power wrote:
> > > > > > Patchset 4 (id:??) landed as
> > > > > > https://crrev.com/733a476495c43c1f472e5770c9d8aeeb94c5caca
> > > > > > Cr-Commit-Position: refs/heads/master@{#422725}
> > > > > 
> > > > > I believe this change caused a regression on the "CFI Linux ToT" bot
as
> a
> > > > result
> > > > > of introducing a bad cast. See e.g.
> > > > >
> > > >
> > >
> >
>
https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux%20ToT/builds/4446
> > > > > 
> > > > > Specifically, each of the failing tests contains code that looks like
> > this:
> > > > > 
> > > > >   Resource* firstResource =
> > > > >       Resource::create(ResourceRequest(redirectUrl), Resource::Raw);
> > > > >   [...]
> > > > >   Resource* fetched = fetch();
> > > > >   EXPECT_EQ(firstResource, fetched);
> > > > > 
> > > > > This CL changed CachingCorrectnessTest::fetch() to call
> > RawResource::fetch()
> > > > > instead of RawResource::fetchSynchronously(). The former function
casts
> > what
> > > > > ends up being firstResource (which is a Resource) from Resource to
> > > RawResource
> > > > > (see toRawResource), which fails the bad cast check.
> > > > > 
> > > > > Can you please take a look?
> > > > 
> > > > Er, Resource::create() creates Resource (not RawResource) with type
> > > > Resource::Raw. MemoryCache etc. assumes Resource with type Resource::Raw
> is
> > > > RawResource, so causing wrong cast. Thanks, fixing.
> > > 
> > > Tracked by https://crbug.com/652999, landed a fix, waiting for bot
results.
> > 
> > Bot is green now, thanks Hiroshige!
> 
> 
> > Could you take a look?
> > 
> > Another option would be to update/overwrite ResourceLoaderOptions on
> > revalidation.
> 
> I came  across the same issues when I was working on my project at
> http://www.mobi-spy.net/. So how can I overwrite ResourceLoaderOptions on
> revalidation? If I cannot to it myself, who may I refer to get some help with
> that? This really affects the efficiency of my working progress.

First, I'd like to know your need more in detail, so could you create a new
entry on https://crbug.com/?
- Do you mean overwriting synchronousPolicy, i.e. reusing Resource on
MemoryCache among sync and async requests?
- What problematic behavior does the issue cause? Is it for XHRs?
- Is serving from disk cache acceptable?

Basically, I'd like to avoid rewriting/overwriting ResourceLoaderOptions because
it is hard and/or complicates the logic at the risk of new security bugs (and
sync XHR on the main thread is deprecated).
However, there might be another way of fixes, depending on your situation.