Make canNavigate() OOPI-friendly

Make canNavigate() OOPI-friendly

* Move SandboxFlags from ExecutionContext to SecurityContext (SandboxFlags aren't propagated for RemoteFrames, but this puts the state in a place where RemoteFrames could access it when it is plumbed)
* Teach HTMLFrameOwnerElement about its Document's SandboxFlags and don't make FrameLoader check both objects to generate this Frame's SandboxFlags.
* Now that SandboxFlags are in a general location, move canNavigate from Document to Frame
* Now that canNavigate() can handle RemoteFrames, move findFrameForNavigation from FrameLoader to Frame

BUG=

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

[Nate Chapin, 2014-12-12 23:22:39]

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

[Nate Chapin, 2014-12-12 23:22:39]

This ended up a bit bigger than I intended, hopefully the summary is
sufficiently clear about what is moving where.

[dcheng, 2014-12-13 01:55:49]

https://codereview.chromium.org/799923006/diff/20001/Source/core/dom/RemoteSe...
File Source/core/dom/RemoteSecurityContext.h (right):

https://codereview.chromium.org/799923006/diff/20001/Source/core/dom/RemoteSe...
Source/core/dom/RemoteSecurityContext.h:18: virtual void
didUpdateSecurityOrigin() override { }
No virtual per
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/sjotlpNyF-8

https://codereview.chromium.org/799923006/diff/20001/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/799923006/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:156: Frame* frame = tree().find(name);
Not really related to this patch, but I wonder if it's intentional that this
doesn't use TreeScope. Does this mean that frames could be navigated across
shadow DOM scopes?

Also, the whole 'search through other pages' bit seems pretty suspicious.

https://codereview.chromium.org/799923006/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:158: return 0;
nullptr

https://codereview.chromium.org/799923006/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:244: // FIXME: We don't yet have access to a
RemoteFrame's security origin.
Should we fix this while we're here?

https://codereview.chromium.org/799923006/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:252: return 0;
nullptr

https://codereview.chromium.org/799923006/diff/20001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/799923006/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.h:92: virtual void printNavigationErrorMessage(const
Frame&, const char* reason) { }
Perhaps keep this pure virtual for consistency? There should only be two Frame
implementations, so it shouldn't be too big of a cost.

https://codereview.chromium.org/799923006/diff/20001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/799923006/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:749: RefPtrWillBeRawPtr<LocalFrame>
targetFrame = toLocalFrame(request.formState() ? 0 :
m_frame->findFrameForNavigation(AtomicString(request.frameName()), m_frame));
Since this is changing anyway, let's use nullptr instead of 0.

https://codereview.chromium.org/799923006/diff/20001/Source/core/page/CreateW...
File Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/799923006/diff/20001/Source/core/page/CreateW...
Source/core/page/CreateWindow.cpp:53: if (Frame* frame =
lookupFrame.findFrameForNavigation(request.frameName(), &openerFrame)) {
openerFrame should never be null right? Should we pass it by reference?

https://codereview.chromium.org/799923006/diff/20001/Source/core/page/CreateW...
Source/core/page/CreateWindow.cpp:57: return frame->isLocalFrame() ?
toLocalFrame(frame) : 0;
nullptr

[Nate Chapin, 2014-12-20 00:09:14]

https://codereview.chromium.org/799923006/diff/20001/Source/core/dom/RemoteSe...
File Source/core/dom/RemoteSecurityContext.h (right):

https://codereview.chromium.org/799923006/diff/20001/Source/core/dom/RemoteSe...
Source/core/dom/RemoteSecurityContext.h:18: virtual void
didUpdateSecurityOrigin() override { }
On 2014/12/13 01:55:48, dcheng wrote:
> No virtual per
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/sjotlpNyF-8

Done.

https://codereview.chromium.org/799923006/diff/20001/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/799923006/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:156: Frame* frame = tree().find(name);
On 2014/12/13 01:55:48, dcheng wrote:
> Not really related to this patch, but I wonder if it's intentional that this
> doesn't use TreeScope. Does this mean that frames could be navigated across
> shadow DOM scopes?

That's an interesting question for the future.

> 
> Also, the whole 'search through other pages' bit seems pretty suspicious.

That.....bothers me. I hadn't noticed that before.

https://codereview.chromium.org/799923006/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:158: return 0;
On 2014/12/13 01:55:48, dcheng wrote:
> nullptr

Done.

https://codereview.chromium.org/799923006/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:244: // FIXME: We don't yet have access to a
RemoteFrame's security origin.
On 2014/12/13 01:55:48, dcheng wrote:
> Should we fix this while we're here?

Done. Also, moved this function to frame, changed return type to frame, and
updated callsites to defend against this returning a RemoteFrame.

https://codereview.chromium.org/799923006/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:252: return 0;
On 2014/12/13 01:55:48, dcheng wrote:
> nullptr

Done.

https://codereview.chromium.org/799923006/diff/20001/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/799923006/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.h:92: virtual void printNavigationErrorMessage(const
Frame&, const char* reason) { }
On 2014/12/13 01:55:48, dcheng wrote:
> Perhaps keep this pure virtual for consistency? There should only be two Frame
> implementations, so it shouldn't be too big of a cost.

Done.

https://codereview.chromium.org/799923006/diff/20001/Source/core/loader/Frame...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/799923006/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoader.cpp:749: RefPtrWillBeRawPtr<LocalFrame>
targetFrame = toLocalFrame(request.formState() ? 0 :
m_frame->findFrameForNavigation(AtomicString(request.frameName()), m_frame));
On 2014/12/13 01:55:49, dcheng wrote:
> Since this is changing anyway, let's use nullptr instead of 0.

Done.

https://codereview.chromium.org/799923006/diff/20001/Source/core/page/CreateW...
File Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/799923006/diff/20001/Source/core/page/CreateW...
Source/core/page/CreateWindow.cpp:53: if (Frame* frame =
lookupFrame.findFrameForNavigation(request.frameName(), &openerFrame)) {
On 2014/12/13 01:55:49, dcheng wrote:
> openerFrame should never be null right? Should we pass it by reference?

Done.

https://codereview.chromium.org/799923006/diff/20001/Source/core/page/CreateW...
Source/core/page/CreateWindow.cpp:57: return frame->isLocalFrame() ?
toLocalFrame(frame) : 0;
On 2014/12/13 01:55:49, dcheng wrote:
> nullptr

Done.

[dcheng, 2015-01-08 00:44:59]

https://codereview.chromium.org/799923006/diff/40001/Source/core/dom/Security...
File Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/799923006/diff/40001/Source/core/dom/Security...
Source/core/dom/SecurityContext.cpp:72: // The SandboxOrigin is stored
redundantly in the security origin.
This comment reads really strangely. It also seems out of date? Maybe we should
remove it (or correct it?)

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

https://codereview.chromium.org/799923006/diff/40001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1711: if (!activeDocument->frame() ||
!activeDocument->frame()->canNavigate(*frame()))
Why does this one require a null check of frame(), while several other instances
do not?

https://codereview.chromium.org/799923006/diff/40001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/799923006/diff/40001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.h:90: virtual void
printNavigationErrorMessage(const Frame&, const char* reason) override;
No virtual.

https://codereview.chromium.org/799923006/diff/40001/Source/core/page/CreateW...
File Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/799923006/diff/40001/Source/core/page/CreateW...
Source/core/page/CreateWindow.cpp:63: return frame->isLocalFrame() ?
toLocalFrame(frame) : nullptr;
Tag this with a FIXME, because it's clearly broken.

Out of curiosity, do you know if it'll be hard is it to change createWindow() to
return Frame*?

[Nate Chapin, 2015-01-08 17:10:52]

https://codereview.chromium.org/799923006/diff/40001/Source/core/dom/Security...
File Source/core/dom/SecurityContext.cpp (right):

https://codereview.chromium.org/799923006/diff/40001/Source/core/dom/Security...
Source/core/dom/SecurityContext.cpp:72: // The SandboxOrigin is stored
redundantly in the security origin.
On 2015/01/08 00:44:59, dcheng wrote:
> This comment reads really strangely. It also seems out of date? Maybe we
should
> remove it (or correct it?)

Yeah, removing.

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

https://codereview.chromium.org/799923006/diff/40001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1711: if (!activeDocument->frame() ||
!activeDocument->frame()->canNavigate(*frame()))
On 2015/01/08 00:44:59, dcheng wrote:
> Why does this one require a null check of frame(), while several other
instances
> do not?

Um.....because this is the only one required to make the layout tests pass? :D

Will add the null checks in the other locations, they're almost certainly
crashable.

https://codereview.chromium.org/799923006/diff/40001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/799923006/diff/40001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.h:90: virtual void
printNavigationErrorMessage(const Frame&, const char* reason) override;
On 2015/01/08 00:44:59, dcheng wrote:
> No virtual.

Someday I'll remember. Done.

https://codereview.chromium.org/799923006/diff/40001/Source/core/page/CreateW...
File Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/799923006/diff/40001/Source/core/page/CreateW...
Source/core/page/CreateWindow.cpp:63: return frame->isLocalFrame() ?
toLocalFrame(frame) : nullptr;
On 2015/01/08 00:44:59, dcheng wrote:
> Tag this with a FIXME, because it's clearly broken.

Done.

> 
> Out of curiosity, do you know if it'll be hard is it to change createWindow()
to
> return Frame*?

I haven't looked, but I'd guess there are a bunch of yaks to shave first. We'll
need to change ChromeClient::createWindow() to know how to create the frame in a
different process.

[dcheng, 2015-01-08 23:17:26]

https://codereview.chromium.org/799923006/diff/40001/Source/core/page/CreateW...
File Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/799923006/diff/40001/Source/core/page/CreateW...
Source/core/page/CreateWindow.cpp:63: return frame->isLocalFrame() ?
toLocalFrame(frame) : nullptr;
On 2015/01/08 at 17:10:52, Nate Chapin wrote:
> On 2015/01/08 00:44:59, dcheng wrote:
> > Tag this with a FIXME, because it's clearly broken.
> 
> Done.
> 
> > 
> > Out of curiosity, do you know if it'll be hard is it to change
createWindow() to
> > return Frame*?
> 
> I haven't looked, but I'd guess there are a bunch of yaks to shave first.
We'll need to change ChromeClient::createWindow() to know how to create the
frame in a different process.

Hmm, I was thinking just change the return type so we could handle the case
where it's targeting a named DOMWindow. But even that look like it requires some
cleanup.

https://codereview.chromium.org/799923006/diff/60001/Source/core/frame/Histor...
File Source/core/frame/History.cpp (right):

https://codereview.chromium.org/799923006/diff/60001/Source/core/frame/Histor...
Source/core/frame/History.cpp:109: if
(!activeDocument->frame()->canNavigate(*m_frame))
Does this need a similar null check for frame()/

[dcheng, 2015-01-08 23:20:19]

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

https://codereview.chromium.org/799923006/diff/40001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:1711: if (!activeDocument->frame() ||
!activeDocument->frame()->canNavigate(*frame()))
On 2015/01/08 at 17:10:52, Nate Chapin wrote:
> On 2015/01/08 00:44:59, dcheng wrote:
> > Why does this one require a null check of frame(), while several other
instances
> > do not?
> 
> Um.....because this is the only one required to make the layout tests pass? :D
> 
> Will add the null checks in the other locations, they're almost certainly
crashable.

Any chance you have a crash stack handy? I'm curious how this was getting
triggered.

[Nate Chapin, 2015-01-09 00:09:41]

On 2015/01/08 23:20:19, dcheng wrote:
>
https://codereview.chromium.org/799923006/diff/40001/Source/core/frame/LocalD...
> File Source/core/frame/LocalDOMWindow.cpp (right):
> 
>
https://codereview.chromium.org/799923006/diff/40001/Source/core/frame/LocalD...
> Source/core/frame/LocalDOMWindow.cpp:1711: if (!activeDocument->frame() ||
> !activeDocument->frame()->canNavigate(*frame()))
> On 2015/01/08 at 17:10:52, Nate Chapin wrote:
> > On 2015/01/08 00:44:59, dcheng wrote:
> > > Why does this one require a null check of frame(), while several other
> instances
> > > do not?
> > 
> > Um.....because this is the only one required to make the layout tests pass?
:D
> > 
> > Will add the null checks in the other locations, they're almost certainly
> crashable.
> 
> Any chance you have a crash stack handy? I'm curious how this was getting
> triggered.

I don't have an interesting, full stack handy (JS is busting the stack trace),
but one of the tests that's crashing without the frame() null checks is
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/security/xs...

It has to be crashing inside of the iframe's first load event handler. In that
case, the activeDocument is the document where the eval() ran, which is the
first document loaded into the iframe (about:blank), and that context is now
detached. I'm pretty confident this technique could be similarly leveraged
against the other functions that call canNavigate().

[Nate Chapin, 2015-01-09 00:12:46]

https://codereview.chromium.org/799923006/diff/60001/Source/core/frame/Histor...
File Source/core/frame/History.cpp (right):

https://codereview.chromium.org/799923006/diff/60001/Source/core/frame/Histor...
Source/core/frame/History.cpp:109: if
(!activeDocument->frame()->canNavigate(*m_frame))
On 2015/01/08 23:17:26, dcheng wrote:
> Does this need a similar null check for frame()/

Done.

[dcheng, 2015-01-09 00:13:49]

lgtm

[Nate Chapin, 2015-01-09 00:14:55]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-01-09 00:15:28]

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

[commit-bot: I haz the power, 2015-01-09 01:58:01]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188097