Fix Document::firstPartyForCookies() for OOPIFs.

Fix Document::firstPartyForCookies() for OOPIFs.

The current usage of topDocument() in firstPartyForCookies() isn't
correct, since topDocument() doesn't actually always return the top
frame's Document, even when it's local.  For example, this will happen
when called from the bottom frame in an A-B-A hierarchy.

In addition, when the top frame is remote, topDocumentURL should use
the top frame's origin, rather than the current frame's origin.

BUG=617677
Committed: https://crrev.com/66606be397f2a0ba853b32dbe335a131e59bd3c7
Cr-Commit-Position: refs/heads/master@{#398561}

[alexmos, 2016-06-06 18:07:40]

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

[alexmos, 2016-06-06 18:07:41]

Mike, can you please take a look?  I'm pretty sure this was just a misuse of
topDocument(), given the isLocalFrame check on the top frame right above.  (It
won't give you the top frame's document if called from the bottom of a A-B-A
hierarchy.)  topDocument() is a terrible name given what it's currently doing
for OOPIFs, and we'll try to remove it.

Also, a couple of questions about this function.  

- for computing topDocumentURL when the top frame is remote, is it using the
current frame's origin intentionally, or did that mean to use the top frame's
origin?

- given your TODO about sandboxed documents, how bad of a problem is this in
general?  Which cases would be broken exactly?  Should it be considered a
blocker for shipping --isolate-extensions?

[Mike West, 2016-06-07 05:42:40]

On 2016/06/06 at 18:07:41, alexmos wrote:
> Mike, can you please take a look?  I'm pretty sure this was just a misuse of
topDocument(), given the isLocalFrame check on the top frame right above.  (It
won't give you the top frame's document if called from the bottom of a A-B-A
hierarchy.)  topDocument() is a terrible name given what it's currently doing
for OOPIFs, and we'll try to remove it.

Nice catch. This change LGTM, but you found a real bug below that it would be
excellent to fix.

> Also, a couple of questions about this function.  
> 
> - for computing topDocumentURL when the top frame is remote, is it using the
current frame's origin intentionally, or did that mean to use the top frame's
origin?

No, that looks wrong. Hrm. I bet we don't have any RemoteFrame tests for this
function. :(

Can we add them in Blink? Is there any mechanism for forcing a document to load
remotely in a layout test or one of WebDocument's unit tests?

> - given your TODO about sandboxed documents, how bad of a problem is this in
general?  Which cases would be broken exactly?  Should it be considered a
blocker for shipping --isolate-extensions?

It only effects cases in which the top-level document sandboxes itself (via CSP,
for instance). It's a correctness problem, but not the biggest deal in the
world, as we'd end up failing closed in cases that matter (e.g. cookies that
should be "first-party" would be considered "third-party").

[alexmos, 2016-06-08 00:24:33]

Description was changed from

==========
Fix incorrect use of topDocument() in Document::firstPartyForCookies().

The current usage isn't correct, since topDocument() doesn't actually
always return the top frame's Document, even when it's local.  For
example, this will happen when called from the bottom frame in an
A-B-A hierarchy.

BUG=617677
==========

to

==========
Fix Document::firstPartyForCookies() for OOPIFs.

The current usage of topDocument() in firstPartyForCookies() isn't
correct, since topDocument() doesn't actually always return the top
frame's Document, even when it's local.  For example, this will happen
when called from the bottom frame in an A-B-A hierarchy.

In addition, when the top frame is remote, topDocumentURL should use
the top frame's origin, rather than the current frame's origin.

BUG=617677
==========

[alexmos, 2016-06-08 00:25:03]

Description was changed from

==========
Fix Document::firstPartyForCookies() for OOPIFs.

The current usage of topDocument() in firstPartyForCookies() isn't
correct, since topDocument() doesn't actually always return the top
frame's Document, even when it's local.  For example, this will happen
when called from the bottom frame in an A-B-A hierarchy.

In addition, when the top frame is remote, topDocumentURL should use
the top frame's origin, rather than the current frame's origin.

BUG=617677
==========

to

==========
Fix Document::firstPartyForCookies() for OOPIFs.

The current usage of topDocument() in firstPartyForCookies() isn't
correct, since topDocument() doesn't actually always return the top
frame's Document, even when it's local.  For example, this will happen
when called from the bottom frame in an A-B-A hierarchy.

In addition, when the top frame is remote, topDocumentURL should use
the top frame's origin, rather than the current frame's origin.

BUG=617677
==========

[alexmos, 2016-06-08 00:29:24]

Thanks!  I've fixed the remote case to use the top frame and added a test -- can
you please take another look?

On 2016/06/07 05:42:40, Mike West (OOO until 30th) wrote:
> On 2016/06/06 at 18:07:41, alexmos wrote:
> > Mike, can you please take a look?  I'm pretty sure this was just a misuse of
> topDocument(), given the isLocalFrame check on the top frame right above.  (It
> won't give you the top frame's document if called from the bottom of a A-B-A
> hierarchy.)  topDocument() is a terrible name given what it's currently doing
> for OOPIFs, and we'll try to remove it.
> 
> Nice catch. This change LGTM, but you found a real bug below that it would be
> excellent to fix.
> 
> > Also, a couple of questions about this function.  
> > 
> > - for computing topDocumentURL when the top frame is remote, is it using the
> current frame's origin intentionally, or did that mean to use the top frame's
> origin?
> 
> No, that looks wrong. Hrm. 

Fixed in latest PS. :)

> I bet we don't have any RemoteFrame tests for this
> function. :(
> 
> Can we add them in Blink? Is there any mechanism for forcing a document to
load
> remotely in a layout test or one of WebDocument's unit tests?

Yes, we've got a bunch of unit tests that do this in WebFrameTest.  I added a
test there to cover this.

> 
> > - given your TODO about sandboxed documents, how bad of a problem is this in
> general?  Which cases would be broken exactly?  Should it be considered a
> blocker for shipping --isolate-extensions?
> 
> It only effects cases in which the top-level document sandboxes itself (via
CSP,
> for instance). It's a correctness problem, but not the biggest deal in the
> world, as we'd end up failing closed in cases that matter (e.g. cookies that
> should be "first-party" would be considered "third-party").

Thanks for clarifying!

[Mike West, 2016-06-08 11:49:31]

LGTM, thanks!

[alexmos, 2016-06-08 14:39:22]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2016-06-08 14:39:44]

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

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

Description was changed from

==========
Fix Document::firstPartyForCookies() for OOPIFs.

The current usage of topDocument() in firstPartyForCookies() isn't
correct, since topDocument() doesn't actually always return the top
frame's Document, even when it's local.  For example, this will happen
when called from the bottom frame in an A-B-A hierarchy.

In addition, when the top frame is remote, topDocumentURL should use
the top frame's origin, rather than the current frame's origin.

BUG=617677
==========

to

==========
Fix Document::firstPartyForCookies() for OOPIFs.

The current usage of topDocument() in firstPartyForCookies() isn't
correct, since topDocument() doesn't actually always return the top
frame's Document, even when it's local.  For example, this will happen
when called from the bottom frame in an A-B-A hierarchy.

In addition, when the top frame is remote, topDocumentURL should use
the top frame's origin, rather than the current frame's origin.

BUG=617677
==========

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

Committed patchset #2 (id:20001)

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

Description was changed from

==========
Fix Document::firstPartyForCookies() for OOPIFs.

The current usage of topDocument() in firstPartyForCookies() isn't
correct, since topDocument() doesn't actually always return the top
frame's Document, even when it's local.  For example, this will happen
when called from the bottom frame in an A-B-A hierarchy.

In addition, when the top frame is remote, topDocumentURL should use
the top frame's origin, rather than the current frame's origin.

BUG=617677
==========

to

==========
Fix Document::firstPartyForCookies() for OOPIFs.

The current usage of topDocument() in firstPartyForCookies() isn't
correct, since topDocument() doesn't actually always return the top
frame's Document, even when it's local.  For example, this will happen
when called from the bottom frame in an A-B-A hierarchy.

In addition, when the top frame is remote, topDocumentURL should use
the top frame's origin, rather than the current frame's origin.

BUG=617677

Committed: https://crrev.com/66606be397f2a0ba853b32dbe335a131e59bd3c7
Cr-Commit-Position: refs/heads/master@{#398561}
==========

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

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