Don't leak contexts in Object.observe

Don't leak contexts in Object.observe

The Object.observe API may construct internal structures as a result of API calls. These structures can persist as long as an object that was once observed persists. This patch ensures that these structures are created in the correct context so as to avoid leaking contexts

R=verwaest@chromium.org, dcarney
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=21126

[Toon Verwaest, 2014-05-02 14:17:34]

lgtm with nit

https://codereview.chromium.org/263833007/diff/60001/test/cctest/test-object-...
File test/cctest/test-object-observe.cc (right):

https://codereview.chromium.org/263833007/diff/60001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:680: CompileRun("function observer() {};"
I guess you don't need the observer here.

https://codereview.chromium.org/263833007/diff/60001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:702: CompileRun("function observer() {};"
no observer needed here either

[rafaelw, 2014-05-02 16:13:15]

Committed patchset #5 manually as r21126 (presubmit successful).

[rafaelw, 2014-05-02 16:18:12]

https://codereview.chromium.org/263833007/diff/60001/test/cctest/test-object-...
File test/cctest/test-object-observe.cc (right):

https://codereview.chromium.org/263833007/diff/60001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:680: CompileRun("function observer() {};"
On 2014/05/02 14:17:34, Toon Verwaest wrote:
> I guess you don't need the observer here.

Done.

https://codereview.chromium.org/263833007/diff/60001/test/cctest/test-object-...
test/cctest/test-object-observe.cc:702: CompileRun("function observer() {};"
On 2014/05/02 14:17:34, Toon Verwaest wrote:
> no observer needed here either

Done.

[Jakob Kummerow, 2014-05-02 19:31:37]

DBC.

https://codereview.chromium.org/263833007/diff/80001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/263833007/diff/80001/src/object-observe.js#ne...
src/object-observe.js:545: function
NativeObjectNotifierPerformChange(objectInfo, changeType, changeFn) {
Please avoid name clashes of JS builtins and runtime functions.

https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc#newcode15003
src/runtime.cc:15003: CONVERT_ARG_HANDLE_CHECKED(Object, callback, 1);
Please be as specific as you can regarding the object type here. I have a
feeling |callback| must be a JSFunction, so use
CONVERT_ARG_HANDLE_CHECKED(JSFunction, callback, 1);

https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc#newcode15037
src/runtime.cc:15037:
RUNTIME_FUNCTION(Runtime_NativeObjectNotifierPerformChange) {
I don't see this function declared in runtime.h, is that intentional?

[adamk, 2014-05-02 20:09:12]

I've uploaded a patch that addresses jkummerow's comments:
https://codereview.chromium.org/264793015/

https://codereview.chromium.org/263833007/diff/80001/src/object-observe.js
File src/object-observe.js (right):

https://codereview.chromium.org/263833007/diff/80001/src/object-observe.js#ne...
src/object-observe.js:545: function
NativeObjectNotifierPerformChange(objectInfo, changeType, changeFn) {
On 2014/05/02 19:31:37, Jakob wrote:
> Please avoid name clashes of JS builtins and runtime functions.

Done, I've renamed the runtime functions to make their purpose clear.

https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc#newcode15003
src/runtime.cc:15003: CONVERT_ARG_HANDLE_CHECKED(Object, callback, 1);
On 2014/05/02 19:31:37, Jakob wrote:
> Please be as specific as you can regarding the object type here. I have a
> feeling |callback| must be a JSFunction, so use
> CONVERT_ARG_HANDLE_CHECKED(JSFunction, callback, 1);

Done.

https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc#newcode15037
src/runtime.cc:15037:
RUNTIME_FUNCTION(Runtime_NativeObjectNotifierPerformChange) {
On 2014/05/02 19:31:37, Jakob wrote:
> I don't see this function declared in runtime.h, is that intentional?

That is not intentional, and combined with the same-naming, it appears that this
caused us to not be doing the right thing with performChange. I've updated the
performChange leak test to actually test that performChange changes contexts.

[Toon Verwaest, 2014-05-03 05:28:25]

https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc#newcode15003
src/runtime.cc:15003: CONVERT_ARG_HANDLE_CHECKED(Object, callback, 1);
I think this should become JSReceiver to handle function proxy callbacks.

On 2014/05/02 19:31:37, Jakob wrote:
> Please be as specific as you can regarding the object type here. I have a
> feeling |callback| must be a JSFunction, so use
> CONVERT_ARG_HANDLE_CHECKED(JSFunction, callback, 1);

[adamk, 2014-05-05 19:55:05]

https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc#newcode15003
src/runtime.cc:15003: CONVERT_ARG_HANDLE_CHECKED(Object, callback, 1);
On 2014/05/03 05:28:26, Toon Verwaest wrote:
> I think this should become JSReceiver to handle function proxy callbacks.

I've filed a but to track the fact that proxies don't work with Object.observe
at the moment: https://code.google.com/p/v8/issues/detail?id=3310

> On 2014/05/02 19:31:37, Jakob wrote:
> > Please be as specific as you can regarding the object type here. I have a
> > feeling |callback| must be a JSFunction, so use
> > CONVERT_ARG_HANDLE_CHECKED(JSFunction, callback, 1);
>

[rossberg, 2014-05-06 10:03:49]

https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc#newcode15003
src/runtime.cc:15003: CONVERT_ARG_HANDLE_CHECKED(Object, callback, 1);
On 2014/05/05 19:55:05, adamk wrote:
> On 2014/05/03 05:28:26, Toon Verwaest wrote:
> > I think this should become JSReceiver to handle function proxy callbacks.
> 
> I've filed a but to track the fact that proxies don't work with Object.observe
> at the moment: https://code.google.com/p/v8/issues/detail?id=3310
> 
> > On 2014/05/02 19:31:37, Jakob wrote:
> > > Please be as specific as you can regarding the object type here. I have a
> > > feeling |callback| must be a JSFunction, so use
> > > CONVERT_ARG_HANDLE_CHECKED(JSFunction, callback, 1);
> > 
> 

Thanks! While you're at it, can you please also create bugs to track the issues
that

1. observing the global object does not currently allowed, due to various issues
(esp with navigation, e.g. notifier context)
2. observing/unobserving in the middle of a splice event does not work correctly
3. change records in cross context scenarios live in different contexts
depending on how they are created
4. the implementation of observe does not handle acceptList accurately (e.g.
observable side effects in accessing .length multiple times)

Some of these might require modifications/extensions to the spec as well.

[adamk, 2014-05-06 19:10:19]

On 2014/05/06 10:03:49, rossberg wrote:
> https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc
> File src/runtime.cc (right):
> 
>
https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc#newcode15003
> src/runtime.cc:15003: CONVERT_ARG_HANDLE_CHECKED(Object, callback, 1);
> On 2014/05/05 19:55:05, adamk wrote:
> > On 2014/05/03 05:28:26, Toon Verwaest wrote:
> > > I think this should become JSReceiver to handle function proxy callbacks.
> > 
> > I've filed a but to track the fact that proxies don't work with
Object.observe
> > at the moment: https://code.google.com/p/v8/issues/detail?id=3310
> > 
> > > On 2014/05/02 19:31:37, Jakob wrote:
> > > > Please be as specific as you can regarding the object type here. I have
a
> > > > feeling |callback| must be a JSFunction, so use
> > > > CONVERT_ARG_HANDLE_CHECKED(JSFunction, callback, 1);
> > > 
> > 
> 
> Thanks! While you're at it, can you please also create bugs to track the
issues
> that
> 
> 1. observing the global object does not currently allowed, due to various
issues
> (esp with navigation, e.g. notifier context)

Not sure where to file this. It's sort of a spec bug, except it's outside the
purview of ECMAScript. I don't think we want to allow this, so long as we're
using Mozilla's split-global approach to cross-frame access at least.

> 2. observing/unobserving in the middle of a splice event does not work
correctly

https://code.google.com/p/v8/issues/detail?id=3314

> 3. change records in cross context scenarios live in different contexts
> depending on how they are created

https://code.google.com/p/v8/issues/detail?id=3316 (this one's a little fuzzy as
I back-formed it from the thread between Toon and Rafael, feel free to add more
detail to the bug)

> 4. the implementation of observe does not handle acceptList accurately (e.g.
> observable side effects in accessing .length multiple times)

https://code.google.com/p/v8/issues/detail?id=3315

> Some of these might require modifications/extensions to the spec as well.

[rossberg, 2014-05-07 08:49:16]

On 2014/05/06 19:10:19, adamk wrote:
> On 2014/05/06 10:03:49, rossberg wrote:
> > https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc
> > File src/runtime.cc (right):
> > 
> >
>
https://codereview.chromium.org/263833007/diff/80001/src/runtime.cc#newcode15003
> > src/runtime.cc:15003: CONVERT_ARG_HANDLE_CHECKED(Object, callback, 1);
> > On 2014/05/05 19:55:05, adamk wrote:
> > > On 2014/05/03 05:28:26, Toon Verwaest wrote:
> > > > I think this should become JSReceiver to handle function proxy
callbacks.
> > > 
> > > I've filed a but to track the fact that proxies don't work with
> Object.observe
> > > at the moment: https://code.google.com/p/v8/issues/detail?id=3310
> > > 
> > > > On 2014/05/02 19:31:37, Jakob wrote:
> > > > > Please be as specific as you can regarding the object type here. I
have
> a
> > > > > feeling |callback| must be a JSFunction, so use
> > > > > CONVERT_ARG_HANDLE_CHECKED(JSFunction, callback, 1);
> > > > 
> > > 
> > 
> > Thanks! While you're at it, can you please also create bugs to track the
> issues
> > that
> > 
> > 1. observing the global object does not currently allowed, due to various
> issues
> > (esp with navigation, e.g. notifier context)
> 
> Not sure where to file this. It's sort of a spec bug, except it's outside the
> purview of ECMAScript. I don't think we want to allow this, so long as we're
> using Mozilla's split-global approach to cross-frame access at least.
> 
> > 2. observing/unobserving in the middle of a splice event does not work
> correctly
> 
> https://code.google.com/p/v8/issues/detail?id=3314
> 
> > 3. change records in cross context scenarios live in different contexts
> > depending on how they are created
> 
> https://code.google.com/p/v8/issues/detail?id=3316 (this one's a little fuzzy
as
> I back-formed it from the thread between Toon and Rafael, feel free to add
more
> detail to the bug)
> 
> > 4. the implementation of observe does not handle acceptList accurately (e.g.
> > observable side effects in accessing .length multiple times)
> 
> https://code.google.com/p/v8/issues/detail?id=3315
> 
> > Some of these might require modifications/extensions to the spec as well.

Thanks, sounds good.