Verify the order of RawResourceClient callbacks in DocumentThreadableLoader

Verify the order of RawResourceClient callbacks in DocumentThreadableLoader

The errors in the order of RawResourceClient callbacks potentially leads to
CORS bypassing, especially in DocumentThreadableLoader.
This CL introduces RawResourceClientStateChecker to check the order of
RawResourceClient callbacks, assuming that the RawResourceClient is added
to at most one RawResource, and applies it to DocumentThreadableLoader.

BUG=633696, 640291
Committed: https://crrev.com/48136fcd4b053686ce2f1f98e64b00a064907ec4
Cr-Commit-Position: refs/heads/master@{#413990}

[hiroshige, 2016-05-31 15:20:37]

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

[commit-bot: I haz the power, 2016-05-31 15:20:55]

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

[commit-bot: I haz the power, 2016-05-31 15:49:08]

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

[commit-bot: I haz the power, 2016-05-31 15:49:09]

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

[hiroshige, 2016-05-31 16:04:07]

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

[commit-bot: I haz the power, 2016-05-31 16:04:24]

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

[commit-bot: I haz the power, 2016-05-31 18:10:01]

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

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

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-06-01 02:58:08]

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

[hiroshige, 2016-06-01 02:58:09]

I think it is important to check that ThreadableLoader's responseReceived() is
always called before dataReceived() (see Issue 612132), so this CL adds
assertions for that.
Not only the responseReceived() and dataReceived(), I think it is good to check
all RawResourceClient overrides in DocumentThreadableLoader.

I also think it is good to check ResourceClient's callback order in general, but
I'm not sure how to implement it, so I implemented this as
ThreadableLoader-specific for now.
- ResourceClients are not yet moved to the Oilpan heap.
- A ResourceClient can be added to multiple Resources in general.
  If a ResourceClient is only added to at most one Resource (e.g.
ResourceOwner), RawResourceClientStateChecker can be applied as-is. But I'm not
sure ResourceOwner should remain after ResourceClient is moved on-heap, and
skipping checks on ResourceClients added to multiple Resources might look
incomplete.
  Alternatively, we can implement RawResourceClientStateChecker so that the
client can be added to multiple Resources, e.g. by adding HashMap<Resource*,
State> (not sure about the correct type though) or HashMap<unsigned long /*
identifier */, State>.

WDYT?

[hiroshige, 2016-06-01 02:58:58]

Description was changed from

==========
Verify the order of RawResourceClient callbacks by RawResourceClientStateChecker

BUG=
==========

to

==========
Verify the order of RawResourceClient callbacks by RawResourceClientStateChecker

BUG=612132
==========

[yhirano, 2016-06-03 07:25:13]

https://codereview.chromium.org/2020313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/RawResource.h (right):

https://codereview.chromium.org/2020313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/RawResource.h:147: class CORE_EXPORT
RawResourceClientStateChecker {
+final

https://codereview.chromium.org/2020313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/RawResource.h:153: void willAddClient();
I'm a bit concerned about the case when DocumentThreadableLoader is a subclass
of ResourceOwner<RawResource>. In that case ResourceOwner::setResource adds the
client to a Resource without notifying DocumentThreadableLoader (Currently no
one outside of the class calls setResource but we will not be able to forbid
it).

Does that mean DocumentThreadableLoader will never be able to be a subclass of
ResourceOwner?

[hiroshige, 2016-06-03 09:58:38]

https://codereview.chromium.org/2020313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/RawResource.h (right):

https://codereview.chromium.org/2020313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/RawResource.h:153: void willAddClient();
On 2016/06/03 07:25:13, yhirano wrote:
> I'm a bit concerned about the case when DocumentThreadableLoader is a subclass
> of ResourceOwner<RawResource>. In that case ResourceOwner::setResource adds
the
> client to a Resource without notifying DocumentThreadableLoader (Currently no
> one outside of the class calls setResource but we will not be able to forbid
> it).
> 
> Does that mean DocumentThreadableLoader will never be able to be a subclass of
> ResourceOwner?

ResourceOwner::setResource() is |protected|.
Doesn't it prevent setResource() calls from outside DocumentThreadableLoader?

[yhirano, 2016-06-07 05:18:26]

https://codereview.chromium.org/2020313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/RawResource.h (right):

https://codereview.chromium.org/2020313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/RawResource.h:153: void willAddClient();
On 2016/06/03 09:58:38, hiroshige (slow) wrote:
> On 2016/06/03 07:25:13, yhirano wrote:
> > I'm a bit concerned about the case when DocumentThreadableLoader is a
subclass
> > of ResourceOwner<RawResource>. In that case ResourceOwner::setResource adds
> the
> > client to a Resource without notifying DocumentThreadableLoader (Currently
no
> > one outside of the class calls setResource but we will not be able to forbid
> > it).
> > 
> > Does that mean DocumentThreadableLoader will never be able to be a subclass
of
> > ResourceOwner?
> 
> ResourceOwner::setResource() is |protected|.
> Doesn't it prevent setResource() calls from outside DocumentThreadableLoader?

I didn't notice that, thank you.
But ResourceOwner may call removeClient(self) in the prefinalizer step, so can
you remove the assertion from the destructor?

[hiroshige, 2016-08-03 11:50:34]

PTAL.

https://codereview.chromium.org/2020313002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/RawResource.h (right):

https://codereview.chromium.org/2020313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/RawResource.h:147: class CORE_EXPORT
RawResourceClientStateChecker {
On 2016/06/03 07:25:13, yhirano wrote:
> +final

Done.

https://codereview.chromium.org/2020313002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/RawResource.h:153: void willAddClient();
On 2016/06/07 05:18:25, yhirano wrote:
> On 2016/06/03 09:58:38, hiroshige (slow) wrote:
> > On 2016/06/03 07:25:13, yhirano wrote:
> > > I'm a bit concerned about the case when DocumentThreadableLoader is a
> subclass
> > > of ResourceOwner<RawResource>. In that case ResourceOwner::setResource
adds
> > the
> > > client to a Resource without notifying DocumentThreadableLoader (Currently
> no
> > > one outside of the class calls setResource but we will not be able to
forbid
> > > it).
> > > 
> > > Does that mean DocumentThreadableLoader will never be able to be a
subclass
> of
> > > ResourceOwner?
> > 
> > ResourceOwner::setResource() is |protected|.
> > Doesn't it prevent setResource() calls from outside
DocumentThreadableLoader?
> 
> I didn't notice that, thank you.
> But ResourceOwner may call removeClient(self) in the prefinalizer step, so can
> you remove the assertion from the destructor?

Done.

[hiroshige, 2016-08-03 12:15:21]

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

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

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-08-03 13:19:12]

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

[commit-bot: I haz the power, 2016-08-03 13:19:13]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-08-04 15:34:09]

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

[hiroshige, 2016-08-04 15:34:10]

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

[Nate Chapin, 2016-08-04 16:46:16]

On 2016/08/04 15:34:10, hiroshige wrote:
> +japhet@, could you also take a look as a core/fetch OWNER?

I'm curious, why isn't this entirely in DocumentThreadableLoader? There aren't
actually any changes to RawResource here, and DocumentThreadableLoader is the
only RawResourceClient that is implementing the state checks, so it feels weird
to put the state machine in RawResource.{h,cpp}.

[hiroshige, 2016-08-05 05:11:37]

On 2016/08/04 16:46:16, Nate Chapin wrote:
> On 2016/08/04 15:34:10, hiroshige wrote:
> > +japhet@, could you also take a look as a core/fetch OWNER?
> 
> I'm curious, why isn't this entirely in DocumentThreadableLoader? There aren't
> actually any changes to RawResource here, and DocumentThreadableLoader is the
> only RawResourceClient that is implementing the state checks, so it feels
weird
> to put the state machine in RawResource.{h,cpp}.

For generality, I'd like to apply RawResourceClientStateChecker (or similar
checker) to all RawResourceClient or ResourceOwner.
But because callback order errors in DocumentThreadableLoader are especially
critical because it directly results in CORS bypassing security bugs, I applied
it first to DocumentThreadableLoader only in this CL.

[hiroshige, 2016-08-05 05:14:11]

Description was changed from

==========
Verify the order of RawResourceClient callbacks by RawResourceClientStateChecker

BUG=612132
==========

to

==========
Verify the order of RawResourceClient callbacks in DocumentThreadableLoader

The errors in the order of RawResourceClient callbacks potentially leads to
CORS bypassing, especially in DocumentThreadableLoader.
This CL introduces RawResourceClientStateChecker to check the order of
RawResourceClient callbacks, assuming that the RawResourceClient is added
to at most one RawResource, and applies it to DocumentThreadableLoader.

BUG=612132
==========

[hiroshige, 2016-08-05 05:14:28]

On 2016/08/05 05:11:37, hiroshige wrote:
> On 2016/08/04 16:46:16, Nate Chapin wrote:
> > On 2016/08/04 15:34:10, hiroshige wrote:
> > > +japhet@, could you also take a look as a core/fetch OWNER?
> > 
> > I'm curious, why isn't this entirely in DocumentThreadableLoader? There
aren't
> > actually any changes to RawResource here, and DocumentThreadableLoader is
the
> > only RawResourceClient that is implementing the state checks, so it feels
> weird
> > to put the state machine in RawResource.{h,cpp}.
> 
> For generality, I'd like to apply RawResourceClientStateChecker (or similar
> checker) to all RawResourceClient or ResourceOwner.
> But because callback order errors in DocumentThreadableLoader are especially
> critical because it directly results in CORS bypassing security bugs, I
applied
> it first to DocumentThreadableLoader only in this CL.

Updated the CL description.

[yhirano, 2016-08-08 08:59:08]

> For generality, I'd like to apply RawResourceClientStateChecker (or similar
> checker) to all RawResourceClient or ResourceOwner.
> But because callback order errors in DocumentThreadableLoader are especially
> critical because it directly results in CORS bypassing security bugs, I
applied
> it first to DocumentThreadableLoader only in this CL.

Do you have a concrete plan? I'm planning to removing setClient and related
functions from DocumentThreadableLoader by making DocumentThreadableLoader
inherit ResourceOwner<RawResource>, but this change makes it impossible.

[hiroshige, 2016-08-08 09:12:55]

On 2016/08/08 08:59:08, yhirano wrote:
> > For generality, I'd like to apply RawResourceClientStateChecker (or similar
> > checker) to all RawResourceClient or ResourceOwner.
> > But because callback order errors in DocumentThreadableLoader are especially
> > critical because it directly results in CORS bypassing security bugs, I
> applied
> > it first to DocumentThreadableLoader only in this CL.
> 
> Do you have a concrete plan?
Not yet.

> I'm planning to removing setClient and related
> functions from DocumentThreadableLoader by making DocumentThreadableLoader
> inherit ResourceOwner<RawResource>, but this change makes it impossible.
Then, how about applying this kind of check to ResourceOwner<RawResource>?
I found your change is already landed so I'll rebase.

[yhirano, 2016-08-08 09:22:36]

On 2016/08/08 09:12:55, hiroshige wrote:
> On 2016/08/08 08:59:08, yhirano wrote:
> > > For generality, I'd like to apply RawResourceClientStateChecker (or
similar
> > > checker) to all RawResourceClient or ResourceOwner.
> > > But because callback order errors in DocumentThreadableLoader are
especially
> > > critical because it directly results in CORS bypassing security bugs, I
> > applied
> > > it first to DocumentThreadableLoader only in this CL.
> > 
> > Do you have a concrete plan?
> Not yet.
> 
> > I'm planning to removing setClient and related
> > functions from DocumentThreadableLoader by making DocumentThreadableLoader
> > inherit ResourceOwner<RawResource>, but this change makes it impossible.
> Then, how about applying this kind of check to ResourceOwner<RawResource>?
I hope that ResourceOwner<T> is defined uniformally (it's not now).

> I found your change is already landed so I'll rebase.
I've reverted the CL for some reason.

It's OK to cancel the plan to make DocumentThreadableLoader inherit
ResourceOwner<RawResource>, but in that case please update the TODO comment.

[hiroshige, 2016-08-08 10:07:50]

On 2016/08/08 09:22:36, yhirano wrote:
> On 2016/08/08 09:12:55, hiroshige wrote:
> > On 2016/08/08 08:59:08, yhirano wrote:
> > > > For generality, I'd like to apply RawResourceClientStateChecker (or
> similar
> > > > checker) to all RawResourceClient or ResourceOwner.
> > > > But because callback order errors in DocumentThreadableLoader are
> especially
> > > > critical because it directly results in CORS bypassing security bugs, I
> > > applied
> > > > it first to DocumentThreadableLoader only in this CL.
> > > 
> > > Do you have a concrete plan?
> > Not yet.
> > 
> > > I'm planning to removing setClient and related
> > > functions from DocumentThreadableLoader by making DocumentThreadableLoader
> > > inherit ResourceOwner<RawResource>, but this change makes it impossible.
> > Then, how about applying this kind of check to ResourceOwner<RawResource>?
> I hope that ResourceOwner<T> is defined uniformally (it's not now).
> 
> > I found your change is already landed so I'll rebase.
> I've reverted the CL for some reason.
> 
> It's OK to cancel the plan to make DocumentThreadableLoader inherit
> ResourceOwner<RawResource>, but in that case please update the TODO comment.

Hmm. How about
1. landing this CL on canary as-is (i.e. while it is blocking making
DocumentThreadableLoader to be a ResourceOwner subclass)
2. landing [1] and merging it to stable
3. reverting this CL or refactoring it so that DocumentThreadableLoader can be a
ResourceOwner.
4. relanding your CLs.
because I'd like to check [1] is not violating the assertions in this CL before
merging [1] to stable.

[1] https://codereview.chromium.org/2210473002/

[yhirano, 2016-08-09 08:14:51]

lgtm

[hiroshige, 2016-08-10 13:53:56]

japhet@, could you take a look?

[hiroshige, 2016-08-23 10:31:04]

japhet@, friendly ping.

[hiroshige, 2016-08-23 10:49:32]

Patchset #4 (id:60001) has been deleted

[Nate Chapin, 2016-08-23 16:25:13]

lgtm

[hiroshige, 2016-08-23 19:08:14]

Description was changed from

==========
Verify the order of RawResourceClient callbacks in DocumentThreadableLoader

The errors in the order of RawResourceClient callbacks potentially leads to
CORS bypassing, especially in DocumentThreadableLoader.
This CL introduces RawResourceClientStateChecker to check the order of
RawResourceClient callbacks, assuming that the RawResourceClient is added
to at most one RawResource, and applies it to DocumentThreadableLoader.

BUG=612132
==========

to

==========
Verify the order of RawResourceClient callbacks in DocumentThreadableLoader

The errors in the order of RawResourceClient callbacks potentially leads to
CORS bypassing, especially in DocumentThreadableLoader.
This CL introduces RawResourceClientStateChecker to check the order of
RawResourceClient callbacks, assuming that the RawResourceClient is added
to at most one RawResource, and applies it to DocumentThreadableLoader.

BUG=612132, 633696
==========

[hiroshige, 2016-08-23 19:12:14]

Description was changed from

==========
Verify the order of RawResourceClient callbacks in DocumentThreadableLoader

The errors in the order of RawResourceClient callbacks potentially leads to
CORS bypassing, especially in DocumentThreadableLoader.
This CL introduces RawResourceClientStateChecker to check the order of
RawResourceClient callbacks, assuming that the RawResourceClient is added
to at most one RawResource, and applies it to DocumentThreadableLoader.

BUG=612132, 633696
==========

to

==========
Verify the order of RawResourceClient callbacks in DocumentThreadableLoader

The errors in the order of RawResourceClient callbacks potentially leads to
CORS bypassing, especially in DocumentThreadableLoader.
This CL introduces RawResourceClientStateChecker to check the order of
RawResourceClient callbacks, assuming that the RawResourceClient is added
to at most one RawResource, and applies it to DocumentThreadableLoader.

BUG=612132, 633696, 640291
==========

[hiroshige, 2016-08-23 19:12:35]

Description was changed from

==========
Verify the order of RawResourceClient callbacks in DocumentThreadableLoader

The errors in the order of RawResourceClient callbacks potentially leads to
CORS bypassing, especially in DocumentThreadableLoader.
This CL introduces RawResourceClientStateChecker to check the order of
RawResourceClient callbacks, assuming that the RawResourceClient is added
to at most one RawResource, and applies it to DocumentThreadableLoader.

BUG=612132, 633696, 640291
==========

to

==========
Verify the order of RawResourceClient callbacks in DocumentThreadableLoader

The errors in the order of RawResourceClient callbacks potentially leads to
CORS bypassing, especially in DocumentThreadableLoader.
This CL introduces RawResourceClientStateChecker to check the order of
RawResourceClient callbacks, assuming that the RawResourceClient is added
to at most one RawResource, and applies it to DocumentThreadableLoader.

BUG=633696, 640291
==========

[hiroshige, 2016-08-23 19:24:13]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2016-08-23 19:24:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2020313002/#ps100001
(title: "Fix rebase error and update TODO comment")

[commit-bot: I haz the power, 2016-08-23 19:24:52]

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-08-23 22:20:11]

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

[commit-bot: I haz the power, 2016-08-23 22:20:12]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hiroshige, 2016-08-24 04:25:08]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2016-08-24 04:25:32]

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-08-24 05:52:46]

Description was changed from

==========
Verify the order of RawResourceClient callbacks in DocumentThreadableLoader

The errors in the order of RawResourceClient callbacks potentially leads to
CORS bypassing, especially in DocumentThreadableLoader.
This CL introduces RawResourceClientStateChecker to check the order of
RawResourceClient callbacks, assuming that the RawResourceClient is added
to at most one RawResource, and applies it to DocumentThreadableLoader.

BUG=633696, 640291
==========

to

==========
Verify the order of RawResourceClient callbacks in DocumentThreadableLoader

The errors in the order of RawResourceClient callbacks potentially leads to
CORS bypassing, especially in DocumentThreadableLoader.
This CL introduces RawResourceClientStateChecker to check the order of
RawResourceClient callbacks, assuming that the RawResourceClient is added
to at most one RawResource, and applies it to DocumentThreadableLoader.

BUG=633696, 640291
==========

[commit-bot: I haz the power, 2016-08-24 05:52:48]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-08-24 05:54:59]

Description was changed from

==========
Verify the order of RawResourceClient callbacks in DocumentThreadableLoader

The errors in the order of RawResourceClient callbacks potentially leads to
CORS bypassing, especially in DocumentThreadableLoader.
This CL introduces RawResourceClientStateChecker to check the order of
RawResourceClient callbacks, assuming that the RawResourceClient is added
to at most one RawResource, and applies it to DocumentThreadableLoader.

BUG=633696, 640291
==========

to

==========
Verify the order of RawResourceClient callbacks in DocumentThreadableLoader

The errors in the order of RawResourceClient callbacks potentially leads to
CORS bypassing, especially in DocumentThreadableLoader.
This CL introduces RawResourceClientStateChecker to check the order of
RawResourceClient callbacks, assuming that the RawResourceClient is added
to at most one RawResource, and applies it to DocumentThreadableLoader.

BUG=633696, 640291

Committed: https://crrev.com/48136fcd4b053686ce2f1f98e64b00a064907ec4
Cr-Commit-Position: refs/heads/master@{#413990}
==========

[commit-bot: I haz the power, 2016-08-24 05:55:00]

Patchset 5 (id:??) landed as
https://crrev.com/48136fcd4b053686ce2f1f98e64b00a064907ec4
Cr-Commit-Position: refs/heads/master@{#413990}