[webkit-reviews] review granted: [Bug 120954] Update Promises to the https://github.com/domenic/promises-unwrapping spec : [Attachment 220116] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 120954: Update Promises to the
https://github.com/domenic/promises-unwrapping spec
https://bugs.webkit.org/show_bug.cgi?id=120954

Attachment 220116: Patch
https://bugs.webkit.org/attachment.cgi?id=220116&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=220116&action=review


> Source/JavaScriptCore/runtime/JSGlobalObject.h:113
> -class TaskContext : public RefCounted<TaskContext> {
> +class MicroTask : public RefCounted<MicroTask> {
>  public:
> -    virtual ~TaskContext()
> +    virtual ~MicroTask()
>      {
>      }
> +
> +    virtual void run(ExecState*) = 0;
>  };

You could be a bro and move this to a separate file while you're at it.

> Source/JavaScriptCore/runtime/JSPromise.cpp:91
> +    for (size_t i = 0; i < thisObject->m_resolveReactions.size(); ++i)
> +	   visitor.append(&thisObject->m_resolveReactions[i]);
> +    for (size_t i = 0; i < thisObject->m_rejectReactions.size(); ++i)
> +	   visitor.append(&thisObject->m_rejectReactions[i]);

You could be a bro and give SlotVisitor an STL-style append(beginIterator,
endIterator) helper.  And then use that here.

> Source/JavaScriptCore/runtime/JSPromise.cpp:161
> +    // 1. Repeat for each reaction in reactions, in original insertion order

> +    for (auto& reaction : reactions) {
> +	   // i. Call QueueMicrotask(ExecutePromiseReaction, (reaction,
argument)).
> +	  
globalObject->queueMicroTask(createExecutePromiseReactionMicroTask(vm,
reaction.get(), argument));

This is super scary.  You've swapped the reactions array, which holds pointers
to the heap, into a stack-allocated vector, which then points to a
C-heap-allocated buffer of GC-invisible pointers to the GC heap.  And then you
proceed to loop over something using some C++ hipsterishness and repeatedly
call JSGlobalObject methods.  The following will likely eventually happen if it
hasn't happened already:

- Whatever methods the C++ compiler secretly calls as part of the for loop
iteration will accidentally trigger a GC.
- JSGlobalObject::queueMicroTask() will trigger a GC.
- createExecutePromiseReactionMicroTask() will trigger a GC.

I think that you probably already have a bug here, and even if you don't,
you're creating a high risk of buggishness in the future.  I think that it's
good practice to either:

- Ensure that all of your heap pointers are always in something that is
reachable from roots.  This means that Vector<WriteBarrier<>> is a terrible
idea unless that vector could at any instant be traced from some visitChildren
method on some object that is visible from the stack.  This isn't the case here
since you're swapping into a local vector.

- Ensure that during the span of time where the Vector<WriteBarrier<>> is live,
you're only doing obviously-not-GCing things.  To me this means sticking to the
C subset of C++ and putting yourself into the same kind of mentality that one
would have when writing a kernel interrupt handler.  This isn't the case here
since you're doing a lot of exotic methods.  To flip this around, if you're
hacking some function inside JSC you probably want to be able to assume that
you can allocate objects.  This is almost always true, so it's common to see
patches that add new allocations, and that's OK.  But this little bit of code
makes that not the case: if you add an allocation to
JSGlobalObject::queueMicroTask(), createExecutePromiseReactionMicroTask(), or
whatever C++ secretly calls for that for loop, then you're dead.

- Use my best friend, DeferGC.	It's a RAII-style guard that defers GC around
GC-unsafe regions.  You could slap that bad boy around JSPromise::reject() or
something, and then everything would be fine.

The DeferGC approach is kind of dirty so we don't want to be using it
everywhere, but it's probably fine here, unless you can find obvious ways to
either make the vector reachable in GC or to make it obvious that a GC cannot
ever happen here.

> Source/JavaScriptCore/runtime/JSPromiseDeferred.cpp:88
> +JSValue getDeferred(ExecState* exec, JSValue C)

Can we change the name of this method to reflect the fact that it's got
something to do with promises?	Right now it's got a very generic-looking
signature:

JSValue getDeferred(ExecState*, JSValue)

Could mean anything...

> Source/JavaScriptCore/runtime/JSPromisePrototype.cpp:169
> +	   // ii. Call QueueMicrotask(ExecutePromiseReaction, (resolveReaction,
resolution)).
> +	  
globalObject->queueMicroTask(createExecutePromiseReactionMicroTask(vm,
resolveReaction, resolution));

Is it MicroTask or Microtask?