2050226 - await in detached iframe never resumes

Status: RESOLVED DUPLICATE of bug 1663090

(Core :: JavaScript Engine, defect, P3)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: await-min.html

See attached test case.

Firefox prints:

[ATTACHED] started
[DETACHED] started
[ATTACHED] red AWAIT(microtask) RESUMED
=== DONE (1.5s) ===

Chrome prints:

[ATTACHED] started
[DETACHED] started
[ATTACHED] red AWAIT(microtask) RESUMED
[DETACHED] red AWAIT(microtask) RESUMED
=== DONE (1.5s) ===

Notice [DETACHED] red AWAIT(microtask) RESUMED is missing

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

This might not be a JS engine issue, but Promise itself and await are rather JS-y.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

I have a vague memory of :arai looking at this in the past?

https://github.com/whatwg/html/issues/1989 / https://github.com/whatwg/html/issues/2621 are relevant here.

Comment 3

[Tooru Fujisawa [:arai]]

possibly a dupe of bug 1663090

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: LLM generated patch to fix this

No idea if this patch is correct at all but it does fix the test case, and when combined with a fix for bug 2050063 fixes bug 1937727, bug 1957422, and bug 2047919

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)

No idea if this patch is correct at all but it does fix the test case, and when combined with a fix for bug 2050063 fixes bug 1937727, bug 1957422, and bug 2047919

Note that the removal of the IsDying check from workers by way of the removal of the call to IsScriptForbidden is fairly concerning since it would reintroduce the potential for content to be running on workers when it's now currently denied, which can be a real problem for C++ code that's not prepared for it. (The global does continue to technically be alive for the purposes of the HasJSGlobal checks for quite some time, which is why they are insufficient.)

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Ah, :mgaudet's changes in this patch mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1663090#c24 should be fine though, since workers get to retain their IsDying check.

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

My impression is that the spec doesn't address any distinction between workers and windows here... but also, it seems sketchy as heck given the complications of worker shutdown I'm aware of.

My knowledge of worker semantics is a bit too weak to be able to say concretely whether or not we'd want to raise that to a specification change to highlight workers should be handled differently, or if this is actually just impossible to observe

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

So a big divergence between the specs when it comes to workers and the reality of our implementation that ends up interacting with everything here is that terminating a worker is supposed to :

• Step 2 of terminate a worker is "If there are any tasks queued in the WorkerGlobalScope object's relevant agent's event loop's task queues, discard them without processing them."
• And step 1 was setting the closing flag to true which means "the event loop's task queues must discard any further tasks that would be added to them".
• Step 3 is "Abort the script currently running in the worker."

So termination is supposed to conceptually stop the current task in its tracks and prevent any other tasks from ever running again. To deal with the reality of multi-threaded code, the worker will continue to potentially run runnables for quite some time, specifically as long as StrongWorkerRefs are held. There had been a broken attempt to require all runnables dispatched to workers to be nsICancelable (later nsIDiscardableRunnable), but it wasn't foolproof and most code extended to support workers did not handle the situation correctly or even well.

So we need the dying mechanism as a backstop / defense-in-depth to enforce this state of "the spec says no tasks should ever run beyond this point". While I know we are talking about microtasks here, there is a meta concern that a spec implementation that looks something like "queue a task to run on global (that is a worker) and then in that task queue a microtask or resolve a promise" and we have been depending on the dying check to let naive code get away with being naive and not necessarily having to call mozilla::GlobalTeardownObserver::CheckCurrentGlobalCorrectness (where the comment should be updated) from a runnable that corresponds to a spec task or otherwise potentially induce the running of script.

I am happy to go into more of the structural issues and how we might address them, but perhaps one thing we could do is to try and rename "dying" to more directly correlate to a terminated worker. Unfortunately, DedicatedWorkerGlobalScope.close which invokes close a worker is notably distinct from terminate a worker in that it continues to let the existing JS on the stack run and I believe is allowed to process microtasks, so "closing" is not an apt description of what's going on (and also would not convey the urgency inherent in termination). We could certainly rename IsDying to IsTerminating and given how the flag is specialized to workers, we could make it IsTerminatingWorker.

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

This now prints what Chrome prints; I'm inclined to dupe this to 1663090