[runtime] Intercept function declarations.

[runtime] Intercept function declarations.

We used to intercept function definitions, but not declarations.
GenericNamedPropertySetterCallback now also intercepts function declarations.

For definitions, we call DeclareGlobal and then InitializeVarGlobal. For
declarations, we never call InitializeVarGlobal, thus we must check for
interceptors in DeclareGlobal.

If the semantics of a redeclaration are wrong, e.g., redeclaring a read-only
property, an exception is thrown independent of whether an interceptor is
installed. Usually, i.e., not during a declaration, we only throw if
the call is not successfully intercepted.

BUG=v8:5375
Committed: https://crrev.com/8439401d2dfdb9fa328c758cca689289922a32d1
Cr-Commit-Position: refs/heads/master@{#39450}

[Franzi, 2016-09-12 14:20:45]

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

[commit-bot: I haz the power, 2016-09-12 14:20:52]

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

[Franzi, 2016-09-12 14:31:17]

Description was changed from

==========
[runtime] Intercept function declarations.

BUG=v8:5375
==========

to

==========
[runtime] Intercept function declarations.

GenericNamedPropertySetterCallback now also intercepts function declarations.
For definitions, we call DeclareGlobal and then InitializeVarGlobal. For
declarations, we never call InitializeVarGlobal, thus we must check for
interceptors in DeclareGlobal.

BUG=v8:5375
==========

[commit-bot: I haz the power, 2016-09-12 14:35:06]

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

[commit-bot: I haz the power, 2016-09-12 14:35:07]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/12541)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[Franzi, 2016-09-12 14:36:38]

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

[commit-bot: I haz the power, 2016-09-12 14:36:46]

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-09-12 14:50:57]

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

[commit-bot: I haz the power, 2016-09-12 14:50:58]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Franzi, 2016-09-12 15:45:23]

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

[commit-bot: I haz the power, 2016-09-12 15:45:28]

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-09-12 15:56:58]

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

[commit-bot: I haz the power, 2016-09-12 15:56:59]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Franzi, 2016-09-13 11:42:44]

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

[commit-bot: I haz the power, 2016-09-13 11:42:48]

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-09-13 11:56:18]

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

[commit-bot: I haz the power, 2016-09-13 11:56:19]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[Franzi, 2016-09-13 12:18:17]

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

[commit-bot: I haz the power, 2016-09-13 12:18:25]

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

[Franzi, 2016-09-13 12:33:49]

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

[commit-bot: I haz the power, 2016-09-13 12:33:57]

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

[Franzi, 2016-09-13 12:47:18]

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

[Franzi, 2016-09-13 12:47:19]

Hi Jochen, 

Please take a look. 

Thanks, 

Franzi

[commit-bot: I haz the power, 2016-09-13 13:04:56]

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

[commit-bot: I haz the power, 2016-09-13 13:04:57]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-09-13 22:22:16]

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

[adamk, 2016-09-13 22:22:17]

I'm a little worried about the possible performance effects of this. Does this
mean that when loading a script like:

<script>
function foo() {}
function bar() {}
...
</script>

each of those function declarations will trigger an interceptor call?

[Franzi, 2016-09-14 02:50:35]

On 2016/09/13 at 22:22:17, adamk wrote:
> I'm a little worried about the possible performance effects of this. Does this
mean that when loading a script like:
> 
> <script>
> function foo() {}
> function bar() {}
> ...
> </script>
> 
> each of those function declarations will trigger an interceptor call?

Just like 

<script>
foo = function() {}
bar = function() {}
...
</script>

already does. Are declarations faster than definitions (because of the missing
interceptor)? Is that something we rely on?

[jochen (gone - plz use gerrit), 2016-09-14 08:46:27]

it's only slow if we had an interceptor on the global object, no?

[jochen (gone - plz use gerrit), 2016-09-14 08:48:32]

what happens if you redeclare a function?

[Franzi, 2016-09-14 11:18:59]

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

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

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

[Franzi, 2016-09-14 11:40:34]

On 2016/09/14 at 08:48:32, jochen wrote:
> what happens if you redeclare a function?

I first check if it's OK to redeclare and then intercept the request. I should
change the order to be consistent with the other interceptors.

[commit-bot: I haz the power, 2016-09-14 11:50:32]

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

[commit-bot: I haz the power, 2016-09-14 11:50:33]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-09-14 14:07:11]

On 2016/09/14 at 11:40:34, franzih wrote:
> On 2016/09/14 at 08:48:32, jochen wrote:
> > what happens if you redeclare a function?
> 
> I first check if it's OK to redeclare and then intercept the request. I should
change the order to be consistent with the other interceptors.

I guess bailing out if it's not ok is fine. Assuming blink doesn't have such an
interceptor and therefore won't get an performance impact this lgtm

[Franzi, 2016-09-14 14:34:22]

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

[commit-bot: I haz the power, 2016-09-14 14:34:25]

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

[Franzi, 2016-09-14 14:39:32]

On 2016/09/14 at 14:07:11, jochen wrote:
> On 2016/09/14 at 11:40:34, franzih wrote:
> > On 2016/09/14 at 08:48:32, jochen wrote:
> > > what happens if you redeclare a function?
> > 
> > I first check if it's OK to redeclare and then intercept the request. I
should change the order to be consistent with the other interceptors.
> 
> I guess bailing out if it's not ok is fine. Assuming blink doesn't have such
an interceptor and therefore won't get an performance impact this lgtm

Yes, I agree it's OK to throw if the semantics of a redeclaration are wrong
rather than calling the interceptor first. So invalid redeclare throws rather
than calling the interceptor first. I added tests for that.

I checked that Blink doesn't have an interceptor on the global object, i.e., we
never make function declarations with a LookupIterator::Interceptor.

[Franzi, 2016-09-14 14:49:22]

Description was changed from

==========
[runtime] Intercept function declarations.

GenericNamedPropertySetterCallback now also intercepts function declarations.
For definitions, we call DeclareGlobal and then InitializeVarGlobal. For
declarations, we never call InitializeVarGlobal, thus we must check for
interceptors in DeclareGlobal.

BUG=v8:5375
==========

to

==========
[runtime] Intercept function declarations.

GenericNamedPropertySetterCallback now also intercepts function declarations.

For definitions, we call DeclareGlobal and then InitializeVarGlobal. For
declarations, we never call InitializeVarGlobal, thus we must check for
interceptors in DeclareGlobal.

If the semantics of a redeclaration are wrong, e.g., redeclaring a read-only
property, an exception is thrown independent of whether an interceptor is 
installed. In usual behavior, i.e., not during a declaration, is different,
namely to throw only if the call is not successfully intercepted. 

BUG=v8:5375
==========

[Franzi, 2016-09-14 14:51:05]

Description was changed from

==========
[runtime] Intercept function declarations.

GenericNamedPropertySetterCallback now also intercepts function declarations.

For definitions, we call DeclareGlobal and then InitializeVarGlobal. For
declarations, we never call InitializeVarGlobal, thus we must check for
interceptors in DeclareGlobal.

If the semantics of a redeclaration are wrong, e.g., redeclaring a read-only
property, an exception is thrown independent of whether an interceptor is 
installed. In usual behavior, i.e., not during a declaration, is different,
namely to throw only if the call is not successfully intercepted. 

BUG=v8:5375
==========

to

==========
[runtime] Intercept function declarations.

GenericNamedPropertySetterCallback now also intercepts function declarations.

For definitions, we call DeclareGlobal and then InitializeVarGlobal. For
declarations, we never call InitializeVarGlobal, thus we must check for
interceptors in DeclareGlobal.

If the semantics of a redeclaration are wrong, e.g., redeclaring a read-only 
property, an exception is thrown independent of whether an interceptor is 
installed. The usual behavior, i.e., not during a declaration, is 
different, namely to throw only if the call is not successfully intercepted. 

BUG=v8:5375
==========

[Franzi, 2016-09-14 14:53:06]

Description was changed from

==========
[runtime] Intercept function declarations.

GenericNamedPropertySetterCallback now also intercepts function declarations.

For definitions, we call DeclareGlobal and then InitializeVarGlobal. For
declarations, we never call InitializeVarGlobal, thus we must check for
interceptors in DeclareGlobal.

If the semantics of a redeclaration are wrong, e.g., redeclaring a read-only 
property, an exception is thrown independent of whether an interceptor is 
installed. The usual behavior, i.e., not during a declaration, is 
different, namely to throw only if the call is not successfully intercepted. 

BUG=v8:5375
==========

to

==========
[runtime] Intercept function declarations.

We used to intercept function definitions, but not declarations.
GenericNamedPropertySetterCallback now also intercepts function declarations.

For definitions, we call DeclareGlobal and then InitializeVarGlobal. For
declarations, we never call InitializeVarGlobal, thus we must check for
interceptors in DeclareGlobal.

If the semantics of a redeclaration are wrong, e.g., redeclaring a read-only 
property, an exception is thrown independent of whether an interceptor is 
installed. Usually, i.e., not during a declaration, we only throw if
the call is not successfully intercepted. 

BUG=v8:5375
==========

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

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

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

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/12742)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)

[Franzi, 2016-09-14 16:40:58]

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

[commit-bot: I haz the power, 2016-09-14 16:41:04]

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

[Franzi, 2016-09-14 16:41:46]

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

[commit-bot: I haz the power, 2016-09-14 16:41:51]

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

[Franzi, 2016-09-14 17:18:14]

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

[commit-bot: I haz the power, 2016-09-14 17:18:24]

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

[adamk, 2016-09-14 17:19:57]

On 2016/09/14 02:50:35, Franzi wrote:
> On 2016/09/13 at 22:22:17, adamk wrote:
> > I'm a little worried about the possible performance effects of this. Does
this
> mean that when loading a script like:
> > 
> > <script>
> > function foo() {}
> > function bar() {}
> > ...
> > </script>
> > 
> > each of those function declarations will trigger an interceptor call?
> 
> Just like 
> 
> <script>
> foo = function() {}
> bar = function() {}
> ...
> </script>
> 
> already does. Are declarations faster than definitions (because of the missing
> interceptor)? Is that something we rely on?

The function declaration pattern is very common (and certainly less common than
"foo = function() {}"). I would be surprised if we weren't implicitly relying on
that being fast, especially on pages with lots of function declarations.

To Jochen's point, don't we always have an interceptor on the global object when
running in Blink?

[Franzi, 2016-09-14 17:20:00]

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

[commit-bot: I haz the power, 2016-09-14 17:20:06]

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

[Franzi, 2016-09-14 18:00:08]

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

[commit-bot: I haz the power, 2016-09-14 18:00:13]

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

[Franzi, 2016-09-14 18:25:52]

> To Jochen's point, don't we always have an interceptor on the global object
when running in Blink?

I tried it in Chrome: crash, if we have a function declaration and if
iterator.state() is interceptor. Didn't crash. Tried it in tests where I
intercept the global object: crashed as expected. I'm pretty confident with this
test. I think we have interceptors on e.g., window, so any request on window is
intercepted and passed on to the global object, like here
https://codesearch.chromium.org/chromium/src/out/Debug/gen/blink/bindings/cor....
But not the other way round. Jochen, can you shed some light on this?

[commit-bot: I haz the power, 2016-09-14 18:29:34]

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

[commit-bot: I haz the power, 2016-09-14 18:29:35]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-09-15 10:16:13]

On 2016/09/14 at 18:25:52, franzih wrote:
> > To Jochen's point, don't we always have an interceptor on the global object
when running in Blink?
> 
> I tried it in Chrome: crash, if we have a function declaration and if
iterator.state() is interceptor. Didn't crash. Tried it in tests where I
intercept the global object: crashed as expected. I'm pretty confident with this
test. I think we have interceptors on e.g., window, so any request on window is
intercepted and passed on to the global object, like here
https://codesearch.chromium.org/chromium/src/out/Debug/gen/blink/bindings/cor....
But not the other way round. Jochen, can you shed some light on this?

this is how the global object in blink looks like:
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/bindin...

if you have an <img id=foo> then window.foo will return that element, this is
implemented by this interceptor. however, the interceptor is not on the global
object itself (and it doesn't have a setter anyways), so we should be good

[Franzi, 2016-09-15 14:46:16]

On 2016/09/15 at 10:16:13, jochen wrote:
> On 2016/09/14 at 18:25:52, franzih wrote:
> > > To Jochen's point, don't we always have an interceptor on the global
object when running in Blink?
> > 
> > I tried it in Chrome: crash, if we have a function declaration and if
iterator.state() is interceptor. Didn't crash. Tried it in tests where I
intercept the global object: crashed as expected. I'm pretty confident with this
test. I think we have interceptors on e.g., window, so any request on window is
intercepted and passed on to the global object, like here
https://codesearch.chromium.org/chromium/src/out/Debug/gen/blink/bindings/cor....
But not the other way round. Jochen, can you shed some light on this?
> 
> this is how the global object in blink looks like:
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/bindin...
> 
> if you have an <img id=foo> then window.foo will return that element, this is
implemented by this interceptor. however, the interceptor is not on the global
object itself (and it doesn't have a setter anyways), so we should be good

Thanks!

[Franzi, 2016-09-15 15:03:54]

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

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

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-09-15 15:28:17]

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

[commit-bot: I haz the power, 2016-09-15 15:28:18]

Dry run: This issue passed the CQ dry run.

[Franzi, 2016-09-15 15:35:58]

The CQ bit was checked by franzih@chromium.org

[Franzi, 2016-09-15 15:35:58]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2334733002/#ps250001
(title: "Fix test.")

[commit-bot: I haz the power, 2016-09-15 15:36:04]

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-09-15 15:47:54]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2016-09-15 15:48:46]

Description was changed from

==========
[runtime] Intercept function declarations.

We used to intercept function definitions, but not declarations.
GenericNamedPropertySetterCallback now also intercepts function declarations.

For definitions, we call DeclareGlobal and then InitializeVarGlobal. For
declarations, we never call InitializeVarGlobal, thus we must check for
interceptors in DeclareGlobal.

If the semantics of a redeclaration are wrong, e.g., redeclaring a read-only 
property, an exception is thrown independent of whether an interceptor is 
installed. Usually, i.e., not during a declaration, we only throw if
the call is not successfully intercepted. 

BUG=v8:5375
==========

to

==========
[runtime] Intercept function declarations.

We used to intercept function definitions, but not declarations.
GenericNamedPropertySetterCallback now also intercepts function declarations.

For definitions, we call DeclareGlobal and then InitializeVarGlobal. For
declarations, we never call InitializeVarGlobal, thus we must check for
interceptors in DeclareGlobal.

If the semantics of a redeclaration are wrong, e.g., redeclaring a read-only
property, an exception is thrown independent of whether an interceptor is
installed. Usually, i.e., not during a declaration, we only throw if
the call is not successfully intercepted.

BUG=v8:5375

Committed: https://crrev.com/8439401d2dfdb9fa328c758cca689289922a32d1
Cr-Commit-Position: refs/heads/master@{#39450}
==========

[commit-bot: I haz the power, 2016-09-15 15:48:48]

Patchset 14 (id:??) landed as
https://crrev.com/8439401d2dfdb9fa328c758cca689289922a32d1
Cr-Commit-Position: refs/heads/master@{#39450}