ResourceFetcher: fix an assertion failure on defered resource revalidation

ResourceFetcher: fix an assertion failure on defered resource revalidation

On revalidating resources, e.g. for page reloads, if two initiators
exist and the loading is defered, both initiators get 'Revalidate'
revalidation policy in ResourceFetcher::requestResource() then it
results in an assertion failure to initialize the resource as a cache
validator.

BUG=616358
Committed: https://crrev.com/11fe1836e4c10b169106f39839504517e0d68a08
Cr-Commit-Position: refs/heads/master@{#398468}

[Takashi Toyoshima, 2016-06-06 13:14:19]

https://codereview.chromium.org/2039893002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2039893002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:806:
DCHECK(existingResource->stillNeedsLoad());
existingResource->canUseCacheValidator() ensures the Status is not Pending, and
not Load/DecodeError. Also, we already have a check that confirms
m_revalidatingRequest.isNull() is true on changing the Status to Cached. So,
here the Status should be always NotStarted until we make another change that
breaks this assumption.

[Takashi Toyoshima, 2016-06-06 13:14:49]

Description was changed from

==========
ResourceFetcher: fix an assertion failure on defered resource revalidation

On revalidating resources, e.g. for page reloads, if two initiators
exist and the loading is defered, both initiators get 'Revalidate'
revalidation policy in ResourceFetcher::requestResource() then it
results in an assertion failure to initialize the resource as a cache
validator.

BUG=616358
==========

to

==========
ResourceFetcher: fix an assertion failure on defered resource revalidation

On revalidating resources, e.g. for page reloads, if two initiators
exist and the loading is defered, both initiators get 'Revalidate'
revalidation policy in ResourceFetcher::requestResource() then it
results in an assertion failure to initialize the resource as a cache
validator.

BUG=616358
==========

[Takashi Toyoshima, 2016-06-06 13:14:49]

toyoshim@chromium.org changed reviewers:
+ japhet@chromium.org

[Yoav Weiss, 2016-06-06 14:51:26]

On 2016/06/06 13:14:19, Takashi Toyoshima wrote:
>
https://codereview.chromium.org/2039893002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/2039893002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:806:
> DCHECK(existingResource->stillNeedsLoad());
> existingResource->canUseCacheValidator() ensures the Status is not Pending,
and
> not Load/DecodeError. Also, we already have a check that confirms
> m_revalidatingRequest.isNull() is true on changing the Status to Cached. So,
> here the Status should be always NotStarted until we make another change that
> breaks this assumption.

Can you please add a test (a test case that previously caused the assertion to
trigger)?

[Takashi Toyoshima, 2016-06-07 05:48:52]

I added unit tests at Patch Set 2.

[Takashi Toyoshima, 2016-06-07 09:05:37]

OK, memory leak in the test should be fixed.
Can you take a look at the patch set 3?

[Yoav Weiss, 2016-06-07 12:55:50]

On 2016/06/07 09:05:37, Takashi Toyoshima wrote:
> OK, memory leak in the test should be fixed.
> Can you take a look at the patch set 3?

LGTM

[Yoav Weiss, 2016-06-07 12:57:08]

On 2016/06/07 12:55:50, Yoav Weiss wrote:
> On 2016/06/07 09:05:37, Takashi Toyoshima wrote:
> > OK, memory leak in the test should be fixed.
> > Can you take a look at the patch set 3?
> 
> LGTM

And thanks for adding the test. It clarifies the scenario in question quite
nicely, as well as making sure the same issue won't come back to haunt us in the
future :)

[Takashi Toyoshima, 2016-06-08 02:26:22]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2016-06-08 02:26:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2039893002/40001

[commit-bot: I haz the power, 2016-06-08 03:12:10]

Description was changed from

==========
ResourceFetcher: fix an assertion failure on defered resource revalidation

On revalidating resources, e.g. for page reloads, if two initiators
exist and the loading is defered, both initiators get 'Revalidate'
revalidation policy in ResourceFetcher::requestResource() then it
results in an assertion failure to initialize the resource as a cache
validator.

BUG=616358
==========

to

==========
ResourceFetcher: fix an assertion failure on defered resource revalidation

On revalidating resources, e.g. for page reloads, if two initiators
exist and the loading is defered, both initiators get 'Revalidate'
revalidation policy in ResourceFetcher::requestResource() then it
results in an assertion failure to initialize the resource as a cache
validator.

BUG=616358
==========

[commit-bot: I haz the power, 2016-06-08 03:12:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-08 03:15:06]

Description was changed from

==========
ResourceFetcher: fix an assertion failure on defered resource revalidation

On revalidating resources, e.g. for page reloads, if two initiators
exist and the loading is defered, both initiators get 'Revalidate'
revalidation policy in ResourceFetcher::requestResource() then it
results in an assertion failure to initialize the resource as a cache
validator.

BUG=616358
==========

to

==========
ResourceFetcher: fix an assertion failure on defered resource revalidation

On revalidating resources, e.g. for page reloads, if two initiators
exist and the loading is defered, both initiators get 'Revalidate'
revalidation policy in ResourceFetcher::requestResource() then it
results in an assertion failure to initialize the resource as a cache
validator.

BUG=616358

Committed: https://crrev.com/11fe1836e4c10b169106f39839504517e0d68a08
Cr-Commit-Position: refs/heads/master@{#398468}
==========

[commit-bot: I haz the power, 2016-06-08 03:15:07]

Patchset 3 (id:??) landed as
https://crrev.com/11fe1836e4c10b169106f39839504517e0d68a08
Cr-Commit-Position: refs/heads/master@{#398468}