DocumentThreadableLoader: Add guards for sync notifyFinished() in setResource()

DocumentThreadableLoader: Add guards for sync notifyFinished() in setResource()

In loadRequest(), setResource() can call clear() synchronously:
  DocumentThreadableLoader::clear()
  DocumentThreadableLoader::handleError()
  Resource::didAddClient()
  RawResource::didAddClient()
and thus |m_client| can be null while resource() isn't null after setResource(),
causing crashes (Issue 595964).

This CL checks whether |*this| is destructed and
whether |m_client| is null after setResource().

BUG=595964
Committed: https://crrev.com/2571533bbb5b554ff47205c8ef1513ccc0817c3e
Cr-Commit-Position: refs/heads/master@{#391001}

[hiroshige, 2016-04-19 05:04:50]

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

[commit-bot: I haz the power, 2016-04-19 05:05:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1902683002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1902683002/1

[hiroshige, 2016-04-19 06:10:35]

Description was changed from

==========
DocumentThreadableLoader: Add guards for sync notifyFinished() in setResource()

setResource()

BUG=595964
==========

to

==========
DocumentThreadableLoader: Add guards for sync notifyFinished() in setResource()

setResource() can call clear() synchronously:
  DocumentThreadableLoader::clear()
  DocumentThreadableLoader::handleError()
  Resource::didAddClient()
  RawResource::didAddClient()
and thus |m_client| can be null while resource() isn't null after setResource(),
causing crashes (Issue 595964).

This CL checks whether |*this| is destructed and
whether |m_client| is null after setResource().

BUG=595964
==========

[hiroshige, 2016-04-19 06:10:36]

hiroshige@chromium.org changed reviewers:
+ tyoshino@chromium.org

[hiroshige, 2016-04-19 06:11:18]

PTAL.
(this should be merged to at least M-51 beta)

[tyoshino (SeeGerritForStatus), 2016-04-19 06:27:44]

lgtm

https://codereview.chromium.org/1902683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1902683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:863: return;
add comment to note that we need to call clear() for the case fetch.*() method
failed but in case clear() is called inside setResource() synchronously,
m_client is already cleared and therefore we need to return here.

[tyoshino (SeeGerritForStatus), 2016-04-19 06:29:00]

https://codereview.chromium.org/1902683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1902683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:863: return;
On 2016/04/19 06:27:44, tyoshino wrote:
> add comment to note that we need to call clear() for the case fetch.*() method
> failed but in case clear() is called inside setResource() synchronously,
> m_client is already cleared and therefore we need to return here.

oh, basically the same as what you've put in the description.

[commit-bot: I haz the power, 2016-04-19 06:32:39]

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

[commit-bot: I haz the power, 2016-04-19 06:32:40]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-04-19 06:34:50]

Description was changed from

==========
DocumentThreadableLoader: Add guards for sync notifyFinished() in setResource()

setResource() can call clear() synchronously:
  DocumentThreadableLoader::clear()
  DocumentThreadableLoader::handleError()
  Resource::didAddClient()
  RawResource::didAddClient()
and thus |m_client| can be null while resource() isn't null after setResource(),
causing crashes (Issue 595964).

This CL checks whether |*this| is destructed and
whether |m_client| is null after setResource().

BUG=595964
==========

to

==========
DocumentThreadableLoader: Add guards for sync notifyFinished() in setResource()

In loadRequest(), setResource() can call clear() synchronously:
  DocumentThreadableLoader::clear()
  DocumentThreadableLoader::handleError()
  Resource::didAddClient()
  RawResource::didAddClient()
and thus |m_client| can be null while resource() isn't null after setResource(),
causing crashes (Issue 595964).

This CL checks whether |*this| is destructed and
whether |m_client| is null after setResource().

BUG=595964
==========

[hiroshige, 2016-04-19 06:40:47]

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

[hiroshige, 2016-04-19 06:40:48]

+japhet@, could you take a look as a core/loader OWNER?

https://codereview.chromium.org/1902683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1902683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:863: return;
On 2016/04/19 06:29:00, tyoshino wrote:
> On 2016/04/19 06:27:44, tyoshino wrote:
> > add comment to note that we need to call clear() for the case fetch.*()
method
> > failed but in case clear() is called inside setResource() synchronously,
> > m_client is already cleared and therefore we need to return here.
> 
> oh, basically the same as what you've put in the description.

Done.

[tyoshino (SeeGerritForStatus), 2016-04-19 08:31:32]

https://codereview.chromium.org/1902683002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/1902683002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:863: return;
On 2016/04/19 06:40:48, hiroshige wrote:
> On 2016/04/19 06:29:00, tyoshino wrote:
> > On 2016/04/19 06:27:44, tyoshino wrote:
> > > add comment to note that we need to call clear() for the case fetch.*()
> method
> > > failed but in case clear() is called inside setResource() synchronously,
> > > m_client is already cleared and therefore we need to return here.
> > 
> > oh, basically the same as what you've put in the description.
> 
> Done.

Thanks. LGTM

[hiroshige, 2016-04-21 16:08:09]

japhet@, could you take a look?

[hiroshige, 2016-04-26 09:49:19]

friendly ping.

[hiroshige, 2016-05-02 09:19:53]

hiroshige@chromium.org changed reviewers:
+ mkwst@chromium.org

[hiroshige, 2016-05-02 09:19:53]

+mkwst@ as another core/loader owner. PTAL.

[Mike West, 2016-05-02 09:32:01]

LGTM. Is there a test you can add that would verify the behavior?

[hiroshige, 2016-05-02 10:02:47]

On 2016/05/02 09:32:01, Mike West wrote:
> LGTM. Is there a test you can add that would verify the behavior?

Clusterfuzz has a reproducible case, but I don't want to expose it until the fix
is fully merged.
So far I couldn't create a unit test that exposes this bug.

[hiroshige, 2016-05-02 10:02:58]

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

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1902683002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1902683002/20001

[commit-bot: I haz the power, 2016-05-02 11:02:52]

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

[commit-bot: I haz the power, 2016-05-02 11:02:53]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-05-02 18:22:36]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2016-05-02 18:23:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1902683002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1902683002/20001

[commit-bot: I haz the power, 2016-05-02 18:30:51]

Description was changed from

==========
DocumentThreadableLoader: Add guards for sync notifyFinished() in setResource()

In loadRequest(), setResource() can call clear() synchronously:
  DocumentThreadableLoader::clear()
  DocumentThreadableLoader::handleError()
  Resource::didAddClient()
  RawResource::didAddClient()
and thus |m_client| can be null while resource() isn't null after setResource(),
causing crashes (Issue 595964).

This CL checks whether |*this| is destructed and
whether |m_client| is null after setResource().

BUG=595964
==========

to

==========
DocumentThreadableLoader: Add guards for sync notifyFinished() in setResource()

In loadRequest(), setResource() can call clear() synchronously:
  DocumentThreadableLoader::clear()
  DocumentThreadableLoader::handleError()
  Resource::didAddClient()
  RawResource::didAddClient()
and thus |m_client| can be null while resource() isn't null after setResource(),
causing crashes (Issue 595964).

This CL checks whether |*this| is destructed and
whether |m_client| is null after setResource().

BUG=595964
==========

[commit-bot: I haz the power, 2016-05-02 18:30:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-02 18:32:32]

Description was changed from

==========
DocumentThreadableLoader: Add guards for sync notifyFinished() in setResource()

In loadRequest(), setResource() can call clear() synchronously:
  DocumentThreadableLoader::clear()
  DocumentThreadableLoader::handleError()
  Resource::didAddClient()
  RawResource::didAddClient()
and thus |m_client| can be null while resource() isn't null after setResource(),
causing crashes (Issue 595964).

This CL checks whether |*this| is destructed and
whether |m_client| is null after setResource().

BUG=595964
==========

to

==========
DocumentThreadableLoader: Add guards for sync notifyFinished() in setResource()

In loadRequest(), setResource() can call clear() synchronously:
  DocumentThreadableLoader::clear()
  DocumentThreadableLoader::handleError()
  Resource::didAddClient()
  RawResource::didAddClient()
and thus |m_client| can be null while resource() isn't null after setResource(),
causing crashes (Issue 595964).

This CL checks whether |*this| is destructed and
whether |m_client| is null after setResource().

BUG=595964

Committed: https://crrev.com/2571533bbb5b554ff47205c8ef1513ccc0817c3e
Cr-Commit-Position: refs/heads/master@{#391001}
==========

[commit-bot: I haz the power, 2016-05-02 18:32:33]

Patchset 2 (id:??) landed as
https://crrev.com/2571533bbb5b554ff47205c8ef1513ccc0817c3e
Cr-Commit-Position: refs/heads/master@{#391001}