binding: Moves the check for the first access to the initial document into BindingSecurity.

binding: Moves the check for the first access to the initial document into BindingSecurity.

Checks the access to the initial document and reports it not only at
securityCheck() in V8Window.cpp but also at every call to
BindingSecurity::shouldAllowAccessTo() because V8 only calls back
securityCheck() on property lookups, and not for function invocation.

BindingSecurity::shouldAllowAccessTo() is called with every possible
cross-origin window, which means every possible new window.  Thus,
shouldAllowAccessTo() should be the right place to check the access
to the initial document.

BUG=630662
TBR=benwells@chromium.org

Committed: https://crrev.com/b8dcfeb065bbfd777cdc5f5433da9a87f25e6ec6
Cr-Commit-Position: refs/heads/master@{#412195}

[Yuki, 2016-08-04 14:33:02]

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

[commit-bot: I haz the power, 2016-08-04 14:34:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 15:33:57]

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

[commit-bot: I haz the power, 2016-08-04 15:33:57]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Yuki, 2016-08-05 07:07:40]

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

[commit-bot: I haz the power, 2016-08-05 07:07:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 07:54:20]

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

[commit-bot: I haz the power, 2016-08-05 07:54:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yuki, 2016-08-05 10:33:44]

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

[commit-bot: I haz the power, 2016-08-05 10:33:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 11:36:44]

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

[commit-bot: I haz the power, 2016-08-05 11:36:45]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Yuki, 2016-08-08 09:29:21]

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

[commit-bot: I haz the power, 2016-08-08 09:29:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 10:39:36]

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

[commit-bot: I haz the power, 2016-08-08 10:39:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Yuki, 2016-08-12 10:41:01]

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

[commit-bot: I haz the power, 2016-08-12 10:41:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 12:02:05]

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

[commit-bot: I haz the power, 2016-08-12 12:02:06]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Yuki, 2016-08-12 12:37:29]

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

[commit-bot: I haz the power, 2016-08-12 12:37:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 13:58:59]

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

[commit-bot: I haz the power, 2016-08-12 13:59:01]

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

[Yuki, 2016-08-12 14:14:04]

yukishiino@chromium.org changed reviewers:
+ benwells@chromium.org, haraken@chromium.org, meacer@chromium.org,
torne@chromium.org

[Yuki, 2016-08-12 14:14:05]

Could you guys review this CL?

haraken@ as the main reviewer

This CL makes Blink call didAccessInitialDocument() more often.  It means that
Blink more often calls back to Chromium (content/).  Then, it revealed that some
of tests didn't wait for the right event.

torne@ for android_webview
Most of popup-related tests check the title of a popup window, so they should
wait for the title change before comparing the title.

benwells@ for chrome/browser/apps/
The test should wait for each tab loading before retrieving its URL.

meacer@ for chrome/browser/ssl/
The test should wait for an interstitial page's attachment before accessing to
its URL.

[Torne, 2016-08-12 15:41:39]

android_webview test updates LGTM

[meacer, 2016-08-12 17:48:41]

chrome/browser/ssl/ LGTM

[haraken, 2016-08-13 02:29:36]

LGTM

https://codereview.chromium.org/2209303002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/2209303002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:49:
SECURITY_DCHECK(!(targetWindow && targetWindow->frame())

I think it's worth changing this to SECURITY_CHECK (in a follow-up CL).

https://codereview.chromium.org/2209303002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:56: if
(!(accessingWindow && targetWindow && targetWindow->isLocalDOMWindow()))

!accessingWindow || !targetWindow || ! targetWindow->isLocalDOMWindow()

https://codereview.chromium.org/2209303002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (left):

https://codereview.chromium.org/2209303002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:102:
targetFrame->loader().didAccessInitialDocument();

You're saying that this CL will call didAccessInitialDocument more often, but
that's not necessarily the case here. Before this CL, didAccessInitialDocument
was called for an access where shouldAllowAccessTo returns false. After this CL,
didAccessInitialDocument is called only when shouldAllowAccessTo returns true.

The change looks good to me but just want to confirm that that is your
intention.

[Yuki, 2016-08-15 03:39:52]

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

[commit-bot: I haz the power, 2016-08-15 03:40:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2016-08-15 03:42:58]

https://codereview.chromium.org/2209303002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/2209303002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:49:
SECURITY_DCHECK(!(targetWindow && targetWindow->frame())
On 2016/08/13 02:29:36, haraken wrote:
> 
> I think it's worth changing this to SECURITY_CHECK (in a follow-up CL).

Done.

https://codereview.chromium.org/2209303002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:56: if
(!(accessingWindow && targetWindow && targetWindow->isLocalDOMWindow()))
On 2016/08/13 02:29:36, haraken wrote:
> 
> !accessingWindow || !targetWindow || ! targetWindow->isLocalDOMWindow()

I'd prefer this (old) style to yours.  My execuses are:
1)
  if (!( complex okay condition ))
    early exit (return, throw, exit, etc.);
is a common pattern (as a substitute of unless(...)).
2)
  if (x && x->foo())
is also a common short-circuit pattern.
  if (!x || !x->foo())
is a bit less easy to read (for me).

https://codereview.chromium.org/2209303002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (left):

https://codereview.chromium.org/2209303002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:102:
targetFrame->loader().didAccessInitialDocument();
On 2016/08/13 02:29:36, haraken wrote:
> 
> You're saying that this CL will call didAccessInitialDocument more often, but
> that's not necessarily the case here. Before this CL, didAccessInitialDocument
> was called for an access where shouldAllowAccessTo returns false. After this
CL,
> didAccessInitialDocument is called only when shouldAllowAccessTo returns true.
> 
> The change looks good to me but just want to confirm that that is your
> intention.

You're right.  Not necessarily, but it's intentional.  If a window does not have
access to another window, there must be no chance to modify the initial document
of another window.  So, there is no need to call didAccessInitialDocument.

Having said that, it's also true that there are more call sites of
BindingSecurity::shouldAllowAccessTo() than the call sites of securityCheck().

[haraken, 2016-08-15 04:32:12]

LGTM

[commit-bot: I haz the power, 2016-08-15 04:56:02]

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

[commit-bot: I haz the power, 2016-08-15 04:56:02]

Dry run: This issue passed the CQ dry run.

[Yuki, 2016-08-16 05:36:09]

Description was changed from

==========
binding: Moves the check for the first access to the initial document into
BindingSecurity.

Checks the access to the initial document and reports it not only at
securityCheck() in V8Window.cpp but also at every call to
BindingSecurity::shouldAllowAccessTo() because V8 only calls back
securityCheck() on property lookups, and not for function invocation.

BindingSecurity::shouldAllowAccessTo() is called with every possible
cross-origin window, which means every possible new window.  Thus,
shouldAllowAccessTo() should be the right place to check the access
to the initial document.

BUG=630662
==========

to

==========
binding: Moves the check for the first access to the initial document into
BindingSecurity.

Checks the access to the initial document and reports it not only at
securityCheck() in V8Window.cpp but also at every call to
BindingSecurity::shouldAllowAccessTo() because V8 only calls back
securityCheck() on property lookups, and not for function invocation.

BindingSecurity::shouldAllowAccessTo() is called with every possible
cross-origin window, which means every possible new window.  Thus,
shouldAllowAccessTo() should be the right place to check the access
to the initial document.

BUG=630662
TBR=benwells@chromium.org
==========

[Yuki, 2016-08-16 05:36:20]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2016-08-16 05:36:21]

The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org,
meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2209303002/#ps140001
(title: "Addressed review comments.")

[commit-bot: I haz the power, 2016-08-16 05:36:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 08:55:28]

Description was changed from

==========
binding: Moves the check for the first access to the initial document into
BindingSecurity.

Checks the access to the initial document and reports it not only at
securityCheck() in V8Window.cpp but also at every call to
BindingSecurity::shouldAllowAccessTo() because V8 only calls back
securityCheck() on property lookups, and not for function invocation.

BindingSecurity::shouldAllowAccessTo() is called with every possible
cross-origin window, which means every possible new window.  Thus,
shouldAllowAccessTo() should be the right place to check the access
to the initial document.

BUG=630662
TBR=benwells@chromium.org
==========

to

==========
binding: Moves the check for the first access to the initial document into
BindingSecurity.

Checks the access to the initial document and reports it not only at
securityCheck() in V8Window.cpp but also at every call to
BindingSecurity::shouldAllowAccessTo() because V8 only calls back
securityCheck() on property lookups, and not for function invocation.

BindingSecurity::shouldAllowAccessTo() is called with every possible
cross-origin window, which means every possible new window.  Thus,
shouldAllowAccessTo() should be the right place to check the access
to the initial document.

BUG=630662
TBR=benwells@chromium.org
==========

[commit-bot: I haz the power, 2016-08-16 08:55:29]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-16 08:57:37]

Description was changed from

==========
binding: Moves the check for the first access to the initial document into
BindingSecurity.

Checks the access to the initial document and reports it not only at
securityCheck() in V8Window.cpp but also at every call to
BindingSecurity::shouldAllowAccessTo() because V8 only calls back
securityCheck() on property lookups, and not for function invocation.

BindingSecurity::shouldAllowAccessTo() is called with every possible
cross-origin window, which means every possible new window.  Thus,
shouldAllowAccessTo() should be the right place to check the access
to the initial document.

BUG=630662
TBR=benwells@chromium.org
==========

to

==========
binding: Moves the check for the first access to the initial document into
BindingSecurity.

Checks the access to the initial document and reports it not only at
securityCheck() in V8Window.cpp but also at every call to
BindingSecurity::shouldAllowAccessTo() because V8 only calls back
securityCheck() on property lookups, and not for function invocation.

BindingSecurity::shouldAllowAccessTo() is called with every possible
cross-origin window, which means every possible new window.  Thus,
shouldAllowAccessTo() should be the right place to check the access
to the initial document.

BUG=630662
TBR=benwells@chromium.org

Committed: https://crrev.com/b8dcfeb065bbfd777cdc5f5433da9a87f25e6ec6
Cr-Commit-Position: refs/heads/master@{#412195}
==========

[commit-bot: I haz the power, 2016-08-16 08:57:40]

Patchset 8 (id:??) landed as
https://crrev.com/b8dcfeb065bbfd777cdc5f5433da9a87f25e6ec6
Cr-Commit-Position: refs/heads/master@{#412195}

[benwells, 2016-08-16 23:20:27]

lgtm