Add API support for passing a C++ function as a microtask callback

Add API support for passing a C++ function as a microtask callback

This allows embedders to enqueue microtasks without having any v8::Context
handy, as happens in Blink in some cases (such as DOM mutations due to editing
triggering MutationObservers).

LOG=Y
R=dcarney@chromium.org

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

[adamk, 2014-05-29 23:15:56]

This could be factored slightly differently: perhaps the wrapping logic should
live in isolate.cc, so that the producer and consumer of the CallHandlerInfo
structs are nearby in the code (it'd also let Isolate::EnqueueMicrotask stay
typesafe).

[dcarney, 2014-06-02 07:22:10]

lgtm, with api change

https://codereview.chromium.org/306053003/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/306053003/diff/1/src/api.cc#newcode6670
src/api.cc:6670: i::Handle<i::CallHandlerInfo> callback_info =
if you're going to reuse the CallHandlerInfo struct, the second field must be a
Handle<Value>.
the embedder can use External::New if needed.  this requires no context

https://codereview.chromium.org/306053003/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/306053003/diff/1/src/isolate.cc#newcode2323
src/isolate.cc:2323: ASSERT(microtask->IsCallHandlerInfo());
no need to assert, ::cast does that

[adamk, 2014-06-02 16:41:49]

https://codereview.chromium.org/306053003/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/306053003/diff/1/src/api.cc#newcode6670
src/api.cc:6670: i::Handle<i::CallHandlerInfo> callback_info =
On 2014/06/02 07:22:10, dcarney wrote:
> if you're going to reuse the CallHandlerInfo struct, the second field must be
a
> Handle<Value>.
> the embedder can use External::New if needed.  this requires no context

I'm guessing this might be the source of my try job failure...but could you
explain what's enforcing this requirement?

Also, it seems a bit weird to me to make an External part of this API, given
that otherwise it's C++-type-based, not V8-type-based. Would you be OK with the
External::New call happening internally?

As for re-using CallHandlerInfo: I at first started to write a new Struct for
this, but CallHandlerInfo seemed so close to what I needed that it seemed like a
lot of useless code. If you think it's weird to re-use it here, I can easily add
a new struct for this case.

[adamk, 2014-06-02 18:51:20]

On 2014/06/02 16:41:49, adamk wrote:
> https://codereview.chromium.org/306053003/diff/1/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/306053003/diff/1/src/api.cc#newcode6670
> src/api.cc:6670: i::Handle<i::CallHandlerInfo> callback_info =
> On 2014/06/02 07:22:10, dcarney wrote:
> > if you're going to reuse the CallHandlerInfo struct, the second field must
be
> a
> > Handle<Value>.
> > the embedder can use External::New if needed.  this requires no context
> 
> I'm guessing this might be the source of my try job failure...but could you
> explain what's enforcing this requirement?

I looked at this harder and I don't see any place that this requirement is
codified. The verifier for CallHandlerInfo doesn't assume anything about the
type of |data|, it simply calls VerifyPointer() on it. Sure, the other places
that use CallHandlerInfo assume it's OK to pass |data| back out to the embedder,
in which case it would of course need to be something representable as a
v8::Value, but given that I handle the unwrapping in isolate.cc I don't see
what's wrong with this approach.

Unless you're just arguing that I shouldn't be reusing CallHandlerInfo unless I
use it in exactly the same way as the other uses. As I said in my previous
message, I think I'd rather create a new struct and allow the API to remain
V8-type-free than force the embedder to wrap and unwrap the void* on the
outside.

> Also, it seems a bit weird to me to make an External part of this API, given
> that otherwise it's C++-type-based, not V8-type-based. Would you be OK with
the
> External::New call happening internally?
> 
> As for re-using CallHandlerInfo: I at first started to write a new Struct for
> this, but CallHandlerInfo seemed so close to what I needed that it seemed like
a
> lot of useless code. If you think it's weird to re-use it here, I can easily
add
> a new struct for this case.

[dcarney, 2014-06-02 21:11:04]

> I looked at this harder and I don't see any place that this requirement is
> codified.

In the garbage collector.  Without an explicit visitor that ignores that field,
it will be relocated.

> > As for re-using CallHandlerInfo: I at first started to write a new Struct
for
> > this, but CallHandlerInfo seemed so close to what I needed

In a way CallHandlerInfo is exactly what you want.  So I'd say use it.  If you
want to wrap the void* up in External internally, that's fine with me.

[adamk, 2014-06-02 21:25:27]

On 2014/06/02 21:11:04, dcarney wrote:
> > I looked at this harder and I don't see any place that this requirement is
> > codified.
> 
> In the garbage collector.  Without an explicit visitor that ignores that
field,
> it will be relocated.

It sounds like there's something being lost in this discussion. |data| is being
transformed into a v8::internal::Foreign by the SET_FIELD_WRAPPED macro. Foreign
is already a HeapObject with explicit GC code attached (ForeignIterateBody). In
fact, this is the same thing used by v8::External, but that's wrapped in the
internal field of a JSObject before being passed back through the API. I'm
simply skipping the JSObject wrapper.

I apologize if I'm being dense, but from what I can tell storing a Foreign in
|data| is completely safe. Clearly, storing a void* directly would not be.

> > > As for re-using CallHandlerInfo: I at first started to write a new Struct
> for
> > > this, but CallHandlerInfo seemed so close to what I needed
> 
> In a way CallHandlerInfo is exactly what you want.  So I'd say use it.  If you
> want to wrap the void* up in External internally, that's fine with me.

[dcarney, 2014-06-03 06:42:10]

> I apologize if I'm being dense, but from what I can tell storing a Foreign in
> |data| is completely safe. Clearly, storing a void* directly would not be.

you're right.  very sorry for the back and forth.  SET_FIELD_WRAPPED was added
recently i think and i didn't look into what it was doing.

everything looks fine as is

[adamk, 2014-06-03 20:12:26]

Committed patchset #1 manually as r21658.