Prevent calling didReceiveData()/didFinishLoading() after didFailAccessControlCheck()

Prevent calling didReceiveData()/didFinishLoading() after didFailAccessControlCheck()

In some cases DocumentThreadableLoader calls m_client->didReceiveData() or
didFinishLoading() after m_client->didFailAccessControlCheck().
WorkerLoaderClientBridgeSyncHelper assumes no calls are made to
ThreadableLoaderClient after didFailAccessControlCheck(), and this mismatch
results in a race condition.

This CL fixes this by omitting calls to didReceiveData()/didFinishLoading()
after didFailAccessControlCheck() using |m_accessControlCheckFailed| flag.
handlePreflightFailure() is called instead of didFailAccessControlCheck() to
ensure |m_accessControlCheckFailed| and |m_actualRequest| are properly managed.

This CL also makes WorkerLoaderClientBridgeSyncHelper to ASSERT/ignore did*()
calls after any of didFail*() or didFinishLoading() is called.
This explicitly states the assumptions of WorkerLoaderClientBridgeSyncHelper,
and prevents possible race conditions as defensive programming.

BUG=421627
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189418

[hiroshige, 2015-02-02 15:41:21]

hiroshige@chromium.org changed reviewers:
+ kinuko@chromium.org, mkwst@chromium.org, tyoshino@chromium.org

[hiroshige, 2015-02-02 15:44:24]

Could you take a look?

[kinuko, 2015-02-03 02:35:36]

lgtm

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Docum...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Docum...
Source/core/loader/DocumentThreadableLoader.cpp:533: // from other places in
DocumentThreadableLoader.
nit: putting this comment in the header feels more useful / common

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Worke...
File Source/core/loader/WorkerLoaderClientBridgeSyncHelper.cpp (right):

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Worke...
Source/core/loader/WorkerLoaderClientBridgeSyncHelper.cpp:71: return;
nit: I am kind of supportive for this style, but chromium style usually doesn't
like this (assert then handle exceptions).

http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

[tyoshino (SeeGerritForStatus), 2015-02-03 06:22:03]

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Docum...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Docum...
Source/core/loader/DocumentThreadableLoader.cpp:534: void
DocumentThreadableLoader::handlePreflightFailure(const String& url, const
String& errorDescription)
rename this as it's no longer specific to preflight handling.

[tyoshino (SeeGerritForStatus), 2015-02-03 06:22:49]

This CL looks ok but could you also consider adding clearResource() call to
places where we should not proceed any more?

[hiroshige, 2015-02-03 08:19:44]

> clearResource()
I'm also testing that, but currently my CLs are failing.
Partially because in sync requests set/clearResource() paths are not used:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Simply calling clearResponse() in handlePreflightFailure does not work (crashed
many layout tests):
https://codereview.chromium.org/892073002/
More attempts locally.

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Docum...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Docum...
Source/core/loader/DocumentThreadableLoader.cpp:533: // from other places in
DocumentThreadableLoader.
On 2015/02/03 02:35:36, kinuko wrote:
> nit: putting this comment in the header feels more useful / common

Done.

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Docum...
Source/core/loader/DocumentThreadableLoader.cpp:534: void
DocumentThreadableLoader::handlePreflightFailure(const String& url, const
String& errorDescription)
On 2015/02/03 06:22:02, tyoshino wrote:
> rename this as it's no longer specific to preflight handling.

Done.

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Worke...
File Source/core/loader/WorkerLoaderClientBridgeSyncHelper.cpp (right):

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Worke...
Source/core/loader/WorkerLoaderClientBridgeSyncHelper.cpp:71: return;
On 2015/02/03 02:35:36, kinuko wrote:
> nit: I am kind of supportive for this style, but chromium style usually
doesn't
> like this (assert then handle exceptions).
> 
>
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

Hmm.
Then, how about use RELEASE_ASSERT here and omit if statement?
(because just omitting if statement results in a thread-safe bug in Release
builds)

[kinuko, 2015-02-03 08:51:22]

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Worke...
File Source/core/loader/WorkerLoaderClientBridgeSyncHelper.cpp (right):

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Worke...
Source/core/loader/WorkerLoaderClientBridgeSyncHelper.cpp:71: return;
On 2015/02/03 08:19:44, hiroshige wrote:
> On 2015/02/03 02:35:36, kinuko wrote:
> > nit: I am kind of supportive for this style, but chromium style usually
> doesn't
> > like this (assert then handle exceptions).
> > 
> >
>
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
> 
> Hmm.
> Then, how about use RELEASE_ASSERT here and omit if statement?
> (because just omitting if statement results in a thread-safe bug in Release
> builds)

That works for me, assuming that leaving a thread-safe bug could lead to a
security problem.

[Mike West, 2015-02-03 09:04:31]

On 2015/02/03 at 08:51:22, kinuko wrote:
>
https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Worke...
> File Source/core/loader/WorkerLoaderClientBridgeSyncHelper.cpp (right):
> 
>
https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Worke...
> Source/core/loader/WorkerLoaderClientBridgeSyncHelper.cpp:71: return;
> On 2015/02/03 08:19:44, hiroshige wrote:
> > On 2015/02/03 02:35:36, kinuko wrote:
> > > nit: I am kind of supportive for this style, but chromium style usually
> > doesn't
> > > like this (assert then handle exceptions).
> > > 
> > >
> >
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
> > 
> > Hmm.
> > Then, how about use RELEASE_ASSERT here and omit if statement?
> > (because just omitting if statement results in a thread-safe bug in Release
> > builds)
> 
> That works for me, assuming that leaving a thread-safe bug could lead to a
security problem.

I think ASSERT would be enough, given that you're doing the work to ensure that
we don't hit this condition.

That said, RELEASE_ASSERT is fine, but please watch for crash reports; killing
the renderer isn't something we should be doing often.

web/ LGTM.

[hiroshige, 2015-02-03 09:21:14]

> This CL looks ok but could you also consider adding clearResource() call to
> places where we should not proceed any more?
I added FIXME.

> I think ASSERT would be enough, given that you're doing the work to ensure
that
> we don't hit this condition.
In this CL I watched didFinishLoading/didReceivedData after
didFailAccessControlCheck paths, but race conditions in
WorkerLoaderClientBridgeSyncHelper can occur when any of did* calls are made
after a didFinishLoading/didFail* call, and I didn't looked at all of them.
(I didn't observe other such cases while testing locally though)

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Worke...
File Source/core/loader/WorkerLoaderClientBridgeSyncHelper.cpp (right):

https://codereview.chromium.org/891263002/diff/40001/Source/core/loader/Worke...
Source/core/loader/WorkerLoaderClientBridgeSyncHelper.cpp:71: return;
On 2015/02/03 08:51:22, kinuko wrote:
> On 2015/02/03 08:19:44, hiroshige wrote:
> > On 2015/02/03 02:35:36, kinuko wrote:
> > > nit: I am kind of supportive for this style, but chromium style usually
> > doesn't
> > > like this (assert then handle exceptions).
> > > 
> > >
> >
>
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
> > 
> > Hmm.
> > Then, how about use RELEASE_ASSERT here and omit if statement?
> > (because just omitting if statement results in a thread-safe bug in Release
> > builds)
> 
> That works for me, assuming that leaving a thread-safe bug could lead to a
> security problem.

Done.

[tyoshino (SeeGerritForStatus), 2015-02-03 13:06:36]

lgtm

https://codereview.chromium.org/891263002/diff/80001/Source/core/loader/Docum...
File Source/core/loader/DocumentThreadableLoader.h (right):

https://codereview.chromium.org/891263002/diff/80001/Source/core/loader/Docum...
Source/core/loader/DocumentThreadableLoader.h:108: //
m_client->didFailAccessControlCheck() from other places in
what does "other" here mean?

[hiroshige, 2015-02-03 13:11:22]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2015-02-03 13:11:39]

Thanks!

https://codereview.chromium.org/891263002/diff/80001/Source/core/loader/Docum...
File Source/core/loader/DocumentThreadableLoader.h (right):

https://codereview.chromium.org/891263002/diff/80001/Source/core/loader/Docum...
Source/core/loader/DocumentThreadableLoader.h:108: //
m_client->didFailAccessControlCheck() from other places in
On 2015/02/03 13:06:36, tyoshino wrote:
> what does "other" here mean?
I meant "anywhere outside handleAccessControlCheckFailure".
This is not so helpful so removed "from other places".

[commit-bot: I haz the power, 2015-02-03 13:12:25]

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

[commit-bot: I haz the power, 2015-02-03 14:58:00]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189418

[Nate Chapin, 2015-02-03 22:04:59]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/902453002/ by japhet@chromium.org.

The reason for reverting is: Broke some nacl browser_tests:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=....