Replicate sandbox flags for OOPIF (Blink part 2)

Replicate sandbox flags for OOPIF (Blink part 2).

* Add plumbing to pass sandbox flags from HTMLFrameOwnerElement to browser process via WebFrameClient::createChildFrame().
* Enable RemoteBridgeFrameOwner::sandboxFlags() to check the remote parent's (replicated) sandbox flags, similarly to what HTMLFrameOwnerElement::sandboxFlags() does.
* Remove the old version of WebRemoteFrame::createLocalChild.

Blink part 1 is here: https://codereview.chromium.org/793493003/
Corresponding Chromium CL: https://codereview.chromium.org/837283003/

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

[alexmos, 2015-01-16 18:31:40]

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

[alexmos, 2015-01-16 18:31:41]

Daniel, could you please take a look?

https://codereview.chromium.org/838903002/diff/60001/Source/web/FrameLoaderCl...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/838903002/diff/60001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.cpp:680: FrameLoadRequest
frameRequest(m_webFrame->frame()->document(), url, name,
shouldCheckContentSecurityPolicy, sandboxFlags);
Instead of plumbing sandboxFlags through createFrame and
FrameLoadRequest, a simpler approach is to make sandboxFlags() public in
HTMLFrameOwnerElement and access it here as ownerElement->sandboxFlags().  Is
there any reason not to make sandboxFlags() (and the rest of FrameOwner methods)
public in HTMLFrameOwnerElement?

[dcheng, 2015-01-16 19:11:18]

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

[dcheng, 2015-01-16 19:11:20]

https://codereview.chromium.org/838903002/diff/60001/Source/core/loader/Frame...
File Source/core/loader/FrameLoadRequest.h (right):

https://codereview.chromium.org/838903002/diff/60001/Source/core/loader/Frame...
Source/core/loader/FrameLoadRequest.h:83: , m_sandboxFlags(SandboxNone)
We should probably consider delegated constructors =)

https://codereview.chromium.org/838903002/diff/60001/Source/core/loader/Frame...
Source/core/loader/FrameLoadRequest.h:138: const SandboxFlags& sandboxFlags()
const { return m_sandboxFlags; }
Are you intentionally returning a reference here? Many accessors for POD types
like this tend to return by value.

https://codereview.chromium.org/838903002/diff/60001/Source/web/FrameLoaderCl...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/838903002/diff/60001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.cpp:680: FrameLoadRequest
frameRequest(m_webFrame->frame()->document(), url, name,
shouldCheckContentSecurityPolicy, sandboxFlags);
On 2015/01/16 at 18:31:40, alexmos wrote:
> Instead of plumbing sandboxFlags through createFrame and
> FrameLoadRequest, a simpler approach is to make sandboxFlags() public in
HTMLFrameOwnerElement and access it here as ownerElement->sandboxFlags().  Is
there any reason not to make sandboxFlags() (and the rest of FrameOwner methods)
public in HTMLFrameOwnerElement?

I haven't really thought about this. +japhet

https://codereview.chromium.org/838903002/diff/60001/Source/web/WebRemoteFram...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/838903002/diff/60001/Source/web/WebRemoteFram...
Source/web/WebRemoteFrameImpl.cpp:71: // Frames need to inherit the sandbox
flags of their parent frame.
It this covered by any of our current tests?

https://codereview.chromium.org/838903002/diff/60001/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/838903002/diff/60001/public/web/WebFrameClien...
public/web/WebFrameClient.h:153: virtual WebFrame*
createChildFrame(WebLocalFrame* parent, const WebString& frameName,
WebSandboxFlags sandboxFlags) { return 0; }
s/0/nullptr since we're changing this

[alexmos, 2015-01-17 02:54:42]

https://codereview.chromium.org/838903002/diff/60001/Source/core/loader/Frame...
File Source/core/loader/FrameLoadRequest.h (right):

https://codereview.chromium.org/838903002/diff/60001/Source/core/loader/Frame...
Source/core/loader/FrameLoadRequest.h:83: , m_sandboxFlags(SandboxNone)
On 2015/01/16 19:11:19, dcheng wrote:
> We should probably consider delegated constructors =)

Oh, neat, didn't realize these are allowed. :)  I can give this a shot after we
hear from Nate about the other question, since I may end up not modifying this.

https://codereview.chromium.org/838903002/diff/60001/Source/core/loader/Frame...
Source/core/loader/FrameLoadRequest.h:138: const SandboxFlags& sandboxFlags()
const { return m_sandboxFlags; }
On 2015/01/16 19:11:19, dcheng wrote:
> Are you intentionally returning a reference here? Many accessors for POD types
> like this tend to return by value.

Unintentional, fixed.

https://codereview.chromium.org/838903002/diff/60001/Source/web/WebRemoteFram...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/838903002/diff/60001/Source/web/WebRemoteFram...
Source/web/WebRemoteFrameImpl.cpp:71: // Frames need to inherit the sandbox
flags of their parent frame.
On 2015/01/16 19:11:19, dcheng wrote:
> It this covered by any of our current tests?

LayoutTests/http/tests/security/sandbox-inherit-to-initial-document.html seems
to cover it.

The cross-site case will also be covered by a browser test I'm adding here:
https://codereview.chromium.org/797813006/

https://codereview.chromium.org/838903002/diff/60001/public/web/WebFrameClient.h
File public/web/WebFrameClient.h (right):

https://codereview.chromium.org/838903002/diff/60001/public/web/WebFrameClien...
public/web/WebFrameClient.h:153: virtual WebFrame*
createChildFrame(WebLocalFrame* parent, const WebString& frameName,
WebSandboxFlags sandboxFlags) { return 0; }
On 2015/01/16 19:11:19, dcheng wrote:
> s/0/nullptr since we're changing this

Done.

[alexmos, 2015-01-21 17:44:02]

Hi Nate - friendly ping about the question in FrameLoaderClientImpl.cpp, and
while at it, could you please also review this for Source/web owners approval?

[Nate Chapin, 2015-01-21 18:27:31]

Source/web/ changes look reasonable in general.

https://codereview.chromium.org/838903002/diff/60001/Source/web/FrameLoaderCl...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/838903002/diff/60001/Source/web/FrameLoaderCl...
Source/web/FrameLoaderClientImpl.cpp:680: FrameLoadRequest
frameRequest(m_webFrame->frame()->document(), url, name,
shouldCheckContentSecurityPolicy, sandboxFlags);
On 2015/01/16 19:11:19, dcheng wrote:
> On 2015/01/16 at 18:31:40, alexmos wrote:
> > Instead of plumbing sandboxFlags through createFrame and
> > FrameLoadRequest, a simpler approach is to make sandboxFlags() public in
> HTMLFrameOwnerElement and access it here as ownerElement->sandboxFlags().  Is
> there any reason not to make sandboxFlags() (and the rest of FrameOwner
methods)
> public in HTMLFrameOwnerElement?
> 
> I haven't really thought about this. +japhet

I have mixed feelings about making all of the FrameOwner api public on
HTMLFrameOwnerElement, but I think this is a reasonable case for exposing at
least sandboxFlags().

Unless there's a situation in which we would remove sandboxFlags() from
FrameOwner and/or HTMLFrameOwnerElement, which seems unlikely?

[alexmos, 2015-01-21 22:18:59]

On 2015/01/21 18:27:31, Nate Chapin wrote:
> Source/web/ changes look reasonable in general.
> 
>
https://codereview.chromium.org/838903002/diff/60001/Source/web/FrameLoaderCl...
> File Source/web/FrameLoaderClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/838903002/diff/60001/Source/web/FrameLoaderCl...
> Source/web/FrameLoaderClientImpl.cpp:680: FrameLoadRequest
> frameRequest(m_webFrame->frame()->document(), url, name,
> shouldCheckContentSecurityPolicy, sandboxFlags);
> On 2015/01/16 19:11:19, dcheng wrote:
> > On 2015/01/16 at 18:31:40, alexmos wrote:
> > > Instead of plumbing sandboxFlags through createFrame and
> > > FrameLoadRequest, a simpler approach is to make sandboxFlags() public in
> > HTMLFrameOwnerElement and access it here as ownerElement->sandboxFlags(). 
Is
> > there any reason not to make sandboxFlags() (and the rest of FrameOwner
> methods)
> > public in HTMLFrameOwnerElement?
> > 
> > I haven't really thought about this. +japhet
> 
> I have mixed feelings about making all of the FrameOwner api public on
> HTMLFrameOwnerElement, but I think this is a reasonable case for exposing at
> least sandboxFlags().
> 
> Unless there's a situation in which we would remove sandboxFlags() from
> FrameOwner and/or HTMLFrameOwnerElement, which seems unlikely?

I went ahead and made  HTMLFrameOwnerElement::sandboxFlags() public and
simplified the plumbing in PS6.  PTAL.  I agree it's unlikely that FrameOwner or
HTMLFrameOwnerElement would lose sandboxFlags().

[dcheng, 2015-01-21 23:19:06]

https://codereview.chromium.org/838903002/diff/100001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/838903002/diff/100001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.h:96: virtual void dispatchLoad()
override;
I think we should move this entire block to public:

There's not a lot of point to making only some of these private, since you can
call them by upcasting to FrameOwner anyway.

https://codereview.chromium.org/838903002/diff/100001/Source/web/WebRemoteFra...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/838903002/diff/100001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:71: // Frames need to inherit the sandbox
flags of their parent frame.
Should we push this logic up into FrameOwner::sandboxFlags() and just make
m_sandboxFlags a protected member? That'd let us avoid duplicating this logic
between RemoteBridgeFrameOwner and HTMLFrameOwnerElement, right?

https://codereview.chromium.org/838903002/diff/100001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:74: ASSERT(parentFrame->isRemoteFrame());
This ASSERT isn't needed because toRemoteFrame() does a release assert that the
argument is the right type.

https://codereview.chromium.org/838903002/diff/100001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:75: flags |=
toRemoteFrame(parentFrame)->securityContext()->sandboxFlags();
I don't think toRemoteFrame() is actually needed here.

[alexmos, 2015-01-22 00:37:58]

https://codereview.chromium.org/838903002/diff/100001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/838903002/diff/100001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.h:96: virtual void dispatchLoad()
override;
On 2015/01/21 23:19:06, dcheng wrote:
> I think we should move this entire block to public:
> 
> There's not a lot of point to making only some of these private, since you can
> call them by upcasting to FrameOwner anyway.

Done.  Nate, are you ok with this?

https://codereview.chromium.org/838903002/diff/100001/Source/web/WebRemoteFra...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/838903002/diff/100001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:71: // Frames need to inherit the sandbox
flags of their parent frame.
On 2015/01/21 23:19:06, dcheng wrote:
> Should we push this logic up into FrameOwner::sandboxFlags() and just make
> m_sandboxFlags a protected member? That'd let us avoid duplicating this logic
> between RemoteBridgeFrameOwner and HTMLFrameOwnerElement, right?

I agree removing the duplication is a good idea, and as we discussed in person,
I'm trying another approach where I brought this check into
FrameLoader::effectiveSandboxFlags(), where it applies uniformly to both
HTMLFrameOwnerElement and RemoteBridgeFrameOwner.  Please take a look at PS7. 
Nate - let us know if you're ok with this, since I know you brought this logic
out of there in an earlier CL.

https://codereview.chromium.org/838903002/diff/100001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:74: ASSERT(parentFrame->isRemoteFrame());
On 2015/01/21 23:19:06, dcheng wrote:
> This ASSERT isn't needed because toRemoteFrame() does a release assert that
the
> argument is the right type.

Done.

https://codereview.chromium.org/838903002/diff/100001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:75: flags |=
toRemoteFrame(parentFrame)->securityContext()->sandboxFlags();
On 2015/01/21 23:19:06, dcheng wrote:
> I don't think toRemoteFrame() is actually needed here.

Done.

[Nate Chapin, 2015-01-22 19:00:28]

https://codereview.chromium.org/838903002/diff/100001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/838903002/diff/100001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.h:96: virtual void dispatchLoad()
override;
On 2015/01/22 00:37:58, alexmos wrote:
> On 2015/01/21 23:19:06, dcheng wrote:
> > I think we should move this entire block to public:
> > 
> > There's not a lot of point to making only some of these private, since you
can
> > call them by upcasting to FrameOwner anyway.
> 
> Done.  Nate, are you ok with this?

Yeah, that's a good argument.

https://codereview.chromium.org/838903002/diff/120001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/838903002/diff/120001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1384: flags |=
parentFrame->securityContext()->sandboxFlags();
This is ok, but I can't help but feel that we should only need to do one of
these to get all the required flags.

[dcheng, 2015-01-22 19:05:53]

LGTM, as long as japhet@ has no objections.

https://codereview.chromium.org/838903002/diff/120001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/838903002/diff/120001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:1384: flags |=
parentFrame->securityContext()->sandboxFlags();
On 2015/01/22 at 19:00:28, Nate Chapin wrote:
> This is ok, but I can't help but feel that we should only need to do one of
these to get all the required flags.

I guess the counter-argument is it's nice to have one place do all the
calculations for this.

[Nate Chapin, 2015-01-22 19:12:14]

On 2015/01/22 19:05:53, dcheng wrote:
> LGTM, as long as japhet@ has no objections.
> 
>
https://codereview.chromium.org/838903002/diff/120001/Source/core/loader/Fram...
> File Source/core/loader/FrameLoader.cpp (right):
> 
>
https://codereview.chromium.org/838903002/diff/120001/Source/core/loader/Fram...
> Source/core/loader/FrameLoader.cpp:1384: flags |=
> parentFrame->securityContext()->sandboxFlags();
> On 2015/01/22 at 19:00:28, Nate Chapin wrote:
> > This is ok, but I can't help but feel that we should only need to do one of
> these to get all the required flags.
> 
> I guess the counter-argument is it's nice to have one place do all the
> calculations for this.

Did I mention that m_forcedSandboxFlags feels dirty to me to? :) My concerns
shouldn't block this patch, LGTM.

[alexmos, 2015-01-22 21:39:34]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-01-22 21:40:52]

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

[commit-bot: I haz the power, 2015-01-22 21:50:29]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188835