bindings: Makes the window object be the inner global object.

bindings: Makes the window object be the inner global object.

Removes the hidden prototypes from the window and
worker/worklet objects.  Makes these global objects be
the inner global object.

BUG=587393
Committed: https://crrev.com/7aff51476ff48588879709275a3c363c6114e085
Cr-Commit-Position: refs/heads/master@{#388170}

[Yuki, 2016-04-15 15:16:36]

Description was changed from

==========
bindings: Makes the window object be the inner global object.

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

to

==========
bindings: Makes the window object be the inner global object.

Removes the hidden prototypes from the window and
worker/worklet objects.  Makes these global objects be
the inner global object.

BUG=587393
==========

[Yuki, 2016-04-18 07:32:31]

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

[Yuki, 2016-04-18 07:32:31]

Could you review this CL?

[haraken, 2016-04-18 08:04:14]

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

[haraken, 2016-04-18 08:04:15]

Mostly looks good.

+jochen FYI.

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp (right):

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:70:
isolate()->SetUseCounterCallback(&useCounterCallback);

Instead of introducing UseCounterDisabledScope, can we just delay calling
isolate()->SetUseCounterCallback() until the context initialization is done?

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:328: //   -- has
prototype --> global object (window wrapper object) [2]

I'd rename:

- "global proxy object" => "outer global object"
- "global object" => "inner global object"

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:363:
V8DOMWrapper::setNativeInfo(windowPrototype, wrapperTypeInfo, window);

Not directly related to this CL, do you know why we need to call setNativeInfo
for the prototype object? We're not calling it for worker's prototype object.

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:367:
V8DOMWrapper::setNativeInfo(windowProperties, wrapperTypeInfo, window);

Ditto.

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:163:
v8::Local<v8::ObjectTemplate> globalTemplate =
wrapperTypeInfo->domTemplate(m_isolate, *m_world)->InstanceTemplate();

Add:

  if (globalTemplate.IsEmpty())
    return false;

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:184:
// The global proxy object.  Note this is not the global object.

Is "global proxy object" a term used in the spec or somewhere?

Since we (the V8 team & the binding team) has been using "outer global object"
and "inner global object", I'd prefer using the terms.

[Yuki, 2016-04-18 08:27:07]

Will address other comments.

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp (right):

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:70:
isolate()->SetUseCounterCallback(&useCounterCallback);
On 2016/04/18 08:04:15, haraken wrote:
> 
> Instead of introducing UseCounterDisabledScope, can we just delay calling
> isolate()->SetUseCounterCallback() until the context initialization is done?

You know, there can be multiple contexts in a single isolate.  When the second
context gets created, V8PerIsolateData is already fully initialized and already
is use.

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:363:
V8DOMWrapper::setNativeInfo(windowPrototype, wrapperTypeInfo, window);
On 2016/04/18 08:04:15, haraken wrote:
> 
> Not directly related to this CL, do you know why we need to call setNativeInfo
> for the prototype object? We're not calling it for worker's prototype object.

Not sure.  I vaguely guess that we can remove it.

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:184:
// The global proxy object.  Note this is not the global object.
On 2016/04/18 08:04:15, haraken wrote:
> 
> Is "global proxy object" a term used in the spec or somewhere?
> 
> Since we (the V8 team & the binding team) has been using "outer global object"
> and "inner global object", I'd prefer using the terms.

I think that V8 team is using the terms of "global proxy object" and "global
object".
--------
  /**
   * Returns the global proxy object.
   *
   * Global proxy object is a thin wrapper whose prototype points to actual
   * context's global object with the properties like Object, etc. This is done
   * that way for security reasons (for more details see
   * https://wiki.mozilla.org/Gecko:SplitWindow).
   *
   * Please note that changes to global proxy object prototype most probably
   * would break VM---v8 expects only global object as a prototype of global
   * proxy object.
   */
  Local<Object> Global();
--------

The corresponding wording in the spec is "WindowProxy" and "Window", I think. 
Otherwise, the spec seems not having the same idea with "outer global" and
"inner global".

Because I often get confused by different wording in V8 and Blink (especially
Blink's one is often wrong or strange), I wanted to use the same wording as V8
as much as possible.

[Yuki, 2016-04-18 09:17:43]

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:163:
v8::Local<v8::ObjectTemplate> globalTemplate =
wrapperTypeInfo->domTemplate(m_isolate, *m_world)->InstanceTemplate();
On 2016/04/18 08:04:15, haraken wrote:
> 
> Add:
> 
>   if (globalTemplate.IsEmpty())
>     return false;

Done.

[haraken, 2016-04-18 09:26:40]

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1858613002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:184:
// The global proxy object.  Note this is not the global object.
On 2016/04/18 08:27:07, Yuki wrote:
> On 2016/04/18 08:04:15, haraken wrote:
> > 
> > Is "global proxy object" a term used in the spec or somewhere?
> > 
> > Since we (the V8 team & the binding team) has been using "outer global
object"
> > and "inner global object", I'd prefer using the terms.
> 
> I think that V8 team is using the terms of "global proxy object" and "global
> object".
> --------
>   /**
>    * Returns the global proxy object.
>    *
>    * Global proxy object is a thin wrapper whose prototype points to actual
>    * context's global object with the properties like Object, etc. This is
done
>    * that way for security reasons (for more details see
>    * https://wiki.mozilla.org/Gecko:SplitWindow).
>    *
>    * Please note that changes to global proxy object prototype most probably
>    * would break VM---v8 expects only global object as a prototype of global
>    * proxy object.
>    */
>   Local<Object> Global();
> --------
> 
> The corresponding wording in the spec is "WindowProxy" and "Window", I think. 
> Otherwise, the spec seems not having the same idea with "outer global" and
> "inner global".
> 
> Because I often get confused by different wording in V8 and Blink (especially
> Blink's one is often wrong or strange), I wanted to use the same wording as V8
> as much as possible.

Thanks for the clarification! Let's use the same terms as the V8 team.

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

https://codereview.chromium.org/1858613002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:99: if
(V8PerIsolateData::from(isolate)->m_useCounterDisabled)

Why does the useCounterCallback crash if it's called during context
initialization? If it's just because isolate->EnteredContext returns an empty
context, we can just check it and immediately return here.

[jochen (gone - plz use gerrit), 2016-04-18 09:31:55]

jochen@chromium.org changed reviewers:
+ verwaest@chromium.org

[jochen (gone - plz use gerrit), 2016-04-18 09:31:56]

+verwaest fyi

[Yuki, 2016-04-18 09:39:30]

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

https://codereview.chromium.org/1858613002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:99: if
(V8PerIsolateData::from(isolate)->m_useCounterDisabled)
On 2016/04/18 09:26:40, haraken wrote:
> 
> Why does the useCounterCallback crash if it's called during context
> initialization? If it's just because isolate->EnteredContext returns an empty
> context, we can just check it and immediately return here.

On line 174, currentExecutionContext(isolate) has the problem.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I'm not sure if isolate->GetCurrentContext() returns an empty handle or not. 
Even if so, is it reliable that the current context is empty while we create a
new context?

If the current context is NOT empty (I need to check it),
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
toExecutionContext() is doing something wrong.  The window object is NOT YET
associated with the native DOMWindow object.

If necessary, I'll dig into deeper.

[haraken, 2016-04-18 09:48:04]

LGTM to move things forward, but let's add a TODO to remove the
UseCounterDisabledScope.

On 2016/04/18 09:39:30, Yuki wrote:
>
https://codereview.chromium.org/1858613002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp (right):
> 
>
https://codereview.chromium.org/1858613002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.cpp:99: if
> (V8PerIsolateData::from(isolate)->m_useCounterDisabled)
> On 2016/04/18 09:26:40, haraken wrote:
> > 
> > Why does the useCounterCallback crash if it's called during context
> > initialization? If it's just because isolate->EnteredContext returns an
empty
> > context, we can just check it and immediately return here.
> 
> On line 174, currentExecutionContext(isolate) has the problem.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> I'm not sure if isolate->GetCurrentContext() returns an empty handle or not. 
> Even if so, is it reliable that the current context is empty while we create a
> new context?
> 
> If the current context is NOT empty (I need to check it),
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> toExecutionContext() is doing something wrong.  The window object is NOT YET
> associated with the native DOMWindow object.
> 
> If necessary, I'll dig into deeper.

It is a responsibility of V8 bindings to enter/leave v8::Contexts. Before you
call ScriptState::Scope in WindowProxy::initialize(), the context you're
creating should not be set on the Isolate. After you call the
ScriptState::Scope, the context you've created is set on the Isolate.

So I don't think it should happen that Isolate::CurrentContext/EnteredContext
returns a half-baked context.

[Yuki, 2016-04-19 09:05:09]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2016-04-19 09:05:10]

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/1858613002/#ps160001
(title: "Synced.")

[commit-bot: I haz the power, 2016-04-19 09:05:16]

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

[commit-bot: I haz the power, 2016-04-19 09:18:06]

Description was changed from

==========
bindings: Makes the window object be the inner global object.

Removes the hidden prototypes from the window and
worker/worklet objects.  Makes these global objects be
the inner global object.

BUG=587393
==========

to

==========
bindings: Makes the window object be the inner global object.

Removes the hidden prototypes from the window and
worker/worklet objects.  Makes these global objects be
the inner global object.

BUG=587393
==========

[commit-bot: I haz the power, 2016-04-19 09:18:07]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-19 09:19:45]

Description was changed from

==========
bindings: Makes the window object be the inner global object.

Removes the hidden prototypes from the window and
worker/worklet objects.  Makes these global objects be
the inner global object.

BUG=587393
==========

to

==========
bindings: Makes the window object be the inner global object.

Removes the hidden prototypes from the window and
worker/worklet objects.  Makes these global objects be
the inner global object.

BUG=587393

Committed: https://crrev.com/7aff51476ff48588879709275a3c363c6114e085
Cr-Commit-Position: refs/heads/master@{#388170}
==========

[commit-bot: I haz the power, 2016-04-19 09:19:46]

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

[jochen (gone - plz use gerrit), 2017-01-04 14:24:27]

https://codereview.chromium.org/1858613002/diff/160001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp
(right):

https://codereview.chromium.org/1858613002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WorkerOrWorkletScriptController.cpp:191:
globalObject = V8DOMWrapper::associateObjectWithWrapper(m_isolate,
scriptWrappable, wrapperTypeInfo, globalObject);
this exposes the global object (instead of the global proxy) to script, e.g.,
when you look at event.target for push events.. while this isn't a security
issue on workers (as there's only one context anyways), this shouldn't happen