[Navigation API] Inconsistency between spec and tests for navigating methods and detached iframes

[jnjaeschke]

What is the issue with the HTML Standard?

While implementing the navigation methods in Gecko it turned out that some test expectations do not match the specification expectations (if I understood everything correctly :) ).

Take this test as an example:

The test performs a navigation on a detached iframe. The navigation should fail, and according to the test it should return rejected promises containing the DOM exception.

However, since the iframe is detached, it is not fully active, which means that promises in the iframe's realm do not run.

Therefore, according to the spec, the mentioned test should time out. However, in Chrome the test passes.

cc @smaug---- @zcorpan @farre @domenic

[domenic]

This is a classic issue. https://github.com/whatwg/html/issues/2621 is probably the main discussion; https://github.com/whatwg/html/issues/4443 is slightly related.

which means that promises in the iframe's realm do not run.

This isn't quite right. As pointed out in https://github.com/whatwg/html/issues/2621#issuecomment-598249631 , the realm that is checked (i.e. the realm passed to HostEnqueuePromiseJob) is the realm of the handler function (i.e. in promise.then(fulfillHandler, rejectHandler). This follows from the JS spec's NewPromiseReactionJob definition.

In the tests you're linking to, the handlers are created in the outer realm. (By the code in testharness.js.) So they should still run.

Let me close this as I believe there's no conflict here. In fact, I feel like we can close #2621...

Of course, if I've missed something, I am happy to reopen.

[arai-a]

The testcase is actually wrong, but due to the helper being wrong. With the helper getting fixed, the testcase becomes correct without any change.

The actual issue comes from Promise.resolve called with a Promise object from a detached iframe, which happens inside Promise.all called inside the helper function assertBothRejectDOM.

https://searchfox.org/mozilla-central/rev/5e24bf00212b4f5c053c1f8d943becf1b5bfd53c/testing/web-platform/tests/navigation-api/navigation-methods/return-value/resources/helpers.js#109,114-123

window.assertBothRejectDOM = async (t, result, expectedDOMExceptionCode, w = window, domExceptionConstructor = w.DOMException) => {
...
  await Promise.all([
    result.committed.then(
      t.unreached_func("committed must not fulfill"),
      t.step_func(r => { committedReason = r; })
    ),
    result.finished.then(
      t.unreached_func("finished must not fulfill"),
      t.step_func(r => { finishedReason = r; })
    )
  ]);

Promise.all retrieves Promise.resolve function at the step 3, and PerformPromiseAll calls it in the step 4.d, for each item inside the passed iterable. Here, the each item is the promise returned by result.committed.then(...) call and result.finished.then(...) call above.

https://tc39.es/ecma262/#sec-promise.all

Promise.all ( iterable )

  1. Let C be the this value.
  2. Let promiseCapability be ? NewPromiseCapability(C).
  3. Let promiseResolve be Completion(GetPromiseResolve(C)).
  ...
  5. Let iteratorRecord be Completion(GetIterator(iterable, sync)).
  ...
  7. Let result be Completion(
       PerformPromiseAll(iteratorRecord, C, promiseCapability, promiseResolve)).
  ...

https://tc39.es/ecma262/#sec-performpromiseall

PerformPromiseAll ( iteratorRecord, constructor, resultCapability,
                    promiseResolve )

  ...
  4. Repeat,
    a. Let next be ? IteratorStepValue(iteratorRecord).
    ...
    d. Let nextPromise be ? Call(promiseResolve, constructor, « next
       »).

result.committed belongs to the iframe's realm, and thus result.committed.then function also belongs to the iframe's realm, and thus the resultCapability's promise created at Promise.prototype.then step 4 also belongs to the iframe's realm, and thus what Promise.all receives is those promises from the iframe's realm.

https://tc39.es/ecma262/#sec-promise.prototype.then

Promise.prototype.then ( onFulfilled, onRejected )

...
  4. Let resultCapability be ? NewPromiseCapability(C).
  5. Return PerformPromiseThen(promise, onFulfilled, onRejected,
     resultCapability).

Then, PromiseResolve performed by Promise.resolve first checks if the parameter is a Promise object from the same realm, and this check fails because the parameter is the promise comes from the iframe's realm. Thus, PromiseResolve calls promiseCapability.[[Resolve]], which is "Promise Resolve Functions".

https://tc39.es/ecma262/#sec-promise.resolve

Promise.resolve ( x )

  1. Let C be the this value.
  2. If C is not an Object, throw a TypeError exception.
  3. Return ? PromiseResolve(C, x).

https://tc39.es/ecma262/#sec-promise.resolve

PromiseResolve ( C, x )

  1. If IsPromise(x) is true, then
    a. Let xConstructor be ? Get(x, "constructor").
    b. If SameValue(xConstructor, C) is true, return x.
  2. Let promiseCapability be ? NewPromiseCapability(C).
  3. Perform ? Call(promiseCapability.[[Resolve]], undefined, « x »).
  4. Return promiseCapability.[[Promise]].

The "Promise Resolve Functions" gets the then property of the passed promise, and creates a job callback thenJobCallback with HostMakeJobCallback, and then creates a job job with NewPromiseResolveThenableJob.

The thenJobCallback.[[Callback]] is the then property value, which belongs to the iframe's realm, and thus NewPromiseResolveThenableJob step 3 gets the iframe's realm. and the job.[[Realm]] becomes the iframe's realm.

Then, given that the iframe's detached, the thenable job is not called, and the promise for Promise.resolve(...) never resolves, that results in the Promise.all(...) above never resolves, and the test hits timeout.

https://tc39.es/ecma262/#sec-promise-resolve-functions

Promise Resolve Functions

  ...
  3. Let promise be F.[[Promise]].
  ...
  9. Let then be Completion(Get(resolution, "then")).
  ...
  11. Let thenAction be then.[[Value]].
  12. If IsCallable(thenAction) is false, then
    ...
    b. Return undefined.
  13. Let thenJobCallback be HostMakeJobCallback(thenAction).
  14. Let job be NewPromiseResolveThenableJob(promise, resolution,
      thenJobCallback).
  15. Perform HostEnqueuePromiseJob(job.[[Job]], job.[[Realm]]).
  16. Return undefined.

https://html.spec.whatwg.org/#hostmakejobcallback

HostMakeJobCallback(callable)

  1. Let incumbent settings be the incumbent settings object.
  2. Let active script be the active script.
  3. Let script execution context be null.
  4. If active script is not null, set script execution context to
     a new JavaScript execution context, with its Function field set to null,
     its Realm field set to active script's settings object's realm,
     and its ScriptOrModule set to active script's record.
  5. Return the JobCallback Record {
       [[Callback]]: callable,
       [[HostDefined]]: {
         [[IncumbentSettings]]: incumbent settings,
         [[ActiveScriptContext]]: script execution context
       }
     }.

https://tc39.es/ecma262/#sec-newpromiseresolvethenablejob

NewPromiseResolveThenableJob ( promiseToResolve, thenable, then )

  1. Let job be a new Job Abstract Closure with no parameters that
     captures promiseToResolve, thenable, and then and performs the following
     steps when called:
    ...
  2. Let getThenRealmResult be
     Completion(GetFunctionRealm(then.[[Callback]])).
  3. If getThenRealmResult is a normal completion,
       let thenRealm be getThenRealmResult.[[Value]].
  4. Else,
       let thenRealm be the current Realm Record.
  5. NOTE: thenRealm is never null. When then.[[Callback]] is a revoked
     Proxy and no code runs, thenRealm is used to create error objects.
  6. Return the Record { [[Job]]: job, [[Realm]]: thenRealm }.

A possible fix is not to perform Promise.resolve on the promises from the iframe's realm. For example, rewriting the await Promise.all(...) with the following makes the test pass on Firefox.

  await new Promise(resolve => {
    let remaining = 2;
    function resolveOne() {
      remaining--;
      if (remaining == 0) {
        resolve();
      }
    }
    result.committed.then(
      t.unreached_func("committed must not fulfill"),
      t.step_func(r => { committedReason = r; resolveOne(); })
    );
    result.finished.then(
      t.unreached_func("finished must not fulfill"),
      t.step_func(r => { finishedReason = r; resolveOne(); })
    );
  });

[zcorpan]

Reopening to track fixing the test expectation or changing helpers.js as @arai-a suggested.

[domenic]

I've traced your reasoning and agree with it.

This is an annoyingly subtle interop bug, and I wonder what part of the spec Chromium is violating to cause the cross-realm Promise.all() to succeed. (It could be in the JS engine, or the event loop processing, or...)

As an immediate step, I'd suggest fixing the test helper in the manner you suggest, adding a comment like the following:

We cannot use Promise.all() because the automatic coercion behavior when promises from multiple realms are involved causes it to hang if one of the promises is from a detached iframe's realm. See discussion at https://github.com/whatwg/html/issues/11252#issuecomment-2984143855 .

If you want to go the extra mile, a failing test case in WPT for a more isolated (non-navigation-API-specific) case, and a Chromium bug, would be appreciated.

[arai-a]

Thank you for verifying :)

To my understanding Chromium and (recent) Safari runs jobs even if the global is not fully active, or at least even if the global is detached iframe. And that would be the reason of the behavior difference here.

https://bugzilla.mozilla.org/show_bug.cgi?id=1663090 has some history.

Chromium has been running jobs in detached iframe at lease from 2020, and it keeps the behavior until now, with slight fix for local file. Safari had been skipping those jobs in 2020, but from some unknown point, it started to run jobs in detached iframe.

There are bugs filed for them:

• Chromium: https://issues.chromium.org/issues/378827518
• Safari/WebKit: https://bugs.webkit.org/show_bug.cgi?id=216149 (it was filed about the initial "not running jobs" behavior tho)

I'll look into fixing the helper and adding the WPT.

[arai-a]

Posted the WPT for promises and detached iframe in https://bugzilla.mozilla.org/show_bug.cgi?id=1972990

[farre]

And now the rewrite to not use Promise.all in assertBothRejectDOM is up: https://bugzilla.mozilla.org/show_bug.cgi?id=1972974

This makes Firefox pass a couple of more tests, and I tested so that Chrome still pass as expected.