Changeset 238391 in webkit


Ignore:
Timestamp:
Nov 19, 2018 11:09:53 PM (5 years ago)
Author:
mark.lam@apple.com
Message:

globalFuncImportModule() should return a promise when it clears exceptions.
​https://bugs.webkit.org/show_bug.cgi?id=191792
<rdar://problem/46090763>

Reviewed by Michael Saboff.

JSTests:

  • stress/global-import-function-should-return-a-promise-when-clearing-exceptions.js: Added.

Source/JavaScriptCore:

If we're clearing the exceptions in a CatchScope, then it means that we've handled
the exception, and is able to proceed in a normal manner. Hence, we should not
return the empty JSValue in this case: instead, we should return a Promise as
expected by import's API.

The only time when we can't return a promise is when we fail to create a Promise.
In that case, we should be propagating the exception.

Hence, globalFuncImportModule() contains a ThrowScope (for propagating the
exception that arises from failure to create the Promise) wrapping a CatchScope
(for catching any exception that arises from failure to execute the import).

Also fixed similar issues, and some exception check issues in JSModuleLoader and
the jsc shell.

  • jsc.cpp:

(GlobalObject::moduleLoaderImportModule):
(GlobalObject::moduleLoaderFetch):

  • runtime/JSGlobalObjectFunctions.cpp:

(JSC::globalFuncImportModule):

  • runtime/JSModuleLoader.cpp:

(JSC::JSModuleLoader::loadAndEvaluateModule):
(JSC::JSModuleLoader::loadModule):
(JSC::JSModuleLoader::requestImportModule):
(JSC::JSModuleLoader::importModule):
(JSC::JSModuleLoader::resolve):
(JSC::JSModuleLoader::fetch):
(JSC::moduleLoaderParseModule):
(JSC::moduleLoaderResolveSync):

Location:
trunk
Files:
1 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r238373 r238391  
     12018-11-19  Mark Lam  <mark.lam@apple.com>
     2
     3        globalFuncImportModule() should return a promise when it clears exceptions.
     4        https://bugs.webkit.org/show_bug.cgi?id=191792
     5        <rdar://problem/46090763>
     6
     7        Reviewed by Michael Saboff.
     8
     9        * stress/global-import-function-should-return-a-promise-when-clearing-exceptions.js: Added.
     10
    1112018-11-19  Guillaume Emont  <guijemont@igalia.com>
    212

  • trunk/Source/JavaScriptCore/ChangeLog

    r238388 r238391  
     12018-11-19  Mark Lam  <mark.lam@apple.com>
     2
     3        globalFuncImportModule() should return a promise when it clears exceptions.
     4        https://bugs.webkit.org/show_bug.cgi?id=191792
     5        <rdar://problem/46090763>
     6
     7        Reviewed by Michael Saboff.
     8
     9        If we're clearing the exceptions in a CatchScope, then it means that we've handled
     10        the exception, and is able to proceed in a normal manner.  Hence, we should not
     11        return the empty JSValue in this case: instead, we should return a Promise as
     12        expected by import's API.
     13
     14        The only time when we can't return a promise is when we fail to create a Promise.
     15        In that case, we should be propagating the exception.
     16
     17        Hence, globalFuncImportModule() contains a ThrowScope (for propagating the
     18        exception that arises from failure to create the Promise) wrapping a CatchScope
     19        (for catching any exception that arises from failure to execute the import).
     20
     21        Also fixed similar issues, and some exception check issues in JSModuleLoader and
     22        the jsc shell.
     23
     24        * jsc.cpp:
     25        (GlobalObject::moduleLoaderImportModule):
     26        (GlobalObject::moduleLoaderFetch):
     27        * runtime/JSGlobalObjectFunctions.cpp:
     28        (JSC::globalFuncImportModule):
     29        * runtime/JSModuleLoader.cpp:
     30        (JSC::JSModuleLoader::loadAndEvaluateModule):
     31        (JSC::JSModuleLoader::loadModule):
     32        (JSC::JSModuleLoader::requestImportModule):
     33        (JSC::JSModuleLoader::importModule):
     34        (JSC::JSModuleLoader::resolve):
     35        (JSC::JSModuleLoader::fetch):
     36        (JSC::moduleLoaderParseModule):
     37        (JSC::moduleLoaderResolveSync):
     38
    1392018-11-19  Alex Christensen  <achristensen@webkit.org>
    240

  • trunk/Source/JavaScriptCore/jsc.cpp

    r237823 r238391  
    802802{
    803803    VM& vm = globalObject->vm();
    804     auto scope = DECLARE_CATCH_SCOPE(vm);
    805 
    806     auto rejectPromise = [&] (JSValue error) {
    807         return JSInternalPromiseDeferred::create(exec, globalObject)->reject(exec, error);
     804    auto throwScope = DECLARE_THROW_SCOPE(vm);
     805
     806    auto* deferred = JSInternalPromiseDeferred::create(exec, globalObject);
     807    RETURN_IF_EXCEPTION(throwScope, nullptr);
     808
     809    auto catchScope = DECLARE_CATCH_SCOPE(vm);
     810    auto reject = [&] (JSValue rejectionReason) {
     811        catchScope.clearException();
     812        auto result = deferred->reject(exec, rejectionReason);
     813        catchScope.clearException();
     814        return result;
    808815    };
    809816
    810817    if (sourceOrigin.isNull())
    811         return rejectPromise(createError(exec, "Could not resolve the module specifier."_s));
     818        return reject(createError(exec, "Could not resolve the module specifier."_s));
    812819
    813820    auto referrer = sourceOrigin.string();
    814821    auto moduleName = moduleNameValue->value(exec);
    815     if (UNLIKELY(scope.exception())) {
    816         JSValue exception = scope.exception();
    817         scope.clearException();
    818         return rejectPromise(exception);
    819     }
     822    if (UNLIKELY(catchScope.exception()))
     823        return reject(catchScope.exception());
    820824
    821825    auto directoryName = extractDirectoryName(referrer.impl());
    822826    if (!directoryName)
    823         return rejectPromise(createError(exec, makeString("Could not resolve the referrer name '", String(referrer.impl()), "'.")));
     827        return reject(createError(exec, makeString("Could not resolve the referrer name '", String(referrer.impl()), "'.")));
    824828
    825829    auto result = JSC::importModule(exec, Identifier::fromString(&vm, resolvePath(directoryName.value(), ModuleName(moduleName))), parameters, jsUndefined());
    826     scope.releaseAssertNoException();
     830    if (UNLIKELY(catchScope.exception()))
     831        return reject(catchScope.exception());
    827832    return result;
    828833}
    … …  
    992997{
    993998    VM& vm = globalObject->vm();
    994     auto scope = DECLARE_CATCH_SCOPE(vm);
     999    auto throwScope = DECLARE_THROW_SCOPE(vm);
    9951000    JSInternalPromiseDeferred* deferred = JSInternalPromiseDeferred::create(exec, globalObject);
     1001    RETURN_IF_EXCEPTION(throwScope, nullptr);
     1002
     1003    auto catchScope = DECLARE_CATCH_SCOPE(vm);
     1004    auto reject = [&] (JSValue rejectionReason) {
     1005        catchScope.clearException();
     1006        auto result = deferred->reject(exec, rejectionReason);
     1007        catchScope.clearException();
     1008        return result;
     1009    };
     1010
    9961011    String moduleKey = key.toWTFString(exec);
    997     if (UNLIKELY(scope.exception())) {
    998         JSValue exception = scope.exception();
    999         scope.clearException();
    1000         return deferred->reject(exec, exception);
    1001     }
     1012    if (UNLIKELY(catchScope.exception()))
     1013        return reject(catchScope.exception());
    10021014
    10031015    // Here, now we consider moduleKey as the fileName.
    10041016    Vector<uint8_t> buffer;
    1005     if (!fetchModuleFromLocalFileSystem(moduleKey, buffer)) {
    1006         auto result = deferred->reject(exec, createError(exec, makeString("Could not open file '", moduleKey, "'.")));
    1007         scope.releaseAssertNoException();
    1008         return result;
    1009     }
     1017    if (!fetchModuleFromLocalFileSystem(moduleKey, buffer))
     1018        return reject(createError(exec, makeString("Could not open file '", moduleKey, "'.")));
    10101019
    10111020#if ENABLE(WEBASSEMBLY)
    … …  
    10131022    if (buffer.size() >= 4) {
    10141023        if (buffer[0] == '\0' && buffer[1] == 'a' && buffer[2] == 's' && buffer[3] == 'm') {
    1015             auto result = deferred->resolve(exec, JSSourceCode::create(vm, SourceCode(WebAssemblySourceProvider::create(WTFMove(buffer), SourceOrigin { moduleKey }, moduleKey))));
    1016             scope.releaseAssertNoException();
     1024            auto source = SourceCode(WebAssemblySourceProvider::create(WTFMove(buffer), SourceOrigin { moduleKey }, moduleKey));
     1025            catchScope.releaseAssertNoException();
     1026            auto sourceCode = JSSourceCode::create(vm, WTFMove(source));
     1027            catchScope.releaseAssertNoException();
     1028            auto result = deferred->resolve(exec, sourceCode);
     1029            catchScope.clearException();
    10171030            return result;
    10181031        }
    … …  
    10201033#endif
    10211034
    1022     auto result = deferred->resolve(exec, JSSourceCode::create(vm, makeSource(stringFromUTF(buffer), SourceOrigin { moduleKey }, moduleKey, TextPosition(), SourceProviderSourceType::Module)));
    1023     scope.releaseAssertNoException();
     1035    auto sourceCode = JSSourceCode::create(vm, makeSource(stringFromUTF(buffer), SourceOrigin { moduleKey }, moduleKey, TextPosition(), SourceProviderSourceType::Module));
     1036    catchScope.releaseAssertNoException();
     1037    auto result = deferred->resolve(exec, sourceCode);
     1038    catchScope.clearException();
    10241039    return result;
    10251040}

  • trunk/Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp

    r237714 r238391  
    784784{
    785785    VM& vm = exec->vm();
     786    auto throwScope = DECLARE_THROW_SCOPE(vm);
     787
     788    auto* globalObject = exec->lexicalGlobalObject();
     789
     790    auto* promise = JSPromiseDeferred::create(exec, globalObject);
     791    RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
     792
    786793    auto catchScope = DECLARE_CATCH_SCOPE(vm);
    787 
    788     auto* globalObject = exec->lexicalGlobalObject();
    789 
    790     auto* promise = JSPromiseDeferred::create(exec, globalObject);
    791     CLEAR_AND_RETURN_IF_EXCEPTION(catchScope, encodedJSValue());
     794    auto reject = [&] (JSValue rejectionReason) {
     795        catchScope.clearException();
     796        promise->reject(exec, rejectionReason);
     797        catchScope.clearException();
     798        return JSValue::encode(promise->promise());
     799    };
    792800
    793801    auto sourceOrigin = exec->callerSourceOrigin();
    794802    RELEASE_ASSERT(exec->argumentCount() == 1);
    795803    auto* specifier = exec->uncheckedArgument(0).toString(exec);
    796     if (Exception* exception = catchScope.exception()) {
    797         catchScope.clearException();
    798         promise->reject(exec, exception->value());
    799         CLEAR_AND_RETURN_IF_EXCEPTION(catchScope, encodedJSValue());
    800         return JSValue::encode(promise->promise());
    801     }
     804    if (Exception* exception = catchScope.exception())
     805        return reject(exception->value());
    802806
    803807    // We always specify parameters as undefined. Once dynamic import() starts accepting fetching parameters,
    … …  
    805809    JSValue parameters = jsUndefined();
    806810    auto* internalPromise = globalObject->moduleLoader()->importModule(exec, specifier, parameters, sourceOrigin);
    807     if (Exception* exception = catchScope.exception()) {
    808         catchScope.clearException();
    809         promise->reject(exec, exception->value());
    810         CLEAR_AND_RETURN_IF_EXCEPTION(catchScope, encodedJSValue());
    811         return JSValue::encode(promise->promise());
    812     }
     811    if (Exception* exception = catchScope.exception())
     812        return reject(exception->value());
    813813    promise->resolve(exec, internalPromise);
    814     CLEAR_AND_RETURN_IF_EXCEPTION(catchScope, encodedJSValue());
    815 
     814
     815    catchScope.clearException();
    816816    return JSValue::encode(promise->promise());
    817817}

  • trunk/Source/JavaScriptCore/runtime/JSModuleLoader.cpp

    r236697 r238391  
    11/*
    2  * Copyright (C) 2015-2017 Apple Inc. All Rights Reserved.
     2 * Copyright (C) 2015-2018 Apple Inc. All Rights Reserved.
    33 * Copyright (C) 2016 Yusuke Suzuki <utatane.tea@gmail.com>.
    44 *
    … …  
    159159    ASSERT(!arguments.hasOverflowed());
    160160
    161     RELEASE_AND_RETURN(scope, jsCast<JSInternalPromise*>(call(exec, function, callType, callData, this, arguments)));
     161    JSValue promise = call(exec, function, callType, callData, this, arguments);
     162    RETURN_IF_EXCEPTION(scope, nullptr);
     163    return jsCast<JSInternalPromise*>(promise);
    162164}
    163165
    … …  
    179181    ASSERT(!arguments.hasOverflowed());
    180182
    181     RELEASE_AND_RETURN(scope, jsCast<JSInternalPromise*>(call(exec, function, callType, callData, this, arguments)));
     183    JSValue promise = call(exec, function, callType, callData, this, arguments);
     184    RETURN_IF_EXCEPTION(scope, nullptr);
     185    return jsCast<JSInternalPromise*>(promise);
    182186}
    183187
    … …  
    218222    ASSERT(!arguments.hasOverflowed());
    219223
    220     RELEASE_AND_RETURN(scope, jsCast<JSInternalPromise*>(call(exec, function, callType, callData, this, arguments)));
     224    JSValue promise = call(exec, function, callType, callData, this, arguments);
     225    RETURN_IF_EXCEPTION(scope, nullptr);
     226    return jsCast<JSInternalPromise*>(promise);
    221227}
    222228
    … …  
    228234    auto* globalObject = exec->lexicalGlobalObject();
    229235    VM& vm = globalObject->vm();
    230     auto scope = DECLARE_CATCH_SCOPE(vm);
     236    auto throwScope = DECLARE_THROW_SCOPE(vm);
    231237
    232238    if (globalObject->globalObjectMethodTable()->moduleLoaderImportModule)
    233         return globalObject->globalObjectMethodTable()->moduleLoaderImportModule(globalObject, exec, this, moduleName, parameters, referrer);
     239        RELEASE_AND_RETURN(throwScope, globalObject->globalObjectMethodTable()->moduleLoaderImportModule(globalObject, exec, this, moduleName, parameters, referrer));
    234240
    235241    auto* deferred = JSInternalPromiseDeferred::create(exec, globalObject);
     242    RETURN_IF_EXCEPTION(throwScope, nullptr);
     243
     244    auto catchScope = DECLARE_CATCH_SCOPE(vm);
    236245    auto moduleNameString = moduleName->value(exec);
    237     if (UNLIKELY(scope.exception())) {
    238         JSValue exception = scope.exception()->value();
    239         scope.clearException();
     246    if (UNLIKELY(catchScope.exception())) {
     247        JSValue exception = catchScope.exception()->value();
     248        catchScope.clearException();
    240249        deferred->reject(exec, exception);
     250        catchScope.clearException();
    241251        return deferred->promise();
    242252    }
    243253    deferred->reject(exec, createError(exec, makeString("Could not import the module '", moduleNameString, "'.")));
     254    catchScope.clearException();
    244255    return deferred->promise();
    245256}
    … …  
    259270{
    260271    VM& vm = exec->vm();
    261     auto scope = DECLARE_CATCH_SCOPE(vm);
     272    auto throwScope = DECLARE_THROW_SCOPE(vm);
    262273
    263274    JSInternalPromiseDeferred* deferred = JSInternalPromiseDeferred::create(exec, exec->lexicalGlobalObject());
    264     scope.releaseAssertNoException();
     275    RETURN_IF_EXCEPTION(throwScope, nullptr);
     276
     277    auto catchScope = DECLARE_CATCH_SCOPE(vm);
     278
    265279    const Identifier moduleKey = resolveSync(exec, name, referrer, scriptFetcher);
    266     if (UNLIKELY(scope.exception())) {
    267         JSValue exception = scope.exception();
    268         scope.clearException();
    269         return deferred->reject(exec, exception);
     280    if (UNLIKELY(catchScope.exception())) {
     281        JSValue exception = catchScope.exception();
     282        catchScope.clearException();
     283        auto result = deferred->reject(exec, exception);
     284        catchScope.clearException();
     285        return result;
    270286    }
    271287    auto result = deferred->resolve(exec, identifierToJSValue(vm, moduleKey));
    272     scope.releaseAssertNoException();
     288    catchScope.clearException();
    273289    return result;
    274290}
    … …  
    281297    JSGlobalObject* globalObject = exec->lexicalGlobalObject();
    282298    VM& vm = globalObject->vm();
    283     auto scope = DECLARE_CATCH_SCOPE(vm);
     299    auto throwScope = DECLARE_THROW_SCOPE(vm);
     300
    284301    if (globalObject->globalObjectMethodTable()->moduleLoaderFetch)
    285         return globalObject->globalObjectMethodTable()->moduleLoaderFetch(globalObject, exec, this, key, parameters, scriptFetcher);
     302        RELEASE_AND_RETURN(throwScope, globalObject->globalObjectMethodTable()->moduleLoaderFetch(globalObject, exec, this, key, parameters, scriptFetcher));
     303
    286304    JSInternalPromiseDeferred* deferred = JSInternalPromiseDeferred::create(exec, globalObject);
     305    RETURN_IF_EXCEPTION(throwScope, nullptr);
     306
     307    auto catchScope = DECLARE_CATCH_SCOPE(vm);
     308
    287309    String moduleKey = key.toWTFString(exec);
    288     if (UNLIKELY(scope.exception())) {
    289         JSValue exception = scope.exception()->value();
    290         scope.clearException();
     310    if (UNLIKELY(catchScope.exception())) {
     311        JSValue exception = catchScope.exception()->value();
     312        catchScope.clearException();
    291313        deferred->reject(exec, exception);
     314        catchScope.clearException();
    292315        return deferred->promise();
    293316    }
    294317    deferred->reject(exec, createError(exec, makeString("Could not open the module '", moduleKey, "'.")));
     318    catchScope.clearException();
    295319    return deferred->promise();
    296320}
    … …  
    337361{
    338362    VM& vm = exec->vm();
    339     auto scope = DECLARE_CATCH_SCOPE(vm);
     363    auto throwScope = DECLARE_THROW_SCOPE(vm);
    340364
    341365    JSInternalPromiseDeferred* deferred = JSInternalPromiseDeferred::create(exec, exec->lexicalGlobalObject());
    342     scope.releaseAssertNoException();
    343 
    344     auto reject = [&] {
    345         JSValue exception = scope.exception();
    346         scope.clearException();
    347         auto result = deferred->reject(exec, exception);
    348         scope.releaseAssertNoException();
     366    RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
     367
     368    auto catchScope = DECLARE_CATCH_SCOPE(vm);
     369    auto reject = [&] (JSValue rejectionReason) {
     370        catchScope.clearException();
     371        auto result = deferred->reject(exec, rejectionReason);
     372        catchScope.clearException();
    349373        return JSValue::encode(result);
    350374    };
    351375
    352376    const Identifier moduleKey = exec->argument(0).toPropertyKey(exec);
    353     if (UNLIKELY(scope.exception()))
    354         return reject();
     377    if (UNLIKELY(catchScope.exception()))
     378        return reject(catchScope.exception());
    355379
    356380    JSValue source = exec->argument(1);
    … …  
    369393        &vm, sourceCode, Identifier(), JSParserBuiltinMode::NotBuiltin,
    370394        JSParserStrictMode::Strict, JSParserScriptMode::Module, SourceParseMode::ModuleAnalyzeMode, SuperBinding::NotNeeded, error);
    371     if (error.isValid()) {
    372         auto result = deferred->reject(exec, error.toErrorObject(exec->lexicalGlobalObject(), sourceCode));
    373         scope.releaseAssertNoException();
    374         return JSValue::encode(result);
    375     }
     395    if (error.isValid())
     396        return reject(error.toErrorObject(exec->lexicalGlobalObject(), sourceCode));
    376397    ASSERT(moduleProgramNode);
    377398
    378399    ModuleAnalyzer moduleAnalyzer(exec, moduleKey, sourceCode, moduleProgramNode->varDeclarations(), moduleProgramNode->lexicalVariables());
    379     if (UNLIKELY(scope.exception()))
    380         return reject();
     400    if (UNLIKELY(catchScope.exception()))
     401        return reject(catchScope.exception());
    381402
    382403    auto result = deferred->resolve(exec, moduleAnalyzer.analyze(*moduleProgramNode));
    383     scope.releaseAssertNoException();
     404    catchScope.clearException();
    384405    return JSValue::encode(result);
    385406}
    … …  
    438459{
    439460    VM& vm = exec->vm();
    440     auto scope = DECLARE_CATCH_SCOPE(vm);
     461    auto scope = DECLARE_THROW_SCOPE(vm);
    441462
    442463    JSModuleLoader* loader = jsDynamicCast<JSModuleLoader*>(vm, exec->thisValue());
Note: See TracChangeset for help on using the changeset viewer.