Bug 190843 – Rejected dynamic import should be consistent on the success/failure paths

• Status: NEW
• Component: JavaScriptCore

[Leo Balter]

From the spec proposal for `import()`:

https://tc39.github.io/proposal-dynamic-import/#sec-hostimportmoduledynamically

---
Failure path:
- At some future time, the host environment must perform FinishDynamicImport(referencingScriptOrModule, specifier, promiseCapability, an abrupt completion), with the abrupt completion representing the cause of failure.

The intent of this specification is to not violate run to completion semantics. The spec-level formalization of this is a work-in-progress.

Every call to HostImportModuleDynamically with the same referencingScriptOrModule and specifier arguments must conform to the same set of requirements above as previous calls do. That is, if the host environment takes the success path once for a given referencingScriptOrModule, specifier pair, it must always do so, and the same for the failure path.
---

So if I have 2 import calls to the same pair of referencingScriptorModule and specifier, and if they go to a failure path from the evaluation of a module by an evaluation abrupt completion, JSC should keep it consistent:

```
async function fn() {
    let err;
    let result = {};
    const keep = result;
    try {
        result = await import('./poisoned.js');
    } catch (error) {
        err = error;
    }
    assert.sameValue(err, 'foo', 'first evaluation should be an abrupt completion');
    assert.sameValue(result, keep, 'result should not be set');

    err = undefined;

    try {
        result = await import('./poisoned.js');
    } catch (error) {
        err = error;
    }

    assert.sameValue(result, keep, 'result should still be the same as keep');
    assert.sameValue(err, 'foo', 'second evaluation should repeat the same abrupt completion');
}

fn().then($DONE, $DONE);
```

the problem here is, the second import to the same poisoned module should repeat the same rejection, summarizing:

```
import('./poisoned.js'); // rejected promise
import('./poisoned.js'); // should also be a rejected promise, but JSC is fulfilling it
```

In that way, I get the second try catch to continue the evaluation up to setting the resolved value to `result`, which is a namespace object created from `./poisoned.js`.

Let's say this is my poisoned module:

```
export var x = 1;
throw 'foo';
export var y = 2;
```

The resolved namespace object is giving me the x and y exported names, with x set to 1, and y being undefined.

---

The expected result should be a rejected promise like on the import() usage.

I'm currently adding tests to cover this on Test262, I hope it helps.

[Ross Kirsling]

Not certain whether it's problematic, but I have noticed one interesting thing:

If the imported module throws on evaluation, we never actually return to the JS-side callsite for module evaluation:
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/builtins/ModuleLoader.js#L315

...even though an encoded jsNull is returned from the CPP side in the usual way:
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/JSModuleLoader.cpp#L488

[Ross Kirsling]

Regarding my earlier comment:
I suppose the reason why control never returns to ModuleLoader.js on throw is because builtins JS is just JS and we're throwing all the way to the userland catch?
And indeed, that might be okay, since we don't really need to getModuleNamespaceObject in that case.

Either way, the second time, the link and evaluate steps early out, so we jump straight to getModuleNamespaceObject.
I've confirmed that redoing both link and evaluate the second time fixes the issue, so it may just be a matter of doing that in the "correct" way.

[Radar WebKit Bug Importer]

<rdar://problem/61740823>

[Yusuke Suzuki]

Created attachment 396424 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 396424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396424&action=review

> Source/JavaScriptCore/builtins/ModuleLoader.js:102
> +        evaluationError: @undefined,
> +        evaluationSucceeded: true,

We need to have two fields. Otherwise, cannot recognize `throw undefined` case.

[Yusuke Suzuki]

Created attachment 396426 [details]
Patch

[Yusuke Suzuki]

Created attachment 396427 [details]
Patch

[Yusuke Suzuki]

Created attachment 396435 [details]
Patch

[Michael Saboff]

Comment on attachment 396435 [details]
Patch

r=me

[Yusuke Suzuki]

Comment on attachment 396435 [details]
Patch

Talked in slack. I think we need to introduce @rethrow to keep debug information nice while fixing this bug. Defer it for now.