Bug 148173

Summary: [ES6] Return JSInternalPromise as result of evaluateModule
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ggaren, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 148262    
Bug Blocks: 147340    
Attachments:
Description Flags
Patch
none
Patch sbarati: review+

Description Yusuke Suzuki 2015-08-19 00:43:08 PDT
Report asynchronous module loader errors
Comment 1 Yusuke Suzuki 2015-08-20 16:18:32 PDT
While implementing this, I've found one memory leak error in the JSStdFunction...
I'll create the separate patch to fix that.
Comment 2 Yusuke Suzuki 2015-08-20 23:48:01 PDT
Created attachment 259587 [details]
Patch
Comment 3 Yusuke Suzuki 2015-08-20 23:56:12 PDT
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!
Comment 4 Yusuke Suzuki 2015-08-20 23:59:41 PDT
Created attachment 259588 [details]
Patch

add ifdef guard for promise = ...
Comment 5 Yusuke Suzuki 2015-08-21 00:10:13 PDT
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]
Comment 6 Saam Barati 2015-08-24 16:39:03 PDT
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.
Comment 7 Yusuke Suzuki 2015-08-24 16:44:49 PDT
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 :)
Comment 8 Yusuke Suzuki 2015-08-24 16:49:08 PDT
Committed r188894: <http://trac.webkit.org/changeset/188894>