Store Object.observe state per-isolate rather than per-context

Store Object.observe state per-isolate rather than per-context

This requires adding a new JSObject to the strong root list and populating it from
object-observe.js. The main other change is that we now directly use ObjectHashTable
from JS rather than using WeakMap, since using the latter would end up leaking whichever
Context initialized that observation state.

Added a test via the API showing that different contexts all end up working on the same state.

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

[adamk, 2012-10-24 15:17:41]

First look at this, no tests yet

[rafaelw, 2012-10-24 15:21:35]

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

https://codereview.chromium.org/11274014/diff/1/src/object-observe.js#newcode54
src/object-observe.js:54: 
suggestion:

var state = %GetObjectObservationState();
if (IS_UNDEFINED(state.observerInfoMap)) {
 state.observerInfoMap = createInternalWeakMap();
 state.objectInfoMap = createInternalWeakMap();
}

var observerInfoMap = state.observerInfoMap;
var objectInfoMap = state.objectInfoMap;

[adamk, 2012-10-24 15:50:38]

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

https://codereview.chromium.org/11274014/diff/1/src/object-observe.js#newcode54
src/object-observe.js:54: 
On 2012/10/24 15:21:35, rafaelw wrote:
> suggestion:
> 
> var state = %GetObjectObservationState();
> if (IS_UNDEFINED(state.observerInfoMap)) {
>  state.observerInfoMap = createInternalWeakMap();
>  state.objectInfoMap = createInternalWeakMap();
> }
> 
> var observerInfoMap = state.observerInfoMap;
> var objectInfoMap = state.objectInfoMap;

Done.

[adamk, 2012-11-05 11:31:35]

+mstarzinger to comment on usage of per-isolate state. He suggested we need to
restructure this to create the WeakMaps in a way that doesn't tie them to a
context, but I'm not clear what the best way to do that will be.

[Michael Starzinger, 2012-11-05 13:08:10]

Drive-by-comments: As discussed offline with Andreas, I think we will need a
separate "InternalWeakMap" that builds on the same backing functionality as
WeakMaps but isn't bound to a specific context. Unfortunately I am not yet sure
about the specifics of that.

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

https://codereview.chromium.org/11274014/diff/14001/src/object-observe.js#new...
src/object-observe.js:48: objectObservationState.observerInfoMap =
createInternalWeakMap();
This will make the WeakMap instantiated in this context globally reachable from
the whole isolate. Thereby the first context will be kept alive indefinitely.

[adamk, 2012-11-05 14:14:48]

Minimal approach: use ObjectHashTable directly from object-observe.js via a few
helper runtime functions. Pretty ugly, in that we're storing non-JS objects in
properties of a real JS object, but seems to work.

[rossberg, 2012-11-06 12:27:09]

Mostly looks good to me, but I'll let Michael stamp it.

https://codereview.chromium.org/11274014/diff/18001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/11274014/diff/18001/src/objects-inl.h#newcode730
src/objects-inl.h:730: // TODO: Is this sufficient?
Yeah, I don't think it's worth introducing the extra (sub)type in the heap_types
list when you cannot check for it anyways.

[adamk, 2012-11-06 12:59:13]

https://codereview.chromium.org/11274014/diff/18001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/11274014/diff/18001/src/objects-inl.h#newcode730
src/objects-inl.h:730: // TODO: Is this sufficient?
On 2012/11/06 12:27:09, rossberg wrote:
> Yeah, I don't think it's worth introducing the extra (sub)type in the
heap_types
> list when you cannot check for it anyways.

Okay, I've removed this and completely redone the functions in runtime.cc to
minimize the checks they do (also changed the safe ones to not use handles). Let
me know if I went overboard there.

[Michael Starzinger, 2012-11-06 14:17:44]

This is already looking good. Just a few comments.

https://codereview.chromium.org/11274014/diff/20003/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11274014/diff/20003/src/runtime.cc#newcode13234
src/runtime.cc:13234: return ObjectHashTable::Allocate(0);
As discussed offline, having this non-JS object escape into JavaScript (even
though we fully control the code) seems fishy. The clean solution would be to
wrap it into a JSValue, which is a full-fledged JSObject with one internal
field. That would add an additional indirection though.

But that would require quite some additional boilerplate and I am fine with
landing this as it is. Just wanted to leave this comment so that we can remember
there is a battle-plan if it ever hits us. I could very well imagine our fuzzer
generating tests that trigger this.

https://codereview.chromium.org/11274014/diff/20003/src/runtime.cc#newcode13241
src/runtime.cc:13241: ObjectHashTable* table = ObjectHashTable::cast(args[0]);
Use the CONVERT_ARG_CHECKED macro here which will throw a proper exception upon
type mismatch. Otherwise our fuzzer might spot crashes when it fuzzes this
runtime function.

https://codereview.chromium.org/11274014/diff/20003/src/runtime.cc#newcode13251
src/runtime.cc:13251: Handle<ObjectHashTable> table =
args.at<ObjectHashTable>(0);
Likewise.

https://codereview.chromium.org/11274014/diff/20003/src/runtime.cc#newcode13262
src/runtime.cc:13262: ObjectHashTable* table = ObjectHashTable::cast(args[0]);
Likewise.

[adamk, 2012-11-06 14:54:21]

https://codereview.chromium.org/11274014/diff/20003/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/11274014/diff/20003/src/heap.h#newcode159
src/heap.h:159: V(JSObject, object_observation_state, ObjectObservationState)
Note, renamed this to just "observation_state" given that the flag changed to
"harmony_observation". Willing to take "harmony_" on the front if requested.

https://codereview.chromium.org/11274014/diff/20003/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11274014/diff/20003/src/runtime.cc#newcode13234
src/runtime.cc:13234: return ObjectHashTable::Allocate(0);
On 2012/11/06 14:17:44, Michael Starzinger wrote:
> As discussed offline, having this non-JS object escape into JavaScript (even
> though we fully control the code) seems fishy. The clean solution would be to
> wrap it into a JSValue, which is a full-fledged JSObject with one internal
> field. That would add an additional indirection though.
> 
> But that would require quite some additional boilerplate and I am fine with
> landing this as it is. Just wanted to leave this comment so that we can
remember
> there is a battle-plan if it ever hits us. I could very well imagine our
fuzzer
> generating tests that trigger this.

Noted. I'd also be fine with wrapping this in a JSValue right now, since the
indirection is likely going to be in the noise compared to the hash lookups, but
either way works for me.

https://codereview.chromium.org/11274014/diff/20003/src/runtime.cc#newcode13241
src/runtime.cc:13241: ObjectHashTable* table = ObjectHashTable::cast(args[0]);
On 2012/11/06 14:17:44, Michael Starzinger wrote:
> Use the CONVERT_ARG_CHECKED macro here which will throw a proper exception
upon
> type mismatch. Otherwise our fuzzer might spot crashes when it fuzzes this
> runtime function.

Done.

https://codereview.chromium.org/11274014/diff/20003/src/runtime.cc#newcode13251
src/runtime.cc:13251: Handle<ObjectHashTable> table =
args.at<ObjectHashTable>(0);
On 2012/11/06 14:17:44, Michael Starzinger wrote:
> Likewise.

Done.

https://codereview.chromium.org/11274014/diff/20003/src/runtime.cc#newcode13262
src/runtime.cc:13262: ObjectHashTable* table = ObjectHashTable::cast(args[0]);
On 2012/11/06 14:17:44, Michael Starzinger wrote:
> Likewise.

Done.

[Michael Starzinger, 2012-11-06 15:04:10]

LGTM. I'll land this for you.

https://codereview.chromium.org/11274014/diff/20003/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/11274014/diff/20003/src/heap.h#newcode159
src/heap.h:159: V(JSObject, object_observation_state, ObjectObservationState)
On 2012/11/06 14:54:21, adamk wrote:
> Note, renamed this to just "observation_state" given that the flag changed to
> "harmony_observation". Willing to take "harmony_" on the front if requested.

That's fine. I prefer it the way it is right now as well.