Plumb the correct owner document through DocumentInit::m_owner.

Plumb the correct owner document through DocumentInit::m_owner.

The current code tries to determine the security origin to inherit (if
any) too late in document initialization. This results in strange and
hard to understand behavior.

For example, opener is not set until /after/ the document's security
context is already initialized. To make this work, initSecurityContext()
has a heuristic: if it should have inherited a security origin (e.g. the
URL is about:blank) but there's nothing to inherit from, it initializes
the security origin as unique, but then marks initialization as failed.

When the opener is /actually/ set, it then calls initSecurityContext()
again. Since the security context hasn't been marked as initialized yet,
the reinitialization is allowed to proceed, and now the frame inherits
its opener's security origin.

Rather than going through this elaborate dance, this CL gets rid of it
and proactively plumbs through the correct owner document to use. With
these changes:

- A security context can never be reinitialized. This requires passing
  the opener around when creating new windows, so that DocumentLoader
  can initialize the owner document correctly.
- javascript: URLs have different inheritance rules: the loading
  machinery can now just directly pass in the correct owner document.
- The exception for reusing a Window object when navigating from the
  initial empty Document has been removed: now it strictly follows the
  spec and reuses it iff it is same-origin to the new Document.

BUG=583445
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/9e24bd35f58fff1562b0784be8ab2e612ece6408
Cr-Commit-Position: refs/heads/master@{#378508}

[dcheng, 2016-02-22 23:51:07]

Description was changed from

==========
Use DocumentInit::m_owner more proactively

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

to

==========
Plumb the correct owner document through DocumentInit::m_owner.

BUG=583445
==========

[dcheng, 2016-02-22 23:51:07]

dcheng@chromium.org changed reviewers:
+ alexmos@chromium.org, creis@chromium.org, esprehn@chromium.org

[dcheng, 2016-02-23 00:02:15]

Description was changed from

==========
Plumb the correct owner document through DocumentInit::m_owner.

BUG=583445
==========

to

==========
Plumb the correct owner document through DocumentInit::m_owner.

The current code tries to determine the security origin to inherit (if
any) too late in document initialization. This results in strange and
hard to understand behavior.

For example, opener is not set until /after/ the document's security
context is already initialized. To make this work, initSecurityContext()
has a heuristic: if it should have inherited a security origin (e.g. the
URL is about:blank) but there's nothing to inherit from, it initializes
the security origin as unique, but then marks initialization as failed.

When the opener is /actually/ set, it then calls initSecurityContext()
again. Since the security context hasn't been marked as initialized yet,
the reinitialization is allowed to proceed, and now the frame inherits
its opener's security origin.

Rather than going through this elaborate dance, this CL gets rid of it
and proactively plumbs through the correct owner document to use. With
these changes:

- A security context can never be reinitialized. This requires passing
  the opener around when creating new windows, so that DocumentLoader
  can initialize the owner document correctly.
- javascript: URLs have different inheritance rules: the loading
  machinery can now just directly pass in the correct owner document.

BUG=583445
==========

[dcheng, 2016-02-23 00:11:30]

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

[dcheng, 2016-02-23 00:11:32]

There are a few layout test crashes still, mostly around page popups. I thought
I had fixed them all, but they came back. I'll be poking at those to try to
address them again, but in the meanwhile, I think this is ready for a first
pass.

https://codereview.chromium.org/1685003002/diff/160001/content/renderer/rende...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1685003002/diff/160001/content/renderer/rende...
content/renderer/render_frame_proxy.cc:93:
RenderFrameImpl::ResolveOpener(opener_routing_id, nullptr);
It's a bit awkward to do this lookup again, but this appears unavoidable without
restructuring some of this code.

https://codereview.chromium.org/1685003002/diff/160001/content/shell/renderer...
File content/shell/renderer/layout_test/blink_test_runner.cc (right):

https://codereview.chromium.org/1685003002/diff/160001/content/shell/renderer...
content/shell/renderer/layout_test/blink_test_runner.cc:854: if (for_new_test) 
{
I'm not quite sure what the right thing to do here is: it doesn't make sense to
always blow these away. The problem is that keeping this here would blow away
the opener that gets set when creating the new view in
RenderViewImpl::Initialize, since this bit runs /after/ the render view is
already initialized.

There are probably other things that this should guard, but I just made the
minimum logical change to get layout tests working.

https://codereview.chromium.org/1685003002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/synchronous-frame-load-in-javascript-url-inherits-correct-origin.html
(right):

https://codereview.chromium.org/1685003002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/synchronous-frame-load-in-javascript-url-inherits-correct-origin.html:38:
This test passes if the alerted location is the same origin as the main frame.
I believe this behavior change is OK: because we inherit the right owner
document now, it's not reaching cross-origin anymore (pre
https://crbug.com/99750, I believe this would have reached into the victim
iframe, which is bad, because that *is* cross origin).

[dcheng, 2016-02-24 21:59:01]

dcheng@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[dcheng, 2016-02-24 21:59:02]

OK, I think this is ready for another look. I've tested that some of the
formerly broken app window tests are now working… we'll see if the bots are
actually OK with it though.

https://codereview.chromium.org/1685003002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/SecurityContext.cpp (left):

https://codereview.chromium.org/1685003002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/SecurityContext.cpp:69: return true;
There are at least two scenarios to consider here:

1) Creating a new top-level page with something like noopener, which prevents
inheriting the security origin. In that case, the LocalDOMWindow object has a
unique origin. No other frame could have injected properties/event listeners
into it, so it's OK to just use a new Window object. From an efficiency
standpoint, it's probably more efficient to reuse the Window object, but from a
perspective of understanding the code, I think it's nice to remove this edge
case.

2) Unfortunately, this doesn't interact well with page popups. Page popups
currently a LocalDOMWindow supplement: without this logic, the LocalDOMWindow
changes after the supplement is installed. Then we crash inside v8 bindings when
the page popup tries to access its bindings. To fix this, I've changed the page
popup supplement to be bound to a frame.

https://codereview.chromium.org/1685003002/diff/230001/extensions/browser/app...
File extensions/browser/app_window/app_window_contents.cc (right):

https://codereview.chromium.org/1685003002/diff/230001/extensions/browser/app...
extensions/browser/app_window/app_window_contents.cc:35: context,
creator_frame->GetSiteInstance());
Without this, we always create a new process. This causes the opener inheritance
bits in Blink to fallover: it can't inherit security origin across a process
boundary.

I /think/ in this case we should always start in the site instance of the
creator... I /think/.

[dcheng, 2016-02-24 22:23:29]

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

[dcheng, 2016-02-24 22:23:30]

+benwells for chrome/browser/apps changes as well

[Devlin, 2016-02-25 00:12:43]

extensions lgtm

[Charlie Reis, 2016-02-25 01:08:08]

[+alexmos for question about forceSandboxFlags]

I looked at content/ and it seems mostly good.  One possible bug below.

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:892: web_frame =
blink::WebLocalFrame::createProvisional(
What about this branch?  Looks like we're losing the opener here.

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
content/renderer/render_view_impl.cc:809: // Ensure that sandbox flags are
inherited from an opener in a different
I'm wondering if the rest of this block needs any cleanup as well, now that we
don't have to call setOpener anymore.  It isn't clear to me if it's really
specific to the main frame, in which case it might make sense to move it inside
the frame's initialization?

Anyway, maybe that's a question for Alex and we can leave it here for now.

https://codereview.chromium.org/1685003002/diff/250001/content/shell/renderer...
File content/shell/renderer/layout_test/blink_test_runner.h (right):

https://codereview.chromium.org/1685003002/diff/250001/content/shell/renderer...
content/shell/renderer/layout_test/blink_test_runner.h:161: void Reset(bool
for_new_test);
Let's add some comment about what this method and its parameter do.

[dcheng, 2016-02-25 01:13:28]

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:892: web_frame =
blink::WebLocalFrame::createProvisional(
On 2016/02/25 at 01:08:08, Charlie Reis (slow til Mar 7) wrote:
> What about this branch?  Looks like we're losing the opener here.

When we create a provisional frame, we copy a bunch of information over: opener
is one of those bits.

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
content/renderer/render_view_impl.cc:809: // Ensure that sandbox flags are
inherited from an opener in a different
On 2016/02/25 at 01:08:08, Charlie Reis (slow til Mar 7) wrote:
> I'm wondering if the rest of this block needs any cleanup as well, now that we
don't have to call setOpener anymore.  It isn't clear to me if it's really
specific to the main frame, in which case it might make sense to move it inside
the frame's initialization?
> 
> Anyway, maybe that's a question for Alex and we can leave it here for now.

Maybe… but this patch has affected a lot more plumbing than I was hoping to
touch. It probably does make sense to clean this up too, but probably in a
separate patch.

https://codereview.chromium.org/1685003002/diff/250001/content/shell/renderer...
File content/shell/renderer/layout_test/blink_test_runner.h (right):

https://codereview.chromium.org/1685003002/diff/250001/content/shell/renderer...
content/shell/renderer/layout_test/blink_test_runner.h:161: void Reset(bool
for_new_test);
On 2016/02/25 at 01:08:08, Charlie Reis (slow til Mar 7) wrote:
> Let's add some comment about what this method and its parameter do.

Done (I'll upload this with my next patch, which tries to fix the app window
tests)

[dcheng, 2016-02-25 20:38:49]

dcheng@chromium.org changed reviewers:
+ asargent@chromium.org
- benwells@chromium.org

[dcheng, 2016-02-25 20:38:50]

OK, I need to fix the Mac build and several extension unit tests, but this is
ready for re-review.

+asargent, mind looking at the bits related to chrome app windows?

[Charlie Reis, 2016-02-25 21:21:25]

content/ LGTM.

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
content/renderer/render_frame_impl.cc:892: web_frame =
blink::WebLocalFrame::createProvisional(
On 2016/02/25 01:13:28, dcheng wrote:
> On 2016/02/25 at 01:08:08, Charlie Reis (slow til Mar 7) wrote:
> > What about this branch?  Looks like we're losing the opener here.
> 
> When we create a provisional frame, we copy a bunch of information over:
opener
> is one of those bits.

Acknowledged.

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
content/renderer/render_view_impl.cc:809: // Ensure that sandbox flags are
inherited from an opener in a different
On 2016/02/25 01:13:28, dcheng wrote:
> On 2016/02/25 at 01:08:08, Charlie Reis (slow til Mar 7) wrote:
> > I'm wondering if the rest of this block needs any cleanup as well, now that
we
> don't have to call setOpener anymore.  It isn't clear to me if it's really
> specific to the main frame, in which case it might make sense to move it
inside
> the frame's initialization?
> > 
> > Anyway, maybe that's a question for Alex and we can leave it here for now.
> 
> Maybe… but this patch has affected a lot more plumbing than I was hoping to
> touch. It probably does make sense to clean this up too, but probably in a
> separate patch.

Separate patch is fine.  Just wanted to raise the question for Alex to see.

[dcheng, 2016-02-25 21:48:47]

Description was changed from

==========
Plumb the correct owner document through DocumentInit::m_owner.

The current code tries to determine the security origin to inherit (if
any) too late in document initialization. This results in strange and
hard to understand behavior.

For example, opener is not set until /after/ the document's security
context is already initialized. To make this work, initSecurityContext()
has a heuristic: if it should have inherited a security origin (e.g. the
URL is about:blank) but there's nothing to inherit from, it initializes
the security origin as unique, but then marks initialization as failed.

When the opener is /actually/ set, it then calls initSecurityContext()
again. Since the security context hasn't been marked as initialized yet,
the reinitialization is allowed to proceed, and now the frame inherits
its opener's security origin.

Rather than going through this elaborate dance, this CL gets rid of it
and proactively plumbs through the correct owner document to use. With
these changes:

- A security context can never be reinitialized. This requires passing
  the opener around when creating new windows, so that DocumentLoader
  can initialize the owner document correctly.
- javascript: URLs have different inheritance rules: the loading
  machinery can now just directly pass in the correct owner document.

BUG=583445
==========

to

==========
Plumb the correct owner document through DocumentInit::m_owner.

The current code tries to determine the security origin to inherit (if
any) too late in document initialization. This results in strange and
hard to understand behavior.

For example, opener is not set until /after/ the document's security
context is already initialized. To make this work, initSecurityContext()
has a heuristic: if it should have inherited a security origin (e.g. the
URL is about:blank) but there's nothing to inherit from, it initializes
the security origin as unique, but then marks initialization as failed.

When the opener is /actually/ set, it then calls initSecurityContext()
again. Since the security context hasn't been marked as initialized yet,
the reinitialization is allowed to proceed, and now the frame inherits
its opener's security origin.

Rather than going through this elaborate dance, this CL gets rid of it
and proactively plumbs through the correct owner document to use. With
these changes:

- A security context can never be reinitialized. This requires passing
  the opener around when creating new windows, so that DocumentLoader
  can initialize the owner document correctly.
- javascript: URLs have different inheritance rules: the loading
  machinery can now just directly pass in the correct owner document.
- The exception for reusing a Window object when navigating from the
  initial empty Document has been removed: now it strictly follows the
  spec and reuses it iff it is same-origin to the new Document.

BUG=583445
==========

[asargent_no_longer_on_chrome, 2016-02-26 18:19:45]

On 2016/02/25 21:21:25, Charlie Reis (slow til Mar 7) wrote:
> content/ LGTM.
> 
>
https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:892: web_frame =
> blink::WebLocalFrame::createProvisional(
> On 2016/02/25 01:13:28, dcheng wrote:
> > On 2016/02/25 at 01:08:08, Charlie Reis (slow til Mar 7) wrote:
> > > What about this branch?  Looks like we're losing the opener here.
> > 
> > When we create a provisional frame, we copy a bunch of information over:
> opener
> > is one of those bits.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
> content/renderer/render_view_impl.cc:809: // Ensure that sandbox flags are
> inherited from an opener in a different
> On 2016/02/25 01:13:28, dcheng wrote:
> > On 2016/02/25 at 01:08:08, Charlie Reis (slow til Mar 7) wrote:
> > > I'm wondering if the rest of this block needs any cleanup as well, now
that
> we
> > don't have to call setOpener anymore.  It isn't clear to me if it's really
> > specific to the main frame, in which case it might make sense to move it
> inside
> > the frame's initialization?
> > > 
> > > Anyway, maybe that's a question for Alex and we can leave it here for now.
> > 
> > Maybe… but this patch has affected a lot more plumbing than I was hoping to
> > touch. It probably does make sense to clean this up too, but probably in a
> > separate patch.
> 
> Separate patch is fine.  Just wanted to raise the question for Alex to see.

extensions and apps changes lgtm

You might need benwells or tapted to review chrome/browser/ui/cocoa/apps/* files
since I'm not in OWNERS there

[alexmos, 2016-02-26 19:55:32]

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

[alexmos, 2016-02-26 19:55:34]

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
content/renderer/render_view_impl.cc:807: // the browser asking us to set our
opener to another frame.
You might want to update this comment, since we don't set the opener in this
block anymore.

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
content/renderer/render_view_impl.cc:809: // Ensure that sandbox flags are
inherited from an opener in a different
On 2016/02/25 21:21:25, Charlie Reis (slow til Mar 7) wrote:
> On 2016/02/25 01:13:28, dcheng wrote:
> > On 2016/02/25 at 01:08:08, Charlie Reis (slow til Mar 7) wrote:
> > > I'm wondering if the rest of this block needs any cleanup as well, now
that
> we
> > don't have to call setOpener anymore.  It isn't clear to me if it's really
> > specific to the main frame, in which case it might make sense to move it
> inside
> > the frame's initialization?
> > > 
> > > Anyway, maybe that's a question for Alex and we can leave it here for now.
> > 
> > Maybe… but this patch has affected a lot more plumbing than I was hoping to
> > touch. It probably does make sense to clean this up too, but probably in a
> > separate patch.
> 
> Separate patch is fine.  Just wanted to raise the question for Alex to see.

Yes, this is specific to the main frame (the scenario where a newly created
popup window needs to inherit its opener's sandbox flags).  I'm fine cleaning
that up in a separate patch later and moving it to something like
CreateMainFrame.

I didn't look through this CL in detail, but it looks like this will now set the
opener for both browser-initiated and renderer-initiated RV creation, since it
drops the was_created_by_renderer check (and it's great that this is now done
consistently).  Did the renderer-initiated path previously rely solely on
setting opener in blink::CreateWindow, and if so, why does createWindowHelper
still need to set the opener?

[dcheng, 2016-02-26 22:45:02]

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

[dcheng, 2016-02-26 22:45:33]

dcheng@chromium.org changed reviewers:
+ sky@chromium.org
- thakis@chromium.org

[dcheng, 2016-02-26 22:52:29]

OK, I think all the failing tests /should/ be fixed.

None of the already-reviewed code has changed (outside of rebasing), so at this
point, the only things that need review are:
+sky for //chrome
+esprehn for //third_party/WebKit

Thanks!

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
content/renderer/render_view_impl.cc:807: // the browser asking us to set our
opener to another frame.
On 2016/02/26 at 19:55:34, alexmos wrote:
> You might want to update this comment, since we don't set the opener in this
block anymore.

Hmm... what would you suggest for this comment? It's still true: it's just the
setting of the opener happens deeper down in the stack, when creating the
WebFrames.

https://codereview.chromium.org/1685003002/diff/410001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/keyboard_controller_browsertest.cc (right):

https://codereview.chromium.org/1685003002/diff/410001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/keyboard_controller_browsertest.cc:171:
"background.js")))))))
Needed to fake out an extension background page: the background page is
referenced by the apps_browsertest_utils.h:CreateAppWindowFromParams(), which
uses it to determine the frame creating the app window.

https://codereview.chromium.org/1685003002/diff/410001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/1685003002/diff/410001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:760: // in
a random RenderFrameHost is Good Enough™.
This is kind of a hack, but in a unit test, this is probably the simplest thing
to do. Since this is a unit test, it can't actually touch any renderer bits, so
the security origin inheritance stuff doesn't matter.

https://codereview.chromium.org/1685003002/diff/410001/chrome/renderer/chrome...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://codereview.chromium.org/1685003002/diff/410001/chrome/renderer/chrome...
chrome/renderer/chrome_render_view_observer.cc:162: void
ChromeRenderViewObserver::DidCommitProvisionalLoad(
Blink no longer always blindly reuses a Window object when navigating away from
the initial empty document: this better matches the spec-described behavior for
reusing Window objects.

However, since the Window object changes now, injecting the scripts at
DidStartLoading() is too early: the Window object hasn't been updated yet, and
the scripts get blown away when the Window object is replaced when committing
the provisional load.

(Note that this change isn't actually web-visible, but it affects a lot of
test/embedder-driven code that assumed this persistence would always happen.)

https://codereview.chromium.org/1685003002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/1685003002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/CreateWindow.cpp:133: if
(openerFrame.settings() && !openerFrame.settings()->supportsMultipleWindows())
I wanted to remove this... but it turns out this is a setting exposed by Android
WebView.

Oh well.

[alexmos, 2016-02-26 23:54:47]

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
content/renderer/render_view_impl.cc:807: // the browser asking us to set our
opener to another frame.
On 2016/02/26 22:52:29, dcheng wrote:
> On 2016/02/26 at 19:55:34, alexmos wrote:
> > You might want to update this comment, since we don't set the opener in this
> block anymore.
> 
> Hmm... what would you suggest for this comment? It's still true: it's just the
> setting of the opener happens deeper down in the stack, when creating the
> WebFrames.

I just thought it was confusing that it talks about setting the opener for a
block that is setting sandbox flags.  Doesn't the opener get set above this now,
as part of CreateFrameProxy or CreateMainFrame?  And that happens without
looking at was_created_by_renderer now, so "we weren't created by a renderer"
doesn't apply anymore, right?  It might be ok to just remove this comment; the
one inside this if block might be enough.

[sky, 2016-02-27 00:27:00]

LGTM

[dcheng, 2016-02-27 01:41:21]

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
content/renderer/render_view_impl.cc:807: // the browser asking us to set our
opener to another frame.
On 2016/02/26 at 23:54:47, alexmos wrote:
> On 2016/02/26 22:52:29, dcheng wrote:
> > On 2016/02/26 at 19:55:34, alexmos wrote:
> > > You might want to update this comment, since we don't set the opener in
this
> > block anymore.
> > 
> > Hmm... what would you suggest for this comment? It's still true: it's just
the
> > setting of the opener happens deeper down in the stack, when creating the
> > WebFrames.
> 
> I just thought it was confusing that it talks about setting the opener for a
block that is setting sandbox flags.  Doesn't the opener get set above this now,
as part of CreateFrameProxy or CreateMainFrame?  And that happens without
looking at was_created_by_renderer now, so "we weren't created by a renderer"
doesn't apply anymore, right?  It might be ok to just remove this comment; the
one inside this if block might be enough.

Done. I also collapsed the two if blocks together.

https://codereview.chromium.org/1685003002/diff/250001/content/renderer/rende...
content/renderer/render_view_impl.cc:809: // Ensure that sandbox flags are
inherited from an opener in a different
On 2016/02/26 at 19:55:34, alexmos wrote:
> On 2016/02/25 21:21:25, Charlie Reis (slow til Mar 7) wrote:
> > On 2016/02/25 01:13:28, dcheng wrote:
> > > On 2016/02/25 at 01:08:08, Charlie Reis (slow til Mar 7) wrote:
> > > > I'm wondering if the rest of this block needs any cleanup as well, now
that
> > we
> > > don't have to call setOpener anymore.  It isn't clear to me if it's really
> > > specific to the main frame, in which case it might make sense to move it
> > inside
> > > the frame's initialization?
> > > > 
> > > > Anyway, maybe that's a question for Alex and we can leave it here for
now.
> > > 
> > > Maybe… but this patch has affected a lot more plumbing than I was hoping
to
> > > touch. It probably does make sense to clean this up too, but probably in a
> > > separate patch.
> > 
> > Separate patch is fine.  Just wanted to raise the question for Alex to see.
> 
> Yes, this is specific to the main frame (the scenario where a newly created
popup window needs to inherit its opener's sandbox flags).  I'm fine cleaning
that up in a separate patch later and moving it to something like
CreateMainFrame.
> 
> I didn't look through this CL in detail, but it looks like this will now set
the opener for both browser-initiated and renderer-initiated RV creation, since
it drops the was_created_by_renderer check (and it's great that this is now done
consistently).  Did the renderer-initiated path previously rely solely on
setting opener in blink::CreateWindow, and if so, why does createWindowHelper
still need to set the opener?

createWindow() doesn't *always* create a new window, and in that case, Blink
still needs to update the state in the renderer itself.

[dcheng, 2016-02-27 01:41:58]

Description was changed from

==========
Plumb the correct owner document through DocumentInit::m_owner.

The current code tries to determine the security origin to inherit (if
any) too late in document initialization. This results in strange and
hard to understand behavior.

For example, opener is not set until /after/ the document's security
context is already initialized. To make this work, initSecurityContext()
has a heuristic: if it should have inherited a security origin (e.g. the
URL is about:blank) but there's nothing to inherit from, it initializes
the security origin as unique, but then marks initialization as failed.

When the opener is /actually/ set, it then calls initSecurityContext()
again. Since the security context hasn't been marked as initialized yet,
the reinitialization is allowed to proceed, and now the frame inherits
its opener's security origin.

Rather than going through this elaborate dance, this CL gets rid of it
and proactively plumbs through the correct owner document to use. With
these changes:

- A security context can never be reinitialized. This requires passing
  the opener around when creating new windows, so that DocumentLoader
  can initialize the owner document correctly.
- javascript: URLs have different inheritance rules: the loading
  machinery can now just directly pass in the correct owner document.
- The exception for reusing a Window object when navigating from the
  initial empty Document has been removed: now it strictly follows the
  spec and reuses it iff it is same-origin to the new Document.

BUG=583445
==========

to

==========
Plumb the correct owner document through DocumentInit::m_owner.

The current code tries to determine the security origin to inherit (if
any) too late in document initialization. This results in strange and
hard to understand behavior.

For example, opener is not set until /after/ the document's security
context is already initialized. To make this work, initSecurityContext()
has a heuristic: if it should have inherited a security origin (e.g. the
URL is about:blank) but there's nothing to inherit from, it initializes
the security origin as unique, but then marks initialization as failed.

When the opener is /actually/ set, it then calls initSecurityContext()
again. Since the security context hasn't been marked as initialized yet,
the reinitialization is allowed to proceed, and now the frame inherits
its opener's security origin.

Rather than going through this elaborate dance, this CL gets rid of it
and proactively plumbs through the correct owner document to use. With
these changes:

- A security context can never be reinitialized. This requires passing
  the opener around when creating new windows, so that DocumentLoader
  can initialize the owner document correctly.
- javascript: URLs have different inheritance rules: the loading
  machinery can now just directly pass in the correct owner document.
- The exception for reusing a Window object when navigating from the
  initial empty Document has been removed: now it strictly follows the
  spec and reuses it iff it is same-origin to the new Document.

BUG=583445
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[dcheng, 2016-02-27 01:42:04]

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

[commit-bot: I haz the power, 2016-02-27 01:44:08]

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

[dcheng, 2016-02-27 03:41:45]

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

[dcheng, 2016-02-27 03:41:46]

+haraken@ and jochen@ as well

[dcheng, 2016-02-27 04:58:11]

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

[commit-bot: I haz the power, 2016-02-27 04:58:24]

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

[commit-bot: I haz the power, 2016-02-27 05:57:23]

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

[commit-bot: I haz the power, 2016-02-27 05:57:25]

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

[dcheng, 2016-02-27 06:04:49]

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

[commit-bot: I haz the power, 2016-02-27 06:05:21]

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

[commit-bot: I haz the power, 2016-02-27 07:27:39]

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

[commit-bot: I haz the power, 2016-02-27 07:27:41]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-02-29 02:17:07]

I'm not eligible to review the security aspects of this CL, so I'll delegate the
decision to jochen@.

[jochen (gone - plz use gerrit), 2016-02-29 16:37:23]

https://codereview.chromium.org/1685003002/diff/450001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/1685003002/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:768:
RefPtrWillBeRawPtr<Document> document =
frame->localDOMWindow()->installNewDocument(mimeType, init);
I wonder whether we should RELEASE_ASSERT that we don't change the security
origin here?

[dcheng, 2016-02-29 17:49:24]

https://codereview.chromium.org/1685003002/diff/450001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/1685003002/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:768:
RefPtrWillBeRawPtr<Document> document =
frame->localDOMWindow()->installNewDocument(mimeType, init);
On 2016/02/29 at 16:37:23, jochen wrote:
> I wonder whether we should RELEASE_ASSERT that we don't change the security
origin here?

It's kind of expected that this will normally change though: this is used for
both JS navigations and regular navigations. I'm not sure what a meaningful
assert here would be.

[jochen (gone - plz use gerrit), 2016-03-01 14:31:15]

On 2016/02/29 at 17:49:24, dcheng wrote:
>
https://codereview.chromium.org/1685003002/diff/450001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):
> 
>
https://codereview.chromium.org/1685003002/diff/450001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/loader/DocumentLoader.cpp:768:
RefPtrWillBeRawPtr<Document> document =
frame->localDOMWindow()->installNewDocument(mimeType, init);
> On 2016/02/29 at 16:37:23, jochen wrote:
> > I wonder whether we should RELEASE_ASSERT that we don't change the security
origin here?
> 
> It's kind of expected that this will normally change though: this is used for
both JS navigations and regular navigations. I'm not sure what a meaningful
assert here would be.

for regular navigations, there shouldn't be a previous security origin (or
about:blank with a unique origin)?

[jochen (gone - plz use gerrit), 2016-03-01 14:31:40]

I assume you've also tested this with the spreadsheet thing that broke last
time?

[dcheng, 2016-03-01 16:42:51]

On 2016/03/01 at 14:31:15, jochen wrote:
> On 2016/02/29 at 17:49:24, dcheng wrote:
> >
https://codereview.chromium.org/1685003002/diff/450001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):
> > 
> >
https://codereview.chromium.org/1685003002/diff/450001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/loader/DocumentLoader.cpp:768:
RefPtrWillBeRawPtr<Document> document =
frame->localDOMWindow()->installNewDocument(mimeType, init);
> > On 2016/02/29 at 16:37:23, jochen wrote:
> > > I wonder whether we should RELEASE_ASSERT that we don't change the
security origin here?
> > 
> > It's kind of expected that this will normally change though: this is used
for both JS navigations and regular navigations. I'm not sure what a meaningful
assert here would be.
> 
> for regular navigations, there shouldn't be a previous security origin (or
about:blank with a unique origin)?

Sorry, I don't think I follow: what do you mean there shouldn't be a previous
security origin? DocumentInit::m_owner should be null? Or something else?

[jochen (gone - plz use gerrit), 2016-03-01 17:19:25]

to summarize the offline discussion:

- lgtm if you manually verify the docs thing
- I think it should be possible to write a regression test for the docs bug, but
that's ok for a follow-up. I think what happened is that docs relied on the CSP
being inherit from the parent from on javascript: navigations
- I meant that we could plumb down the info that the navigation is an
javascript: navigation and release assert that it then doesn't change the
origin. But I agree that this only has limited value

[dcheng, 2016-03-01 17:28:48]

Just manually verified that scrolling (from
https://bugs.chromium.org/p/chromium/issues/detail?id=585091) works, even with
this patch.

[dcheng, 2016-03-01 17:31:58]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-03-01 17:31:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, creis@chromium.org, asargent@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1685003002/#ps450001
(title: "Rebase")

[commit-bot: I haz the power, 2016-03-01 17:32:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1685003002/450001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1685003002/450001

[dcheng, 2016-03-01 18:20:32]

+estark fyi

[commit-bot: I haz the power, 2016-03-01 19:16:04]

Committed patchset #24 (id:450001)

[commit-bot: I haz the power, 2016-03-01 19:18:02]

Description was changed from

==========
Plumb the correct owner document through DocumentInit::m_owner.

The current code tries to determine the security origin to inherit (if
any) too late in document initialization. This results in strange and
hard to understand behavior.

For example, opener is not set until /after/ the document's security
context is already initialized. To make this work, initSecurityContext()
has a heuristic: if it should have inherited a security origin (e.g. the
URL is about:blank) but there's nothing to inherit from, it initializes
the security origin as unique, but then marks initialization as failed.

When the opener is /actually/ set, it then calls initSecurityContext()
again. Since the security context hasn't been marked as initialized yet,
the reinitialization is allowed to proceed, and now the frame inherits
its opener's security origin.

Rather than going through this elaborate dance, this CL gets rid of it
and proactively plumbs through the correct owner document to use. With
these changes:

- A security context can never be reinitialized. This requires passing
  the opener around when creating new windows, so that DocumentLoader
  can initialize the owner document correctly.
- javascript: URLs have different inheritance rules: the loading
  machinery can now just directly pass in the correct owner document.
- The exception for reusing a Window object when navigating from the
  initial empty Document has been removed: now it strictly follows the
  spec and reuses it iff it is same-origin to the new Document.

BUG=583445
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Plumb the correct owner document through DocumentInit::m_owner.

The current code tries to determine the security origin to inherit (if
any) too late in document initialization. This results in strange and
hard to understand behavior.

For example, opener is not set until /after/ the document's security
context is already initialized. To make this work, initSecurityContext()
has a heuristic: if it should have inherited a security origin (e.g. the
URL is about:blank) but there's nothing to inherit from, it initializes
the security origin as unique, but then marks initialization as failed.

When the opener is /actually/ set, it then calls initSecurityContext()
again. Since the security context hasn't been marked as initialized yet,
the reinitialization is allowed to proceed, and now the frame inherits
its opener's security origin.

Rather than going through this elaborate dance, this CL gets rid of it
and proactively plumbs through the correct owner document to use. With
these changes:

- A security context can never be reinitialized. This requires passing
  the opener around when creating new windows, so that DocumentLoader
  can initialize the owner document correctly.
- javascript: URLs have different inheritance rules: the loading
  machinery can now just directly pass in the correct owner document.
- The exception for reusing a Window object when navigating from the
  initial empty Document has been removed: now it strictly follows the
  spec and reuses it iff it is same-origin to the new Document.

BUG=583445
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/9e24bd35f58fff1562b0784be8ab2e612ece6408
Cr-Commit-Position: refs/heads/master@{#378508}
==========

[commit-bot: I haz the power, 2016-03-01 19:18:04]

Patchset 24 (id:??) landed as
https://crrev.com/9e24bd35f58fff1562b0784be8ab2e612ece6408
Cr-Commit-Position: refs/heads/master@{#378508}

[sof, 2016-03-02 07:26:18]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2016-03-02 07:26:19]

https://codereview.chromium.org/1685003002/diff/450001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1685003002/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:4920: ASSERT(!securityOrigin());
Does this merit being a release assert?

[dcheng, 2016-03-02 07:36:05]

https://codereview.chromium.org/1685003002/diff/450001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1685003002/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:4920: ASSERT(!securityOrigin());
On 2016/03/02 at 07:26:19, sof wrote:
> Does this merit being a release assert?

In the current form, I think it's fine being an ASSERT. It's no longer public,
and it's only called in the constructor.

[Shu Chen, 2016-03-02 15:06:05]

shuchen@chromium.org changed reviewers:
+ shuchen@chromium.org

[Shu Chen, 2016-03-02 15:06:07]

This cl broke the IME window, please see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re....

However, my draft cl https://codereview.chromium.org/1756853002 fails to fix the
problem.
The IME window still reports: Error in response to input.ime.createWindow:
SecurityError: Blocked a frame with origin
"chrome-extension://hjmfdplinagacoddgleifabikjmipagg" from accessing a
cross-origin frame.

dcheng@, can you please help me diagnose the issue? Thanks!

[Shu Chen, 2016-03-02 15:11:28]

On 2016/03/02 at 15:06:07, Shu Chen wrote:
> This cl broke the IME window, please see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re....
> 
> However, my draft cl https://codereview.chromium.org/1756853002 fails to fix
the problem.
> The IME window still reports: Error in response to input.ime.createWindow:
SecurityError: Blocked a frame with origin
"chrome-extension://hjmfdplinagacoddgleifabikjmipagg" from accessing a
cross-origin frame.
> 
> dcheng@, can you please help me diagnose the issue? Thanks!

Sorry, I've just figured it out: I forgot to set the correct SiteInstance.