Add an API to let the embedder associate arbitrary data with a v8::Context. (issue 11087020)

[aba...@chromium.org]

Reviewers: danno,

Description:
Add an API to let the embedder associate arbitrary data with a v8::Context.

I had hoped that v8::Context::SetData would let the embedder associate a
void*
with a v8::Context, but it turns out we can't use that API because the
debugger
makes assumptions about the sorts of values that will be stored via that
API.

See https://bugs.webkit.org/show_bug.cgi?id=98679 for more context.

Please review this at https://codereview.chromium.org/11087020/

SVN Base: http://v8.googlecode.com/svn/trunk/

Affected files:
M include/v8.h
M src/api.cc
M src/contexts.h


Index: include/v8.h
===================================================================
--- include/v8.h (revision 12669)
+++ include/v8.h (working copy)
@@ -3722,6 +3722,13 @@
Local<Value> GetData();

/**
+ * Associate an opaque pointer with the context. This pointer can be
used by
+ * the embedder to associate additional state with the context.
+ */
+ void SetEmbedderData(Handle<Value> data);
+ Local<Value> GetEmbedderData();
+
+ /**
* Control whether code generation from strings is allowed. Calling
* this method with false will disable 'eval' and the 'Function'
* constructor for code running in this context. If 'eval' or the
Index: src/api.cc
===================================================================
--- src/api.cc (revision 12669)
+++ src/api.cc (working copy)
@@ -792,6 +792,27 @@
}


+void Context::SetEmbedderData(v8::Handle<Value> data) {
+ i::Handle<i::Context> env = Utils::OpenHandle(this);
+ i::Isolate* isolate = env->GetIsolate();
+ if (IsDeadCheck(isolate, "v8::Context::SetEmbedderData()")) return;
+ i::Handle<i::Object> raw_data = Utils::OpenHandle(*data);
+ ASSERT(env->IsNativeContext());
+ env->set_embedder_data(*raw_data);
+}
+
+
+v8::Local<Value> Context::GetEmbedderData() {
+ i::Handle<i::Context> env = Utils::OpenHandle(this);
+ i::Isolate* isolate = env->GetIsolate();
+ if (IsDeadCheck(isolate, "v8::Context::GetEmbedderData()")) {
+ return Local<Value>();
+ }
+ i::Handle<i::Object> result(env->embedder_data(), isolate);
+ return Utils::ToLocal(result);
+}
+
+
i::Object** v8::HandleScope::RawClose(i::Object** value) {
if (!ApiCheck(!is_closed_,
"v8::HandleScope::Close()",
Index: src/contexts.h
===================================================================
--- src/contexts.h (revision 12669)
+++ src/contexts.h (working copy)
@@ -153,6 +153,7 @@
V(OUT_OF_MEMORY_INDEX, Object, out_of_memory) \
V(MAP_CACHE_INDEX, Object, map_cache) \
V(CONTEXT_DATA_INDEX, Object, data) \
+ V(CONTEXT_EMBEDDER_DATA_INDEX, Object, embedder_data) \
V(ALLOW_CODE_GEN_FROM_STRINGS_INDEX, Object,
allow_code_gen_from_strings) \
V(ERROR_MESSAGE_FOR_CODE_GEN_FROM_STRINGS_INDEX, Object, \
error_message_for_code_gen_from_strings) \
@@ -282,6 +283,7 @@
CONTEXT_EXTENSION_FUNCTION_INDEX,
OUT_OF_MEMORY_INDEX,
CONTEXT_DATA_INDEX,
+ CONTEXT_EMBEDDER_DATA_INDEX,
ALLOW_CODE_GEN_FROM_STRINGS_INDEX,
ERROR_MESSAGE_FOR_CODE_GEN_FROM_STRINGS_INDEX,
TO_COMPLETE_PROPERTY_DESCRIPTOR_INDEX,

[sven...@chromium.org]

I took a look into the WebKit issue, and I think that your problems are
related
to the currently broken External class: Although it is declared as a
subclass of
Value (i.e. promises to return a JavaScript object), Wrap/New can currently
return an internal object (a Foreign), which in turn causes the assertion
you
mention in the isssue to be triggered.

I think the current CL is probably not needed when we agree on the proposal
I
suggested last week: Add an API for accessing aligned pointers, making
External
a real Value, and deprecating the old unaligned pointer API plus
Wrap/Unwrap.

https://codereview.chromium.org/11087020/

[aba...@chromium.org]

On 2012/10/09 06:30:47, Sven Panne wrote:
> I took a look into the WebKit issue, and I think that your problems are
related
> to the currently broken External class: Although it is declared as a
> subclass
of
> Value (i.e. promises to return a JavaScript object), Wrap/New can
> currently
> return an internal object (a Foreign), which in turn causes the assertion
> you
> mention in the isssue to be triggered.

Yes, that's what causes the ASSERT.

> I think the current CL is probably not needed when we agree on the
> proposal I
> suggested last week: Add an API for accessing aligned pointers, making
External
> a real Value, and deprecating the old unaligned pointer API plus
> Wrap/Unwrap.

That's certainly possible.

What's going on here is that we have a class (V8PerContextData) that holds a
bunch of WebKit data associated with a v8::Context. We need to be able to
get
to that object quickly from a handle to a v8::Context. I don't particularly
care what mechanism we use---just as long as it is fast. :)

I ran into trouble with v8::Context::SetData because of the External/Foreign
issue. I also had trouble removing all of the WebKit-side code that's using
SetData because there's a bunch of code that integrates with the V8 debugger
that reads the data from JavaScript (rather than from C++). Even if we
solve
the External/Foreign issue, I'm not sure if I actually use SetData because
of
that issue.

https://codereview.chromium.org/11087020/

[aba...@chromium.org]

I've updated the patch to re-phrase the API in terms of void* rather than
Handle<Value>. That way, we can change the implementation later when we
want to
get rid of External.

@Sven: Do you think we should move forward with this patch while we work out
what to do with External and pointer alignment?

https://codereview.chromium.org/11087020/

[sven...@chromium.org]

Extending the Context API with yet another field (of different type) is not
very
nice, because it hacks in just one specialized use case. I think that the
right
way would be providing a more general API in Context for adding any number
of
embedder data fields, similar to the proposed API for Object:

class Context {
...
Local<Value> GetInternalField(int index);
void SetInternalField(int index, Handle<Value> data);

void* GetAlignedPointerFromInternalField(int index);
void SetAlignedPointerInInternalField(int index, void* ptr);
}

The number of internal fields would be dynamic (i.e. we would reallocate if
necessary), and the old SetData/GetData will be deprecated, but implemented
via
field 0 in the meantime. We can probably totally inline the getters in the
header, so things would be quite efficient.

This would be consistent with the other proposal, we will be able solve the
typing problems with External, and you can even associate any number of
embedder
data with a Context.

If this is OK, I think I can implement all this in the new one or two
days...

https://codereview.chromium.org/11087020/

[aba...@chromium.org]

> If this is OK, I think I can implement all this in the new one or two
> days...

Sure, that sounds great. Thanks for giving this issue some attention!

https://codereview.chromium.org/11087020/