Hi,
I will submit the following patch to V8 (option B from comment #2), which
aligns the C++ code with the Torque version.
diff --git a/deps/v8/src/objects/objects.cc b/deps/v8/src/objects/objects.cc
index 3e8fa8d3368f..7b40af964558 100644
--- a/deps/v8/src/objects/objects.cc
+++ b/deps/v8/src/objects/objects.cc
@@ -5166,13 +5166,13 @@ MaybeHandle<Object>
JSPromise::Resolve(DirectHandle<JSPromise> promise,
MaybeDirectHandle<Object> then;
// Make sure a lookup of "then" on any JSPromise whose [[Prototype]] is
the
- // initial %PromisePrototype% yields the initial method. In addition this
- // protector also guards the negative lookup of "then" on the intrinsic
- // %ObjectPrototype%, meaning that such lookups are guaranteed to yield
- // undefined without triggering any side-effects.
+ // initial %PromisePrototype% (in the current native context) yields the
+ // initial method. In addition this protector also guards the negative
lookup
+ // of "then" on the intrinsic %ObjectPrototype%, meaning that such
lookups are
+ // guaranteed to yield undefined without triggering any side-effects.
if (IsJSPromise(*resolution_recv) &&
- resolution_recv->map()->prototype()->map()->instance_type() ==
- JS_PROMISE_PROTOTYPE_TYPE &&
+ resolution_recv->map()->prototype()->SafeEquals(
+ *isolate->promise_prototype()) &&
Protectors::IsPromiseThenLookupChainIntact(isolate)) {
// We can skip the "then" lookup on {resolution} if its [[Prototype]]
// is the (initial) Promise.prototype and the Promise#then protector
I have a possible workaround on the Node side: the inner module registers a
callback with the outer microtask queue, and whenever the outer queue
completes a checkpoint, the callback tries to checkpoint the inner queue
too, just in case. This is a little silly as there will typically be
nothing to drain on the inner queue. At least it's not an expensive
operation to invoke PerformCheckpoint() on an empty queue.
Thanks,
Eric
On Fri, Sep 5, 2025 at 12:06 PM Eric Rannaud <[email protected]> wrote:
> Hi Leszek,
>
> On Fri, Sep 5, 2025 at 7:36 AM Leszek Swirski <[email protected]>
> wrote:
>
>> It sounds like you've found a bug in our C++ implementation which is
>> incompatible with the spec; I'd have to read into the exact details but it
>> sounds plausible. This is great, and we should fix this, however your
>> subsequent comments sound like they want the broken behaviour?
>>
>
> Something in between. While the C++ code is closer, I think we'd want a
> variant: we want the C++ behavior only if the resolution is an
> already-fulfilled promise, otherwise we want the Torque behavior.
>
>
>> In general, for us, spec is non-negotiable (the few times we do break
>> spec is for web compatibility or some serious performance concern), so if
>> you think a break from spec behaviour would be useful for Node, then you'd
>> want to make that a spec proposal.
>>
>
> Understood.
>
> The nested microtask queues Node is using sound a bit broken, since it
>> sounds like the microtask queue is only drained once? If I understand the
>> issue correctly, then the right fix would be for Node to always drain the
>> inner microtask queue after something executes in the inner realm, or, I
>> guess, patch the native context after draining it to refer to the outer
>> microtask queue.
>>
>
> I did consider patching the context to point to the outer queue after
> evaluate(), or just set the queue to null, but in typical node:vm usage
> this context object may be shared with other (inner) modules that may be
> evaluated later, so I don't think we can actually modify it.
>
> Every time some code awaits on the already-fulfilled promise returned by
> evaluate(), a new (trivial) task will appear on the inner microtask queue,
> otherwise (long) inactive.
>
> The issue for Node is that there is no way of knowing that the inner
> microtask queue should be drained. This (trivial) task gets enqueued after
> inner_module.evaluate() returns, at a point where we are done with
> inner_module, and I'm not sure where there would be a reasonable place to
> check "what if the queue for this other context we're not using anymore
> needs draining?"
>
> Another way to describe the issue is that with two contexts A and B, with
> queues QA and QB, context B is effectively waiting on something to happen
> in context A (the trivial thenable job task), but while there is a task
> enqueued on QA, there are no tasks enqueued on QB to indicate that the
> (outer) module B has more work to do, leading to the evaluation of module B
> to just terminate early. The lack of dependency tracking between contexts B
> and A is what is broken in the spec, I think.
>
> Whenever context B enqueues something onto QA, it is possible (but not
> guaranteed) that A will in turn enqueue something back onto QB. So, context
> B should be aware of that possibility and consider itself waiting on a QA
> checkpoint. It's only after a QA checkpoint is performed that context B can
> check if its queue is effectively empty or not.
>
>
> Possibly, In Node or in v8::SourceTextModule, after finding that the
> current queue is empty, before terminating the evaluation of the current
> module, we could check if any other "related" microtask queues are
> non-empty and drain them, before checking if doing this added any tasks to
> our current queue.
>
>
> Or, after all, it may be worth considering another option I included in
> comment #3: "In V8, add a callback to the public API MicrotaskQueue to be
> notified, with policy kExplicit: either when a task is enqueued, or more
> efficiently, when there are enqueued tasks in strategic places (maybe in
> the same places where, with policy kAuto, a checkpoint may be performed).
> This would let Node know that the qeueue associated with an
> already-evaluated module needs to be drained again. I have not tried this,
> but it seems very clunky."
>
> This callback could be associated with the queue:
> OnMicrotaskEnqueue(microtask_queue). In Node, we would maintain enough
> state to know if we've entered a context associated with this queue. In the
> callback: if the queue is in an entered context, we know that the queue
> will get drained later, we have nothing to do; but if this queue is not
> associated with an entered context, we immediately drain the queue.
>
> Or this callback could be associated with the native context:
> OnMicrotaskEnqueue(native_context). The Node implementation of this
> callback would be simpler, as we only need to check if native_context
> matches the isolate current native context.
>
> Thanks,
> Eric
>
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups
"v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/v8-dev/CA%2BzRj8WvoH5Jc5V_aJa%2B_QTWtSX2XzXYBev8XC-J433yZNZjfQ%40mail.gmail.com.
