Blink bindings: use v8 to enforce method call access checks

Blink bindings: use v8 to enforce method call access checks

Current, method calls on cross-origin interfaces perform manual access
checks in the generated bindings. However, v8 also has the capability
to perform the access check itself if the function template specifies
SetAcceptsAnyReceiver(false).

Using SetAcceptsAnyReceiver(false) has several advantages:
- Removes the need for the EventTarget performance hack, since the fast
  path for same context access is simply a pointer comparison.
- Removes the need for the complicated binding template logic to
  handle DOMWindow/LocalDOMWindow.
- Enforces access checks more consistently throughout the bindings
- Reduces the amount of generated bindings code
- Makes the access check failure more consistent for a detached window:
  when a frame is removed from the DOM, v8 allows the access, while the
  custom Blink method call access check denies it. This was a behavior
  difference between accessors and methods, and now behaves the same
  for both. Note that there are still some differences, which will be
  resolved by a followup patch to base access checks off DOMWindow
  instead of Frame.

The main disadvantage is that the thrown security error is less precise:
however, this is already a problem for every other cross-origin access.
The right solution is to resolve the longstanding TODO to plumb through
object/property information from v8 in the failed access check callback.

BUG=none

Review-Url: https://codereview.chromium.org/2713413002
Cr-Commit-Position: refs/heads/master@{#453397}
Committed: https://chromium.googlesource.com/chromium/src/+/4bf51e553baa31ca6b1ab7e45f8dffc9ca114641

[dcheng, 2017-02-26 10:14:10]

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

[commit-bot: I haz the power, 2017-02-26 10:14:17]

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

[dcheng, 2017-02-26 11:40:56]

Description was changed from

==========
Experiment with SetAcceptAnyReceiver(false) for forcing access checks.
==========

to

==========
Enforce access checks on method calls in v8

Current, methods on cross-origin interfaces perform manual access checks
in the generated bindings. However, v8 can also enforce the access check
if the  function template specifies SetAcceptsAnyReceiver(false).

Using SetAcceptsAnyReceiver(false) has several advantages:
- Removes the need for the EventTarget performance hack, since the fast
  path for same context access is simply a pointer comparison.
- Removes the need for the complicated binding template logic to
  handle DOMWindow/LocalDOMWindow.
- Enforces access checks more consistently throughout the bindings

The main disadvantage is that the thrown security error is less precise:
however, this is already a problem for every other cross-origin access.
The right solution is to resolve the longstanding TODO to plumb through
object/property information from v8 in the report access failed callback.

TODO(dcheng): Propagate check_security_for_receiver to partial interfaces
TODO(dcheng): Add a test that verifies pointer comparison for security token
              works.
==========

[dcheng, 2017-02-26 11:40:56]

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

[dcheng, 2017-02-26 11:41:51]

Description was changed from

==========
Enforce access checks on method calls in v8

Current, methods on cross-origin interfaces perform manual access checks
in the generated bindings. However, v8 can also enforce the access check
if the  function template specifies SetAcceptsAnyReceiver(false).

Using SetAcceptsAnyReceiver(false) has several advantages:
- Removes the need for the EventTarget performance hack, since the fast
  path for same context access is simply a pointer comparison.
- Removes the need for the complicated binding template logic to
  handle DOMWindow/LocalDOMWindow.
- Enforces access checks more consistently throughout the bindings

The main disadvantage is that the thrown security error is less precise:
however, this is already a problem for every other cross-origin access.
The right solution is to resolve the longstanding TODO to plumb through
object/property information from v8 in the report access failed callback.

TODO(dcheng): Propagate check_security_for_receiver to partial interfaces
TODO(dcheng): Add a test that verifies pointer comparison for security token
              works.
==========

to

==========
Enforce access checks on method calls in v8
Current, methods on cross-origin interfaces perform manual access checks
in the generated bindings. However, v8 can also enforce the access check
if the  function template specifies SetAcceptsAnyReceiver(false).

Using SetAcceptsAnyReceiver(false) has several advantages:
- Removes the need for the EventTarget performance hack, since the fast
  path for same context access is simply a pointer comparison.
- Removes the need for the complicated binding template logic to
  handle DOMWindow/LocalDOMWindow.
- Enforces access checks more consistently throughout the bindings

The main disadvantage is that the thrown security error is less precise:
however, this is already a problem for every other cross-origin access.
The right solution is to resolve the longstanding TODO to plumb through
object/property information from v8 in the failed access check callback.

TODO(dcheng): Propagate check_security_for_receiver to partial interfaces
TODO(dcheng): Add a test that verifies pointer comparison for security token
              works.

BUG=none
==========

[dcheng, 2017-02-26 11:43:36]

Description was changed from

==========
Enforce access checks on method calls in v8
Current, methods on cross-origin interfaces perform manual access checks
in the generated bindings. However, v8 can also enforce the access check
if the  function template specifies SetAcceptsAnyReceiver(false).

Using SetAcceptsAnyReceiver(false) has several advantages:
- Removes the need for the EventTarget performance hack, since the fast
  path for same context access is simply a pointer comparison.
- Removes the need for the complicated binding template logic to
  handle DOMWindow/LocalDOMWindow.
- Enforces access checks more consistently throughout the bindings

The main disadvantage is that the thrown security error is less precise:
however, this is already a problem for every other cross-origin access.
The right solution is to resolve the longstanding TODO to plumb through
object/property information from v8 in the failed access check callback.

TODO(dcheng): Propagate check_security_for_receiver to partial interfaces
TODO(dcheng): Add a test that verifies pointer comparison for security token
              works.

BUG=none
==========

to

==========
Blink bindings: use v8 to enforce method call access checks

Current, method calls on cross-origin interfaces perform manual access
checks in the generated bindings. However, v8 also has the capability
to perform the access check itself if the function template specifies
SetAcceptsAnyReceiver(false).

Using SetAcceptsAnyReceiver(false) has several advantages:
- Removes the need for the EventTarget performance hack, since the fast
  path for same context access is simply a pointer comparison.
- Removes the need for the complicated binding template logic to
  handle DOMWindow/LocalDOMWindow.
- Enforces access checks more consistently throughout the bindings
- Reduces the amount of generated bindings code

The main disadvantage is that the thrown security error is less precise:
however, this is already a problem for every other cross-origin access.
The right solution is to resolve the longstanding TODO to plumb through
object/property information from v8 in the failed access check callback.

TODO(dcheng): Propagate check_security_for_receiver to partial interfaces
TODO(dcheng): Add a test that verifies pointer comparison for security token
              works.

BUG=none
==========

[commit-bot: I haz the power, 2017-02-26 11:53:20]

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

[commit-bot: I haz the power, 2017-02-26 11:53:20]

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

[dcheng, 2017-02-27 03:29:12]

Description was changed from

==========
Blink bindings: use v8 to enforce method call access checks

Current, method calls on cross-origin interfaces perform manual access
checks in the generated bindings. However, v8 also has the capability
to perform the access check itself if the function template specifies
SetAcceptsAnyReceiver(false).

Using SetAcceptsAnyReceiver(false) has several advantages:
- Removes the need for the EventTarget performance hack, since the fast
  path for same context access is simply a pointer comparison.
- Removes the need for the complicated binding template logic to
  handle DOMWindow/LocalDOMWindow.
- Enforces access checks more consistently throughout the bindings
- Reduces the amount of generated bindings code

The main disadvantage is that the thrown security error is less precise:
however, this is already a problem for every other cross-origin access.
The right solution is to resolve the longstanding TODO to plumb through
object/property information from v8 in the failed access check callback.

TODO(dcheng): Propagate check_security_for_receiver to partial interfaces
TODO(dcheng): Add a test that verifies pointer comparison for security token
              works.

BUG=none
==========

to

==========
Blink bindings: use v8 to enforce method call access checks

Current, method calls on cross-origin interfaces perform manual access
checks in the generated bindings. However, v8 also has the capability
to perform the access check itself if the function template specifies
SetAcceptsAnyReceiver(false).

Using SetAcceptsAnyReceiver(false) has several advantages:
- Removes the need for the EventTarget performance hack, since the fast
  path for same context access is simply a pointer comparison.
- Removes the need for the complicated binding template logic to
  handle DOMWindow/LocalDOMWindow.
- Enforces access checks more consistently throughout the bindings
- Reduces the amount of generated bindings code
- Makes the access check failure more consistent for a detached window:
  when a frame is removed from the DOM, v8 allows the access, while the
  custom Blink method call access check denies it. This was a behavior
  difference between accessors and methods, and now behaves the same
  for both.

The main disadvantage is that the thrown security error is less precise:
however, this is already a problem for every other cross-origin access.
The right solution is to resolve the longstanding TODO to plumb through
object/property information from v8 in the failed access check callback.

TODO(dcheng): Propagate check_security_for_receiver to partial interfaces
TODO(dcheng): Add a test that verifies pointer comparison for security token
              works.

BUG=none
==========

[dcheng, 2017-02-27 05:24:03]

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

[commit-bot: I haz the power, 2017-02-27 05:24:12]

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

[dcheng, 2017-02-27 05:33:22]

Description was changed from

==========
Blink bindings: use v8 to enforce method call access checks

Current, method calls on cross-origin interfaces perform manual access
checks in the generated bindings. However, v8 also has the capability
to perform the access check itself if the function template specifies
SetAcceptsAnyReceiver(false).

Using SetAcceptsAnyReceiver(false) has several advantages:
- Removes the need for the EventTarget performance hack, since the fast
  path for same context access is simply a pointer comparison.
- Removes the need for the complicated binding template logic to
  handle DOMWindow/LocalDOMWindow.
- Enforces access checks more consistently throughout the bindings
- Reduces the amount of generated bindings code
- Makes the access check failure more consistent for a detached window:
  when a frame is removed from the DOM, v8 allows the access, while the
  custom Blink method call access check denies it. This was a behavior
  difference between accessors and methods, and now behaves the same
  for both.

The main disadvantage is that the thrown security error is less precise:
however, this is already a problem for every other cross-origin access.
The right solution is to resolve the longstanding TODO to plumb through
object/property information from v8 in the failed access check callback.

TODO(dcheng): Propagate check_security_for_receiver to partial interfaces
TODO(dcheng): Add a test that verifies pointer comparison for security token
              works.

BUG=none
==========

to

==========
Blink bindings: use v8 to enforce method call access checks

Current, method calls on cross-origin interfaces perform manual access
checks in the generated bindings. However, v8 also has the capability
to perform the access check itself if the function template specifies
SetAcceptsAnyReceiver(false).

Using SetAcceptsAnyReceiver(false) has several advantages:
- Removes the need for the EventTarget performance hack, since the fast
  path for same context access is simply a pointer comparison.
- Removes the need for the complicated binding template logic to
  handle DOMWindow/LocalDOMWindow.
- Enforces access checks more consistently throughout the bindings
- Reduces the amount of generated bindings code
- Makes the access check failure more consistent for a detached window:
  when a frame is removed from the DOM, v8 allows the access, while the
  custom Blink method call access check denies it. This was a behavior
  difference between accessors and methods, and now behaves the same
  for both. Note that there are still some differences, which will be
  resolved by a followup patch to base access checks off DOMWindow
  instead of Frame.

The main disadvantage is that the thrown security error is less precise:
however, this is already a problem for every other cross-origin access.
The right solution is to resolve the longstanding TODO to plumb through
object/property information from v8 in the failed access check callback.

BUG=none
==========

[dcheng, 2017-02-27 05:34:19]

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/fetch/chromium/discarded-window.html
(right):

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/chromium/discarded-window.html:23:
assert_equals(e.message, "Failed to execute 'fetch' on 'Window': The global
scope is shutting down.");
Per yhirano, it's OK to return a promise, as long as it's a rejected promise.

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/security/cross-frame-access-call-expected.txt
(left):

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/security/cross-frame-access-call-expected.txt:1:
CONSOLE WARNING: line 1: 'getMatchedCSSRules()' is deprecated. For more help,
check https://code.google.com/p/chromium/issues/detail?id=437569#c2
This console message disappears because v8 denies the access before it ever
calls into the Blink bindings.

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:370: //
TODO(dcheng): Does this need an access check?
I don't think it does, since I think this implies a static method?

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h (right):

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h:59: enum
HolderCheckConfiguration : unsigned {
Just to match how we're storing the enums.

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (left):

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:51: // Performance
hack for EventTarget.  Checking whether it's a Window or not
I think this shouldn't be needed anymore, as the common fast path for security
token checks is quick (it's just a pointer comparison). But if there are some
steps of how to confirm this, that would be helpful.

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right):

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:483: {% if
world_suffix in method.activity_logging_world_list %}
Restoring the activity logger back to the original location.

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/DOMWindowTimers.cpp (right):

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/DOMWindowTimers.cpp:53: return false;
This is now required, due to a subtle difference between v8's access checks and
Blink's access checks. Otherwise, it crashes in jochen's new CHECK in
ScheduledAction.

In v8, it's OK to access the global of a detached frame: v8 skips the access
check completely due to how the access check is implemented: in
HeapObject::IsAccessCheckNeeded(), there is a special case for JSGlobalProxy. If
the current context's global object is attached as |this|'s global object, the
access check is skipped (e.g. the access is allowed). When a frame is removed
from the DOM, we call WindowProxyManager::clearForClose(), which does *not*
detach the global object from the global proxy. Thus, same-origin access to a
Window object for a detached frame is OK.

However, Blink's access check currently denies access if the DOMWindow has no
frame. Thus, we have a situation where v8 thinks the access is OK, but Blink
thinks it is not OK. So for now, we manually bail out.

I'm working on a separate patch to make access checks in Blink based on Window
=)

[haraken, 2017-02-27 05:49:25]

LGTM.

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:370: //
TODO(dcheng): Does this need an access check?
On 2017/02/27 05:34:19, dcheng wrote:
> I don't think it does, since I think this implies a static method?

Agreed. We won't need the access check. I'm okay with adding the check just in
case though.

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/DOMWindowTimers.cpp (right):

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/DOMWindowTimers.cpp:53: return false;
On 2017/02/27 05:34:19, dcheng wrote:
> This is now required, due to a subtle difference between v8's access checks
and
> Blink's access checks. Otherwise, it crashes in jochen's new CHECK in
> ScheduledAction.
> 
> In v8, it's OK to access the global of a detached frame: v8 skips the access
> check completely due to how the access check is implemented: in
> HeapObject::IsAccessCheckNeeded(), there is a special case for JSGlobalProxy.
If
> the current context's global object is attached as |this|'s global object, the
> access check is skipped (e.g. the access is allowed). When a frame is removed
> from the DOM, we call WindowProxyManager::clearForClose(), which does *not*
> detach the global object from the global proxy. Thus, same-origin access to a
> Window object for a detached frame is OK.
> 
> However, Blink's access check currently denies access if the DOMWindow has no
> frame. Thus, we have a situation where v8 thinks the access is OK, but Blink
> thinks it is not OK. So for now, we manually bail out.
> 
> I'm working on a separate patch to make access checks in Blink based on Window
> =)
> 

Sounds reasonable to me.

[commit-bot: I haz the power, 2017-02-27 07:13:29]

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

[commit-bot: I haz the power, 2017-02-27 07:13:30]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-27 07:52:22]

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

[dcheng, 2017-02-27 07:52:26]

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:370: //
TODO(dcheng): Does this need an access check?
On 2017/02/27 05:49:25, haraken wrote:
> On 2017/02/27 05:34:19, dcheng wrote:
> > I don't think it does, since I think this implies a static method?
> 
> Agreed. We won't need the access check. I'm okay with adding the check just in
> case though.
> 

Hmm... it's not possible to do an access check in that case though, since there
is no |impl|. I will just remove the TODO and write a comment why an access
check is not necessary in this case.

[commit-bot: I haz the power, 2017-02-27 07:52:38]

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

[haraken, 2017-02-27 07:53:57]

On 2017/02/27 07:52:26, dcheng wrote:
>
https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp
(right):
> 
>
https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:370: //
> TODO(dcheng): Does this need an access check?
> On 2017/02/27 05:49:25, haraken wrote:
> > On 2017/02/27 05:34:19, dcheng wrote:
> > > I don't think it does, since I think this implies a static method?
> > 
> > Agreed. We won't need the access check. I'm okay with adding the check just
in
> > case though.
> > 
> 
> Hmm... it's not possible to do an access check in that case though, since
there
> is no |impl|. I will just remove the TODO and write a comment why an access
> check is not necessary in this case.

Sounds good.

[commit-bot: I haz the power, 2017-02-27 09:17:10]

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

[commit-bot: I haz the power, 2017-02-27 09:17:10]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-27 22:07:45]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-02-27 22:07:45]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2713413002/#ps120001
(title: "Restore comment")

[commit-bot: I haz the power, 2017-02-27 22:09:05]

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, 2017-02-27 23:52:27]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488233265127050,
"parent_rev": "fd938bac4105603ff6a997d5de3cabe39d8869c0", "commit_rev":
"4bf51e553baa31ca6b1ab7e45f8dffc9ca114641"}

[commit-bot: I haz the power, 2017-02-27 23:53:08]

Description was changed from

==========
Blink bindings: use v8 to enforce method call access checks

Current, method calls on cross-origin interfaces perform manual access
checks in the generated bindings. However, v8 also has the capability
to perform the access check itself if the function template specifies
SetAcceptsAnyReceiver(false).

Using SetAcceptsAnyReceiver(false) has several advantages:
- Removes the need for the EventTarget performance hack, since the fast
  path for same context access is simply a pointer comparison.
- Removes the need for the complicated binding template logic to
  handle DOMWindow/LocalDOMWindow.
- Enforces access checks more consistently throughout the bindings
- Reduces the amount of generated bindings code
- Makes the access check failure more consistent for a detached window:
  when a frame is removed from the DOM, v8 allows the access, while the
  custom Blink method call access check denies it. This was a behavior
  difference between accessors and methods, and now behaves the same
  for both. Note that there are still some differences, which will be
  resolved by a followup patch to base access checks off DOMWindow
  instead of Frame.

The main disadvantage is that the thrown security error is less precise:
however, this is already a problem for every other cross-origin access.
The right solution is to resolve the longstanding TODO to plumb through
object/property information from v8 in the failed access check callback.

BUG=none
==========

to

==========
Blink bindings: use v8 to enforce method call access checks

Current, method calls on cross-origin interfaces perform manual access
checks in the generated bindings. However, v8 also has the capability
to perform the access check itself if the function template specifies
SetAcceptsAnyReceiver(false).

Using SetAcceptsAnyReceiver(false) has several advantages:
- Removes the need for the EventTarget performance hack, since the fast
  path for same context access is simply a pointer comparison.
- Removes the need for the complicated binding template logic to
  handle DOMWindow/LocalDOMWindow.
- Enforces access checks more consistently throughout the bindings
- Reduces the amount of generated bindings code
- Makes the access check failure more consistent for a detached window:
  when a frame is removed from the DOM, v8 allows the access, while the
  custom Blink method call access check denies it. This was a behavior
  difference between accessors and methods, and now behaves the same
  for both. Note that there are still some differences, which will be
  resolved by a followup patch to base access checks off DOMWindow
  instead of Frame.

The main disadvantage is that the thrown security error is less precise:
however, this is already a problem for every other cross-origin access.
The right solution is to resolve the longstanding TODO to plumb through
object/property information from v8 in the failed access check callback.

BUG=none

Review-Url: https://codereview.chromium.org/2713413002
Cr-Commit-Position: refs/heads/master@{#453397}
Committed:
https://chromium.googlesource.com/chromium/src/+/4bf51e553baa31ca6b1ab7e45f8d...
==========

[commit-bot: I haz the power, 2017-02-27 23:53:14]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/4bf51e553baa31ca6b1ab7e45f8d...

[Yuki, 2017-03-01 08:36:17]

LGTM.

https://codereview.chromium.org/2713413002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/fetch/chromium/discarded-window.html
(right):

https://codereview.chromium.org/2713413002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/fetch/chromium/discarded-window.html:19:
assert_true(false, "This Promise must always be rejected.");
nit: assert_unreached
No need to fix.  Just FYI for future use cases.

[Yuki, 2017-03-01 08:40:11]

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2713413002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:370: //
TODO(dcheng): Does this need an access check?
On 2017/02/27 07:52:26, dcheng wrote:
> On 2017/02/27 05:49:25, haraken wrote:
> > On 2017/02/27 05:34:19, dcheng wrote:
> > > I don't think it does, since I think this implies a static method?
> > 
> > Agreed. We won't need the access check. I'm okay with adding the check just
in
> > case though.
> > 
> 
> Hmm... it's not possible to do an access check in that case though, since
there
> is no |impl|. I will just remove the TODO and write a comment why an access
> check is not necessary in this case.

Just FYI, the spec doesn't require any access check here because the property is
a data property.  There is no way to perform a security check when just getting
a data property.