Special-case LocalDOMWindow for same-origin access in bindings.

Special-case LocalDOMWindow for same-origin access in bindings.

When generating IDL bindings for the Window interface, the bindings
will now downcast to LocalDOMWindow if the access is a same-origin
accesses. This allows for simplification of DOMWindow and its
subclasses to be simpler and easier to understand:

- Cross-origin behavior is more clearly highlighted as the shared base
  class only has a few virtual methods now.
- Same-origin behavior is moved completely into LocalDOMWindow.
- RemoteDOMWindow no longer has several dozen methods that simply
  ASSERT_NOT_REACHED().
- Partial window interfaces are now correctly anchored off
  LocalDOMWindow rather than requiring all implementations to downcast

BUG=none

Review-Url: https://codereview.chromium.org/2668753006
Cr-Commit-Position: refs/heads/master@{#448039}
Committed: https://chromium.googlesource.com/chromium/src/+/8abb3475d41c011a9f573ea65200e7f16a9a4496

[dcheng, 2017-02-01 07:17:13]

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

[commit-bot: I haz the power, 2017-02-01 07:17:30]

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-01 07:22:58]

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

[dcheng, 2017-02-01 07:22:59]

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:221: Document*
document = toLocalDOMWindow(target)->document();
I am considering moving SecurityContext to DOMWindow, but it doesn't seem like
it will gain us much either. Either way, I'll investigate in a followup.

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/DOMWindow.cpp:213: ?
blink::toLocalDOMWindow(this)->document()->url()
I did this to explicitly call the non-method version, but I guess we could call
the method one here. It just seems a bit weird either way:
- If we call the member one, it's a random EventTarget override that just
happens to be in scope
- If we call the non-member one, then the callsite just looks weird.

I opted for the latter since it's more consistent with how we generally do
downcasts (using the non-member function)...

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/Performance.cpp (right):

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/Performance.cpp:243: emptyString(),
emptyString());
To avoid constructing three new empty strings.

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/Performance.h (right):

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/Performance.h:82: LocalFrame*
observerFrame);
We can change this because if you read the long task timing code, it's always
passing in Performance::frame(), which by definition is local.

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/compositorworker/WindowAnimationWorklet.cpp
(left):

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/compositorworker/WindowAnimationWorklet.cpp:44:
m_animationWorklet = AnimationWorklet::create(window.frame());
This helper got moved into the constructor: there's no point in having it
separate, because when we create it, this will always be executed anyway (the
execution context should still be live).

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/compositorworker/WindowAnimationWorklet.cpp
(right):

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/compositorworker/WindowAnimationWorklet.cpp:23:
// TODO(dcheng): Why isn't this just all on the Oilpan heap?
Something that might be good to fix... not really sure why this isn't just all
in Oilpan. As far as I can tell, everything lives inside Blink.

[dcheng, 2017-02-01 07:33:48]

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

[commit-bot: I haz the power, 2017-02-01 07:34:04]

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

[Yuki, 2017-02-01 07:54:35]

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

[Yuki, 2017-02-01 07:54:36]

Looks great in general.

Thinking about same origin vs cross origin and local frame vs remote frame, I
vaguely guess that [LocalFrameImplementedAs=...] would be more appropriate?

I think we're assuming that, if a DOM attribute or operation is not cross origin
accessible and author script actually calls back a Blink's callback function,
then it must be a local frame (because we reject the access if it's a remote
frame even if it's same origin).

Then, what about [LocalFrameImplementedAs=...] or something like that?

[dcheng, 2017-02-01 08:00:53]

On 2017/02/01 07:54:36, Yuki wrote:
> Looks great in general.
> 
> Thinking about same origin vs cross origin and local frame vs remote frame, I
> vaguely guess that [LocalFrameImplementedAs=...] would be more appropriate?
> 
> I think we're assuming that, if a DOM attribute or operation is not cross
origin
> accessible and author script actually calls back a Blink's callback function,
> then it must be a local frame (because we reject the access if it's a remote
> frame even if it's same origin).
> 
> Then, what about [LocalFrameImplementedAs=...] or something like that?

Hmmm... the reason I picked same origin is because same origin implies local
frame, but local frame does not imply same origin.

That being said, I'm not super thrilled about the name... so suggestions
definitely welcome. I just worry that local frame wouldn't clearly imply that
the access checks passed.

[Yuki, 2017-02-01 08:23:19]

On 2017/02/01 08:00:53, dcheng wrote:
> On 2017/02/01 07:54:36, Yuki wrote:
> > Looks great in general.
> > 
> > Thinking about same origin vs cross origin and local frame vs remote frame,
I
> > vaguely guess that [LocalFrameImplementedAs=...] would be more appropriate?
> > 
> > I think we're assuming that, if a DOM attribute or operation is not cross
> origin
> > accessible and author script actually calls back a Blink's callback
function,
> > then it must be a local frame (because we reject the access if it's a remote
> > frame even if it's same origin).
> > 
> > Then, what about [LocalFrameImplementedAs=...] or something like that?
> 
> Hmmm... the reason I picked same origin is because same origin implies local
> frame, but local frame does not imply same origin.

It's okay that local frame does not imply same origin, isn't it?  Because it
doesn't imply, so we need to do perform the security check, and we actually do.

> That being said, I'm not super thrilled about the name... so suggestions
> definitely welcome. I just worry that local frame wouldn't clearly imply that
> the access checks passed.

I'm fine with, for example, [LocalSameOriginImplementedAs=].

My point here is: [LocalFrameImplementedAs=LocalDOMWindow] is a declaration that
the implementation class for local frame is LocalDOMWindow.  Just it.  It says
nothing about when / in what case it's a local frame.  If it's a local frame,
then we should use LocalDOMWindow.

IDL is, by definition, to declare interfaces, not to represent an
implementations.  I'd like IDLs to be declarative.

And, "same origin implies local frame" is true in Blink, but not true (or not
applicable) in Web IDL.  The definition of "same origin" is different between
Blink and spec.  I'm afraid that it would be confusing.  (Actually, I was
confused, as you know.)

So, I'd prefer "Local" rather than "SameOrigin".

Just my two cents.

[haraken, 2017-02-01 08:32:01]

A couple of questions:

> Hmmm... the reason I picked same origin is because same origin implies local
> frame, but local frame does not imply same origin.

Just help me understand: Is it possible that a local frame is used for a
cross-origin frame?

Is [SameOriginImplementedAs] going to be used in an interface other than
DOMWindow? If no, another option will be adding a special casing to the IDL
compiler only for DOMWindow. (The IDL compiler already has a bunch of special
casing for DOMWindow.)

Do you think that it's cleaner to introduce [SameOriginImplementedAs] than
[CrossOriginImplementedAs]?

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/compositorworker/WindowAnimationWorklet.cpp
(right):

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/compositorworker/WindowAnimationWorklet.cpp:23:
// TODO(dcheng): Why isn't this just all on the Oilpan heap?
On 2017/02/01 07:22:59, dcheng wrote:
> Something that might be good to fix... not really sure why this isn't just all
> in Oilpan. As far as I can tell, everything lives inside Blink.

I'm not sure how easy it will be to move ThreadedWorkletMessagingProxy to Oilpan
since it's handling a bunch of cross-thread complicated pointers and thread
lifetime issues.

Also, even if we move ThreadedWorkletMessagingProxy to Oilpan, it won't solve
the cycle issue because we need to use CrossThreadPersistent to have a
cross-thread reference.

[commit-bot: I haz the power, 2017-02-01 10:01:41]

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

[commit-bot: I haz the power, 2017-02-01 10:01:42]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-01 18:00:13]

On 2017/02/01 08:23:19, Yuki wrote:
> On 2017/02/01 08:00:53, dcheng wrote:
> > On 2017/02/01 07:54:36, Yuki wrote:
> > > Looks great in general.
> > > 
> > > Thinking about same origin vs cross origin and local frame vs remote
frame,
> I
> > > vaguely guess that [LocalFrameImplementedAs=...] would be more
appropriate?
> > > 
> > > I think we're assuming that, if a DOM attribute or operation is not cross
> > origin
> > > accessible and author script actually calls back a Blink's callback
> function,
> > > then it must be a local frame (because we reject the access if it's a
remote
> > > frame even if it's same origin).
> > > 
> > > Then, what about [LocalFrameImplementedAs=...] or something like that?
> > 
> > Hmmm... the reason I picked same origin is because same origin implies local
> > frame, but local frame does not imply same origin.
> 
> It's okay that local frame does not imply same origin, isn't it?  Because it
> doesn't imply, so we need to do perform the security check, and we actually
do.
> 
> > That being said, I'm not super thrilled about the name... so suggestions
> > definitely welcome. I just worry that local frame wouldn't clearly imply
that
> > the access checks passed.
> 
> I'm fine with, for example, [LocalSameOriginImplementedAs=].
> 
> My point here is: [LocalFrameImplementedAs=LocalDOMWindow] is a declaration
that
> the implementation class for local frame is LocalDOMWindow.  Just it.  It says
> nothing about when / in what case it's a local frame.  If it's a local frame,
> then we should use LocalDOMWindow.
> 

Right, but the reason we can infer it's local frame is because it's same-origin.
In any case, my latest patchset removes the attribute and just hardcodes this
behavior directly in the templates.

> IDL is, by definition, to declare interfaces, not to represent an
> implementations.  I'd like IDLs to be declarative.
> 
> And, "same origin implies local frame" is true in Blink, but not true (or not
> applicable) in Web IDL.  The definition of "same origin" is different between
> Blink and spec.  I'm afraid that it would be confusing.  (Actually, I was
> confused, as you know.)
> 
> So, I'd prefer "Local" rather than "SameOrigin".
> 
> Just my two cents.

[dcheng, 2017-02-01 18:00:16]

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

[commit-bot: I haz the power, 2017-02-01 18:00:34]

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-01 18:30:28]

On 2017/02/01 08:32:01, haraken wrote:
> A couple of questions:
> 
> > Hmmm... the reason I picked same origin is because same origin implies local
> > frame, but local frame does not imply same origin.
> 
> Just help me understand: Is it possible that a local frame is used for a
> cross-origin frame?

Yes, that's correct. Once lightweight v8 contexts lands, we might be able to
experiment with forcing all cross-origin frames to RemoteFrames (happy to talk
more about how this would work offline).

> 
> Is [SameOriginImplementedAs] going to be used in an interface other than
> DOMWindow? If no, another option will be adding a special casing to the IDL
> compiler only for DOMWindow. (The IDL compiler already has a bunch of special
> casing for DOMWindow.)
> 
> Do you think that it's cleaner to introduce [SameOriginImplementedAs] than
> [CrossOriginImplementedAs]?

I tried that as well, but it's harder to maintain at this time. The problem is
things like UIEvent::initUIEvent. The generated v8 bindings code looks like this
if we use CrossOriginImplementedAs:

static void initUIEventMethod(const v8::FunctionCallbackInfo<v8::Value>& info) {
  ExceptionState exceptionState(info.GetIsolate(),
ExceptionState::ExecutionContext, "UIEvent", "initUIEvent");

  UIEvent* impl = V8UIEvent::toImpl(info.Holder());

  V8StringResource<> type;
  bool bubbles;
  bool cancelable;
  LocalDOMWindow* view;
  int detail;
  type = info[0];
  if (!type.prepare())
    return;

  bubbles = toBoolean(info.GetIsolate(), info[1], exceptionState);
  if (exceptionState.hadException())
    return;

  cancelable = toBoolean(info.GetIsolate(), info[2], exceptionState);
  if (exceptionState.hadException())
    return;

  view = toDOMWindow(info.GetIsolate(), info[3]);
  if (!view && !isUndefinedOrNull(info[3])) {
    exceptionState.throwTypeError("parameter 4 is not of type 'Window'.");

    return;
  }

  detail = toInt32(info.GetIsolate(), info[4], NormalConversion,
exceptionState);
  if (exceptionState.hadException())
    return;

  impl->initUIEvent(type, bubbles, cancelable, view, detail);
}

The problem is the |view| argument doesn't have to be a same-origin window, so
downcasting to LocalDOMWindow* here is unsafe. It's difficult to make this case
work correctly in bindings without making a much more intrusive change.

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/compositorworker/WindowAnimationWorklet.cpp
(right):

https://codereview.chromium.org/2668753006/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/compositorworker/WindowAnimationWorklet.cpp:23:
// TODO(dcheng): Why isn't this just all on the Oilpan heap?
On 2017/02/01 08:32:01, haraken wrote:
> On 2017/02/01 07:22:59, dcheng wrote:
> > Something that might be good to fix... not really sure why this isn't just
all
> > in Oilpan. As far as I can tell, everything lives inside Blink.
> 
> I'm not sure how easy it will be to move ThreadedWorkletMessagingProxy to
Oilpan
> since it's handling a bunch of cross-thread complicated pointers and thread
> lifetime issues.
> 
> Also, even if we move ThreadedWorkletMessagingProxy to Oilpan, it won't solve
> the cycle issue because we need to use CrossThreadPersistent to have a
> cross-thread reference.

OK, removed the TODO. It feels like it's harder than it has to be, but maybe
this doesn't affect a lot of things.

[commit-bot: I haz the power, 2017-02-01 20:00:24]

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

[commit-bot: I haz the power, 2017-02-01 20:00:25]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-02-02 05:53:55]

Could you update the subject and the description of the CL?

Then, LGTM.

[dcheng, 2017-02-02 05:56:18]

Description was changed from

==========
Add [SameOriginImplementedAs=...] IDL extended attribute.

When specified, bindings will downcast to the specified type on
same-origin accesses. This allows for simplification of DOMWindow and
its subclasses to be simpler and easier to understand:

- Cross-origin behavior is more clearly highlighted as the shared base
  class only has a few virtual methods now.
- Same-origin behavior is moved completely into LocalDOMWindow.
- RemoteDOMWindow no longer has several dozen methods that simply
  ASSERT_NOT_REACHED().
- Partial window interfaces are now correctly anchored off
  LocalDOMWindow rather than requiring all implementations to downcast

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

to

==========
Special-case LocalDOMWindow for same-origin access in bindings.

When generating IDL bindings for the Window interface, the bindings
will now downcast to LocalDOMWindow if the access is a same-origin
accesses. This allows for simplification of DOMWindow and its
subclasses to be simpler and easier to understand:

- Cross-origin behavior is more clearly highlighted as the shared base
  class only has a few virtual methods now.
- Same-origin behavior is moved completely into LocalDOMWindow.
- RemoteDOMWindow no longer has several dozen methods that simply
  ASSERT_NOT_REACHED().
- Partial window interfaces are now correctly anchored off
  LocalDOMWindow rather than requiring all implementations to downcast

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

[dcheng, 2017-02-02 05:56:34]

On 2017/02/02 05:53:55, Yuki wrote:
> Could you update the subject and the description of the CL?
> 
> Then, LGTM.

Done.

[haraken, 2017-02-03 09:03:18]

Sorry about the review delay. LGTM.

https://codereview.chromium.org/2668753006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2668753006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:47:
LocalDOMWindow* impl = toLocalDOMWindow({{v8_class}}::toImpl(holder));

Add a comment and explain what it's doing.

https://codereview.chromium.org/2668753006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/2668753006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1384: External*
LocalDOMWindow::external() const {

You'll need to rebase this part with ToT.

[dcheng, 2017-02-03 10:04:25]

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

[dcheng, 2017-02-03 10:04:26]

https://codereview.chromium.org/2668753006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2668753006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:47:
LocalDOMWindow* impl = toLocalDOMWindow({{v8_class}}::toImpl(holder));
On 2017/02/03 09:03:18, haraken wrote:
> 
> Add a comment and explain what it's doing.

Done.

https://codereview.chromium.org/2668753006/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/2668753006/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1384: External*
LocalDOMWindow::external() const {
On 2017/02/03 09:03:18, haraken wrote:
> 
> You'll need to rebase this part with ToT.

Done.

[dcheng, 2017-02-03 17:12:40]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-02-03 17:12:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2668753006/#ps120001
(title: "Rebase and address comments.")

[commit-bot: I haz the power, 2017-02-03 17:13:00]

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-03 19:21:20]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486141960556820,
"parent_rev": "d8d7e0da939d5a60057549c99b2bcee3b9eb28db", "commit_rev":
"8abb3475d41c011a9f573ea65200e7f16a9a4496"}

[commit-bot: I haz the power, 2017-02-03 19:21:54]

Description was changed from

==========
Special-case LocalDOMWindow for same-origin access in bindings.

When generating IDL bindings for the Window interface, the bindings
will now downcast to LocalDOMWindow if the access is a same-origin
accesses. This allows for simplification of DOMWindow and its
subclasses to be simpler and easier to understand:

- Cross-origin behavior is more clearly highlighted as the shared base
  class only has a few virtual methods now.
- Same-origin behavior is moved completely into LocalDOMWindow.
- RemoteDOMWindow no longer has several dozen methods that simply
  ASSERT_NOT_REACHED().
- Partial window interfaces are now correctly anchored off
  LocalDOMWindow rather than requiring all implementations to downcast

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

to

==========
Special-case LocalDOMWindow for same-origin access in bindings.

When generating IDL bindings for the Window interface, the bindings
will now downcast to LocalDOMWindow if the access is a same-origin
accesses. This allows for simplification of DOMWindow and its
subclasses to be simpler and easier to understand:

- Cross-origin behavior is more clearly highlighted as the shared base
  class only has a few virtual methods now.
- Same-origin behavior is moved completely into LocalDOMWindow.
- RemoteDOMWindow no longer has several dozen methods that simply
  ASSERT_NOT_REACHED().
- Partial window interfaces are now correctly anchored off
  LocalDOMWindow rather than requiring all implementations to downcast

BUG=none

Review-Url: https://codereview.chromium.org/2668753006
Cr-Commit-Position: refs/heads/master@{#448039}
Committed:
https://chromium.googlesource.com/chromium/src/+/8abb3475d41c011a9f573ea65200...
==========

[commit-bot: I haz the power, 2017-02-03 19:21:55]

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