<iframe sandbox> should inherit through <a target='_blank'>.

<iframe sandbox> should inherit through <a target='_blank'>.

We already do this properly for 'window.open', but we intentionally
dropped targeted anchor navigation in https://crbug.com/353253. This
patch reverts that decision.

https://bugzilla.mozilla.org/show_bug.cgi?id=1037381#c1 walks through
the relevant portions of the HTML spec; we're simply wrong here.

BUG=393401, 353253
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191352

[Mike West, 2015-03-03 09:35:36]

mkwst@chromium.org changed reviewers:
+ yoav@yoav.ws

[Mike West, 2015-03-03 09:36:24]

mkwst@chromium.org changed reviewers:
+ sigbjornf@opera.com

[Mike West, 2015-03-03 09:36:25]

Yoav, Sigbjorn, mind taking a look?

[sof, 2015-03-03 09:52:38]

https://codereview.chromium.org/967423005/diff/1/Source/core/page/CreateWindo...
File Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/967423005/diff/1/Source/core/page/CreateWindo...
Source/core/page/CreateWindow.cpp:193: if (newFrame != &openerFrame && newFrame
!= openerFrame.tree().top())
Can we common up and move this check to the local createWindow() instead?

[Mike West, 2015-03-03 10:25:37]

https://codereview.chromium.org/967423005/diff/1/Source/core/page/CreateWindo...
File Source/core/page/CreateWindow.cpp (right):

https://codereview.chromium.org/967423005/diff/1/Source/core/page/CreateWindo...
Source/core/page/CreateWindow.cpp:193: if (newFrame != &openerFrame && newFrame
!= openerFrame.tree().top())
On 2015/03/03 at 09:52:38, sof wrote:
> Can we common up and move this check to the local createWindow() instead?

Sounds totally reasonable.

[sof, 2015-03-03 10:40:37]

lgtm

[Mike West, 2015-03-03 11:13:28]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-03-03 11:14:35]

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

[Mike West, 2015-03-03 12:17:31]

Amusingly, we did the exact opposite of this patch at
https://code.google.com/p/chromium/issues/detail?id=353253. CCing japhet@ as an
FYI that I'm changing things to match Firefox's implementation (and the spec).

Happily, though, the test japhet@ added revealed a bug in this implementation.
Fixing now.

[Mike West, 2015-03-03 12:17:35]

The CQ bit was unchecked by mkwst@chromium.org

[Mike West, 2015-03-03 13:44:21]

mkwst@chromium.org changed reviewers:
+ japhet@chromium.org
- yoav@yoav.ws

[Mike West, 2015-03-03 13:44:21]

Sigbjorn, Nate, would you mind taking a(nother) look?

[sof, 2015-03-03 19:01:50]

On 2015/03/03 13:44:21, Mike West wrote:
> Sigbjorn, Nate, would you mind taking a(nother) look?

A couple of tests appear to time out.

I'm hoping Nate will have the time to have look; that would be ideal. I can go
through the earlier issues and get to grips with details tomorrow, if not
possible.

[sof, 2015-03-03 19:12:41]

On 2015/03/03 19:01:50, sof wrote:
> On 2015/03/03 13:44:21, Mike West wrote:
> > Sigbjorn, Nate, would you mind taking a(nother) look?
> 
> A couple of tests appear to time out.
> 
> I'm hoping Nate will have the time to have look; that would be ideal. I can go
> through the earlier issues and get to grips with details tomorrow, if not
> possible.

sorry, "have a look". me write pretty one day.

[Mike West, 2015-03-03 19:16:10]

On 2015/03/03 at 19:12:41, sigbjornf wrote:
> On 2015/03/03 19:01:50, sof wrote:
> > On 2015/03/03 13:44:21, Mike West wrote:
> > > Sigbjorn, Nate, would you mind taking a(nother) look?
> > 
> > A couple of tests appear to time out.
> > 
> > I'm hoping Nate will have the time to have look; that would be ideal. I can
go
> > through the earlier issues and get to grips with details tomorrow, if not
> > possible.
> 
> sorry, "have a look". me write pretty one day.

No rush. We've been broken for a while, being broken another few days isn't
going to kill anyone. :)

[Nate Chapin, 2015-03-04 17:44:56]

lgtm w/nits

https://codereview.chromium.org/967423005/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/security/sandbox-inherit-to-blank-document.html
(right):

https://codereview.chromium.org/967423005/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/sandbox-inherit-to-blank-document.html:1:
<!DOCTYPE html>
I don't see any expected results for this new test?

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

https://codereview.chromium.org/967423005/diff/40001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:163: Frame* Frame::findFrameForNavigation(const
AtomicString& name, Frame& activeFrame)
This unconditionally returns the result of tree().find() now. Remove this and
call find() directly?

[Mike West, 2015-03-05 07:59:48]

Bah. So, the tests that I "fixed" were, in fact, behaving as expected. In short,
folks decided ~7 years ago (http://trac.webkit.org/changeset/29380) to treat
disallowed frame navigations as though the frame didn't exist, resulting in a
popup. The failing/timing out tests tested that behavior. I've verified that
Firefox is doing the same thing in these scenarios, even though it doesn't
really make a whole lot of sense to me. Ah well. Such is the internet.

Reverting to a simpler version of this patch, and just fixing the two tests.

[Mike West, 2015-03-05 08:00:09]

On 2015/03/05 at 07:59:48, Mike West wrote:
> Reverting to a simpler version of this patch, and just fixing the two tests.

(that failed in patchset #1).

[Mike West, 2015-03-05 09:04:11]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2015-03-05 09:04:11]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com,
japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/967423005/#ps60001 (title:
"Ugh.")

[commit-bot: I haz the power, 2015-03-05 09:05:22]

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

[sof, 2015-03-05 09:06:17]

Still lgtm - is the description sync'ed?

[Mike West, 2015-03-05 09:14:33]

On 2015/03/05 at 09:06:17, sigbjornf wrote:
> Still lgtm - is the description sync'ed?

Yes, thanks. I dropped the bit about 'findFrameForNavigation', so it should be
an accurate description of the change.

[commit-bot: I haz the power, 2015-03-05 09:18:58]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191352