Reset ResourceLoader::m_loader in the pre-finalizer

Reset ResourceLoader::m_loader in the pre-finalizer

A ResourceLoader is held as a raw-ptr in WebURLLoaderImpl and hence it must
cancel the WebURLLoaderImpl when it's destroyed. We thought
blink::Resource::allClientsAndObserversRemoved did the job but it's wrong:
the cancel timer doesn't work when the resource is about to be destroyed.

BUG=675947
R=hiroshige@chromium.org, japhet@chromium.org

Review-Url: https://codereview.chromium.org/2625823003
Cr-Commit-Position: refs/heads/master@{#443167}
Committed: https://chromium.googlesource.com/chromium/src/+/6e6d7a20f7cb4b23ff305b16579faf94f7ddf992

[yhirano, 2017-01-11 02:26:50]

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

[commit-bot: I haz the power, 2017-01-11 02:27:43]

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

[yhirano, 2017-01-11 02:30:32]

PTAL

See also: https://codereview.chromium.org/2619513002/ and
https://codereview.chromium.org/2615993005/.

[kinuko, 2017-01-11 04:56:37]

We're also seeing some crashes for accessing WebURLLoaderImpl::Context::client_
after it's poisoned, this likely fixes it too?  (lgtm)

[commit-bot: I haz the power, 2017-01-11 05:07:36]

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

[commit-bot: I haz the power, 2017-01-11 05:07:37]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-01-11 09:54:52]

On 2017/01/11 04:56:37, kinuko wrote:
> We're also seeing some crashes for accessing
WebURLLoaderImpl::Context::client_
> after it's poisoned, this likely fixes it too?  (lgtm)

Thank you. yes, I expect so. But I'm not sure this is the right fix, so I'm
asking for experts' comments.

[hiroshige, 2017-01-11 23:50:53]

On 2017/01/11 09:54:52, yhirano wrote:
> On 2017/01/11 04:56:37, kinuko wrote:
> > We're also seeing some crashes for accessing
> WebURLLoaderImpl::Context::client_
> > after it's poisoned, this likely fixes it too?  (lgtm)
> 
> Thank you. yes, I expect so. But I'm not sure this is the right fix, so I'm
> asking for experts' comments.

Looks good, if cancelling WebURLLoader in a prefinalizer is safe (I expect so).

> when the resource is about to be destroyed
In this case, are Resource, ResourceLoader and ResourceFetcher all destroyed?
(And thus we don't need to call ResourceLoader::cancel() in dispose()?)

Also, should we allow ResourceLoader to be destroyed without canceling before
(like in Patch Set 1)?
  If so, we can remove some explicit cancellation of ResourceLoader in some unit
tests (not in this CL).
Or should we still require ResourceLoader to be cancelled or its cancellation is
scheduled before its destruction (by adding DCHECK()s)?
I slightly prefer the latter to keep the "cancel explicitly before destruction"
rule also in ResourceLoader, but not so sure.

Adding a unit test is preferrable (perhaps in a follow-up CL, not to block this
CL as this CL is expected to be merged to beta).

[yhirano, 2017-01-12 04:11:48]

On 2017/01/11 23:50:53, hiroshige wrote:
> On 2017/01/11 09:54:52, yhirano wrote:
> > On 2017/01/11 04:56:37, kinuko wrote:
> > > We're also seeing some crashes for accessing
> > WebURLLoaderImpl::Context::client_
> > > after it's poisoned, this likely fixes it too?  (lgtm)
> > 
> > Thank you. yes, I expect so. But I'm not sure this is the right fix, so I'm
> > asking for experts' comments.
> 
> Looks good, if cancelling WebURLLoader in a prefinalizer is safe (I expect
so).
> 
> > when the resource is about to be destroyed
> In this case, are Resource, ResourceLoader and ResourceFetcher all destroyed?
> (And thus we don't need to call ResourceLoader::cancel() in dispose()?)
> 
Yes.

> Also, should we allow ResourceLoader to be destroyed without canceling before
> (like in Patch Set 1)?
>   If so, we can remove some explicit cancellation of ResourceLoader in some
unit
> tests (not in this CL).
> Or should we still require ResourceLoader to be cancelled or its cancellation
is
> scheduled before its destruction (by adding DCHECK()s)?
> I slightly prefer the latter to keep the "cancel explicitly before
destruction"
> rule also in ResourceLoader, but not so sure.
> 
> Adding a unit test is preferrable (perhaps in a follow-up CL, not to block
this
> CL as this CL is expected to be merged to beta).

Thanks.

ResourceLoader::cancel and ResourceLoader::dispose is different. The latter
doesn't report the cancellation to anyone, so I think using cancel is generally
more preferable. Calling ResourceLoader::dispose synchronously and reporting it
later might be good in Resource::allClientsAndObserversRemoved but I'm not sure.

[yhirano, 2017-01-12 04:11:54]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2017-01-12 04:12: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, 2017-01-12 06:19:55]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1484194314100000, "parent_rev":
"0fbb8cde3b9aa70029c2ceb9b492b446675f1877", "commit_rev":
"6e6d7a20f7cb4b23ff305b16579faf94f7ddf992"}

[commit-bot: I haz the power, 2017-01-12 06:21:00]

Description was changed from

==========
Reset ResourceLoader::m_loader in the pre-finalizer

A ResourceLoader is held as a raw-ptr in WebURLLoaderImpl and hence it must
cancel the WebURLLoaderImpl when it's destroyed. We thought
blink::Resource::allClientsAndObserversRemoved did the job but it's wrong:
the cancel timer doesn't work when the resource is about to be destroyed.

BUG=675947
R=hiroshige@chromium.org, japhet@chromium.org
==========

to

==========
Reset ResourceLoader::m_loader in the pre-finalizer

A ResourceLoader is held as a raw-ptr in WebURLLoaderImpl and hence it must
cancel the WebURLLoaderImpl when it's destroyed. We thought
blink::Resource::allClientsAndObserversRemoved did the job but it's wrong:
the cancel timer doesn't work when the resource is about to be destroyed.

BUG=675947
R=hiroshige@chromium.org, japhet@chromium.org

Review-Url: https://codereview.chromium.org/2625823003
Cr-Commit-Position: refs/heads/master@{#443167}
Committed:
https://chromium.googlesource.com/chromium/src/+/6e6d7a20f7cb4b23ff305b16579f...
==========

[commit-bot: I haz the power, 2017-01-12 06:21:04]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/6e6d7a20f7cb4b23ff305b16579f...