Bug 148173 – [ES6] Return JSInternalPromise as result of evaluateModule

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Report asynchronous module loader errors

[Yusuke Suzuki]

While implementing this, I've found one memory leak error in the JSStdFunction...
I'll create the separate patch to fix that.

[Yusuke Suzuki]

Created attachment 259587 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 259587 [details]
Patch

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

The patch is ready. Added comment for the ease of the review.

> Source/JavaScriptCore/jsc.cpp:1375
> +    });

The error handler function to catch the asynchronous errors and report them.

> Source/JavaScriptCore/jsc.cpp:1388
> +                promise = evaluateModule(globalObject->globalExec(), fileName);

Load the module with the entry point moduleName.

> Source/JavaScriptCore/jsc.cpp:1407
> +                promise = evaluateModule(globalObject->globalExec(), jscSource(script, fileName));

Load the module with the source string.

> Source/JavaScriptCore/jsc.cpp:1409
> +            promise->then(globalObject->globalExec(), nullptr, errorHandler);

JSInernalPromise.then with the error handler to report the asynchronous errors.

> Source/JavaScriptCore/jsc.cpp:1410
>              globalObject->vm().drainMicrotasks();

This draining ensures that the loading, execution and error reporting of the module are done here before loading the next file.

> Source/JavaScriptCore/runtime/JSInternalPromise.h:42
> +// CAUTION: Must not leak the JSInternalPromise to the user space to keep its integrity.

Add the note about JSInternalPromise.

> Source/JavaScriptCore/runtime/JSInternalPromise.h:52
> +    JS_EXPORT_PRIVATE JSInternalPromise* then(ExecState*, JSFunction* = nullptr, JSFunction* = nullptr);

We just add "then" helper only to JSInternalPromise (we don't add it to JSPromise) because "then" method may be replaced/modified in the user-exposed Promises.

> Source/JavaScriptCore/runtime/ModuleLoaderObject.cpp:150
> +    return jsCast<JSInternalPromise*>(call(exec, function, callType, callData, this, arguments));

Since "requestInstantiateAll" is builtin internal method, we can ensure that the result of this function is always JSInternalPromise*.

> Source/JavaScriptCore/runtime/ModuleLoaderObject.cpp:164
> +    return jsCast<JSInternalPromise*>(call(exec, function, callType, callData, this, arguments));

Ditto.

> Source/JavaScriptCore/runtime/ModuleLoaderObject.h:74
> +    JSInternalPromise* instantiate(ExecState*, JSValue key, JSValue source);

Ensure the result value is JSInternalPromise* by type!

[Yusuke Suzuki]

Created attachment 259588 [details]
Patch

add ifdef guard for promise = ...

[Yusuke Suzuki]

The patch is ready. 
The following is the loading results.


===== tmp/main.js
import "tmp/test001.js"

===== tmp/test001.js
import 'tmp/arith.js'
import 'tmp/test001.js'

===== tmp/arith.js
import "tmp/test001.js"

export function add(a, b)
{
    return a + b;
}

= result
$ Tools/Scripts/run-jsc -m tmp/main.js
Running 1 time(s): DYLD_FRAMEWORK_PATH=/Users/yusukesuzuki/development/Modules/WebKitBuild/async/Release /Users/yusukesuzuki/development/Modules/WebKitBuild/async/Release/jsc -m tmp/main.js
Loader [resolve] tmp/main.js
Loader [fetch] tmp/main.js
Loader [translate] tmp/main.js
Loader [instantiate] tmp/main.js

Analyzing ModuleRecord key('tmp/main.js')
    Dependencies: 1 modules
      module('tmp/test001.js')
    Import: 0 entries
    Export: 0 entries
Loader [resolve] tmp/test001.js
Loader [fetch] tmp/test001.js
Loader [translate] tmp/test001.js
Loader [instantiate] tmp/test001.js

Analyzing ModuleRecord key('tmp/test001.js')
    Dependencies: 2 modules
      module('tmp/arith.js')
      module('tmp/test001.js')
    Import: 0 entries
    Export: 0 entries
Loader [resolve] tmp/arith.js
Loader [resolve] tmp/test001.js
Loader [fetch] tmp/arith.js
Loader [translate] tmp/arith.js
Loader [instantiate] tmp/arith.js

Analyzing ModuleRecord key('tmp/arith.js')
    Dependencies: 1 modules
      module('tmp/test001.js')
    Import: 0 entries
    Export: 1 entries
      [Local] export('add'), local('add')
Loader [resolve] tmp/test001.js


===== tmp/bad.js
import "tmp/not-found.js"


= result
$ Tools/Scripts/run-jsc -m tmp/bad.js
Running 1 time(s): DYLD_FRAMEWORK_PATH=/Users/yusukesuzuki/development/Modules/WebKitBuild/async/Release /Users/yusukesuzuki/development/Modules/WebKitBuild/async/Release/jsc -m tmp/bad.js
Loader [resolve] tmp/bad.js
Loader [fetch] tmp/bad.js
Loader [translate] tmp/bad.js
Loader [instantiate] tmp/bad.js

Analyzing ModuleRecord key('tmp/bad.js')
    Dependencies: 1 modules
      module('tmp/not-found.js')
    Import: 0 entries
    Export: 0 entries
Loader [resolve] tmp/not-found.js
Loader [fetch] tmp/not-found.js
Could not open file: tmp/not-found.js
Exception: Error: Could not open file 'tmp/not-found.js'.
fetch@[native code]
requestFetch@[native code]
requestTranslate@[native code]
requestInstantiate@[native code]
requestResolveDependencies@[native code]
[native code]
promiseReactionJob@[native code]




===== tmp/bad2.js
import "tmp/bad-syntax.js"


===== tmp/bad-syntax.js
function


= result
$ Tools/Scripts/run-jsc -m tmp/bad2.js
Running 1 time(s): DYLD_FRAMEWORK_PATH=/Users/yusukesuzuki/development/Modules/WebKitBuild/async/Release /Users/yusukesuzuki/development/Modules/WebKitBuild/async/Release/jsc -m tmp/bad2.js
Loader [resolve] tmp/bad2.js
Loader [fetch] tmp/bad2.js
Loader [translate] tmp/bad2.js
Loader [instantiate] tmp/bad2.js

Analyzing ModuleRecord key('tmp/bad2.js')
    Dependencies: 1 modules
      module('tmp/bad-syntax.js')
    Import: 0 entries
    Export: 0 entries
Loader [resolve] tmp/bad-syntax.js
Loader [fetch] tmp/bad-syntax.js
Loader [translate] tmp/bad-syntax.js
Loader [instantiate] tmp/bad-syntax.js
Exception: SyntaxError: Unexpected end of script
parseModule@[native code]
instantiation@[native code]
commitInstantiated@[native code]
[native code]
promiseReactionJob@[native code]

[Saam Barati]

Comment on attachment 259588 [details]
Patch

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

r=me

> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:404
> +    // Loader.resolve hook point.

Maybe it would nice to also say: "resolve: moduleName => Promise(moduleKey)"
This is slightly redundant with the code, but I think it makes these hook points explicitly clear.

[Yusuke Suzuki]

Comment on attachment 259588 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/builtins/ModuleLoaderObject.js:404
>> +    // Loader.resolve hook point.
> 
> Maybe it would nice to also say: "resolve: moduleName => Promise(moduleKey)"
> This is slightly redundant with the code, but I think it makes these hook points explicitly clear.

Agreed. The name "resolve" is slightly vague. Adding these notes makes the code more readable.
When completing the prototype, we have the chance to rename these APIs more descriptive ones :)

[Yusuke Suzuki]

Committed r188894: <http://trac.webkit.org/changeset/188894>