Fix frameElement for frames with a remote parent.

Fix frameElement for frames with a remote parent.

Previously, accessing window.frameElement on a frame with a remote
parent crashed due to the assert in LocalDOMWindow::frameElement().
This CL fixes things so that we properly throw an exception in that
case.

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

[alexmos, 2015-04-30 20:45:25]

alexmos@chromium.org changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org

[alexmos, 2015-04-30 20:45:26]

haraken@: please take a look.

dcheng@: you may want to review this as well, plus a question in
LocalDOMWindow.cpp below.

Note that we couldn't just pass null into 
shouldAllowAccessToNode when frameOwner is remote, since that would not throw an
exception as it should, so I switched the access check to use
shouldAllowAccessToFrame on the parent frame, which should be equivalent.

https://codereview.chromium.org/1112843007/diff/40001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1112843007/diff/40001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:736: if (!frame() || (frame()->owner() &&
!frame()->owner()->isLocal()))
The comment about the binding security check below implies that frameElement()
is expected to be called only when the frame and parent are same-origin.  Do we
want to keep this, or should we return nullptr for the remote case as opposed to
hitting the ASSERT below?  The fix in this CL doesn't need this change; I'm just
curious what you think.

[dcheng, 2015-04-30 20:58:52]

https://codereview.chromium.org/1112843007/diff/40001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1112843007/diff/40001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:736: if (!frame() || (frame()->owner() &&
!frame()->owner()->isLocal()))
On 2015/04/30 20:45:26, alexmos wrote:
> The comment about the binding security check below implies that frameElement()
> is expected to be called only when the frame and parent are same-origin.  Do
we
> want to keep this, or should we return nullptr for the remote case as opposed
to
> hitting the ASSERT below?  The fix in this CL doesn't need this change; I'm
just
> curious what you think.

I'd prefer to just hit the assert below. deprecatedLocalOwner() will also assert
(though since we're in this code, it might be good to just change that to
toHTMLFrameOwnerElement(frame()->owner(), so we know this particular usage is
"kosher").

[alexmos, 2015-04-30 21:12:58]

https://codereview.chromium.org/1112843007/diff/40001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1112843007/diff/40001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:736: if (!frame() || (frame()->owner() &&
!frame()->owner()->isLocal()))
On 2015/04/30 20:58:51, dcheng wrote:
> On 2015/04/30 20:45:26, alexmos wrote:
> > The comment about the binding security check below implies that
frameElement()
> > is expected to be called only when the frame and parent are same-origin.  Do
> we
> > want to keep this, or should we return nullptr for the remote case as
opposed
> to
> > hitting the ASSERT below?  The fix in this CL doesn't need this change; I'm
> just
> > curious what you think.
> 
> I'd prefer to just hit the assert below. deprecatedLocalOwner() will also
assert
> (though since we're in this code, it might be good to just change that to
> toHTMLFrameOwnerElement(frame()->owner(), so we know this particular usage is
> "kosher").

Sounds good -- done.

[dcheng, 2015-04-30 21:18:13]

Source/core and the test changes lgtm, with one nit

https://codereview.chromium.org/1112843007/diff/60001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1112843007/diff/60001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:740: ASSERT(!frame()->owner() ||
frame()->owner()->isLocal());
Minor nit: the isLocal() becomes redundant here.

[alexmos, 2015-04-30 22:29:57]

https://codereview.chromium.org/1112843007/diff/60001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1112843007/diff/60001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:740: ASSERT(!frame()->owner() ||
frame()->owner()->isLocal());
On 2015/04/30 21:18:13, dcheng wrote:
> Minor nit: the isLocal() becomes redundant here.

Yeah, I suppose this whole ASSERT is now redundant with the one in
toHTMLFrameOwnerElement.  Removed in PS5.

[haraken, 2015-05-01 00:12:21]

Or would it make sense to keep impl->frameElement() in the binding layer by
making LocalDOMWindow::frameElement() return nullptr for a remote frame element?

Because V8Window::frameElementAttributeGetterCustom is using
impl->frameElement() in multiple places, it would be less confusing if we could
use it for the access check as well.

[alexmos, 2015-05-01 00:43:12]

On 2015/05/01 00:12:21, haraken wrote:
> Or would it make sense to keep impl->frameElement() in the binding layer by
> making LocalDOMWindow::frameElement() return nullptr for a remote frame
element?
> 
> Because V8Window::frameElementAttributeGetterCustom is using
> impl->frameElement() in multiple places, it would be less confusing if we
could
> use it for the access check as well.

I tried that first, and the problem with that is that we need to throw an
exception for the remote parent case, but if we pass nullptr into
shouldAllowAccessToNode, it would return right away without throwing.  (And to
generate the right exception message, something needs to call
crossDomainAccessErrorMessage on the parent frame.)

[haraken, 2015-05-01 01:11:29]

On 2015/05/01 00:43:12, alexmos wrote:
> On 2015/05/01 00:12:21, haraken wrote:
> > Or would it make sense to keep impl->frameElement() in the binding layer by
> > making LocalDOMWindow::frameElement() return nullptr for a remote frame
> element?
> > 
> > Because V8Window::frameElementAttributeGetterCustom is using
> > impl->frameElement() in multiple places, it would be less confusing if we
> could
> > use it for the access check as well.
> 
> I tried that first, and the problem with that is that we need to throw an
> exception for the remote parent case, but if we pass nullptr into
> shouldAllowAccessToNode, it would return right away without throwing.  (And to
> generate the right exception message, something needs to call
> crossDomainAccessErrorMessage on the parent frame.)

Ah, that makes sense. We need to do the access check against the frame, not
against the node. Let's add a comment about it.

LGTM.

[alexmos, 2015-05-01 18:38:53]

> Ah, that makes sense. We need to do the access check against the frame, not
> against the node. Let's add a comment about it.

Done.  Thanks!

[alexmos, 2015-05-01 18:39:48]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2015-05-01 18:39:48]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1112843007/#ps100001
(title: "Add comment about doing the access check on the frame vs node.")

[commit-bot: I haz the power, 2015-05-01 18:40:33]

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

[commit-bot: I haz the power, 2015-05-01 20:33:47]

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