Bug 179826 – WebAssembly JS API: throw when a promise can't be created

• Component: WebAssembly
• Status: RESOLVED FIXED

[JF Bastien]

Failure *in* a promise causes rejection, but failure to create a promise (because of stack overflow) isn't really spec'd (as all stack things JS). This applies to WebAssembly.compile and WebAssembly.instantiate.

Dan's current proposal says:

  https://littledan.github.io/spec/document/js-api/index.html#stack-overflow

  Whenever a stack overflow occurs in WebAssembly code, the same class of exception is thrown as for a stack overflow in JavaScript. The particular exception here is implementation-defined in both cases.
  Note: ECMAScript doesn’t specify any sort of behavior on stack overflow; implementations have been observed to throw RangeError, InternalError or Error. Any is valid here.

This is for general stack overflow within WebAssembly, not specifically for promise creation within JavaScript, but it seems like a stack overflow in promise creation should follow the same rule instead of, say, swallowing the overflow and returning undefined.

[JF Bastien]

Created attachment 327190 [details]
patch

[JF Bastien]

<rdar://problem/35455813>

[JF Bastien]

Created attachment 327191 [details]
patch

Missing radar in ChangeLog.

[Mark Lam]

Comment on attachment 327191 [details]
patch

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

I suspect once you've changed these to using ThrowScopes, some jsc tests will start failing (due to missing exception checks).  Please change these and retest.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:97
>      auto scope = DECLARE_CATCH_SCOPE(vm);
>      auto* globalObject = exec->lexicalGlobalObject();
>  
> -    JSPromiseDeferred* promise = JSPromiseDeferred::create(exec, globalObject);
> -    CLEAR_AND_RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +    JSPromiseDeferred* promise = createPromise(vm, exec, globalObject);
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());

This does not look right.  Being in a CatchScope means we should never return with an exception.  I think you'll want to change that DECLARE_CATCH_SCOPE to DECLARE_THROW_SCOPE.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:199
>      auto scope = DECLARE_CATCH_SCOPE(vm);
> -
> -    JSPromiseDeferred* promise = JSPromiseDeferred::create(exec, exec->lexicalGlobalObject());
> -    CLEAR_AND_RETURN_IF_EXCEPTION(scope, encodedJSValue());
> +    auto* globalObject = exec->lexicalGlobalObject();
> +    
> +    JSPromiseDeferred* promise = createPromise(vm, exec, globalObject);
> +    RETURN_IF_EXCEPTION(scope, encodedJSValue());

Ditto.  You should be using a DECLARE_THROW_SCOPE here.

[JF Bastien]

Created attachment 327201 [details]
patch

Updated as discussed offline with mlam.

[Mark Lam]

Comment on attachment 327201 [details]
patch

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

From the missing clearException()s in this patch, I realized that the CatchScope destructor is missing an ASSERT(!vm.exception()).  If that causes more failures to manifest, you can file a bug to add that assert and fix those assertion failures later.  It also looks like we may also be missing some test cases:
1. we need one that throws an exception and trigger the rejection immediately (which should fail that assertion in the CatchScope destructor, if not earlier in the reject function).
2. we need one that throws an exception in the reject callback (which should also fail that assertion in the CatchScope destructor).

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:101
> +        if (UNLIKELY(catchScope.exception()))
> +            reject(exec, catchScope, promise);

This only detects the exception, but does not clear it.  You should also clear the exception using catchScope.clearException() before calling the reject code.

Hmmm, that also makes me wonder what happens if the reject callback throws an exception?  If it does, I presume we should eat it too just like we do for async calls to the reject callback.  That means you'll also need to call catchScope.clearException() after the call to reject().

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:181
> +            if (UNLIKELY(scope.exception()))
>                  return reject(exec, scope, promise);

Ditto.  You'll need to clearException() before and after the reject function().

[JF Bastien]

(In reply to Mark Lam from comment #6)
> Comment on attachment 327201 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327201&action=review
> 
> From the missing clearException()s in this patch, I realized that the
> CatchScope destructor is missing an ASSERT(!vm.exception()).  If that causes
> more failures to manifest, you can file a bug to add that assert and fix
> those assertion failures later.

That fails miserably :(

Filed #179836.


> It also looks like we may also be missing
> some test cases:
> 1. we need one that throws an exception and trigger the rejection
> immediately (which should fail that assertion in the CatchScope destructor,
> if not earlier in the reject function).
> 2. we need one that throws an exception in the reject callback (which should
> also fail that assertion in the CatchScope destructor).
> 
> > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:101
> > +        if (UNLIKELY(catchScope.exception()))
> > +            reject(exec, catchScope, promise);
> 
> This only detects the exception, but does not clear it.  You should also
> clear the exception using catchScope.clearException() before calling the
> reject code.
> 
> Hmmm, that also makes me wonder what happens if the reject callback throws
> an exception?  If it does, I presume we should eat it too just like we do
> for async calls to the reject callback.  That means you'll also need to call
> catchScope.clearException() after the call to reject().
> 
> > Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:181
> > +            if (UNLIKELY(scope.exception()))
> >                  return reject(exec, scope, promise);
> 
> Ditto.  You'll need to clearException() before and after the reject
> function().

It's handled in the helper:

static void reject(ExecState* exec, CatchScope& catchScope, JSPromiseDeferred* promise)
{
    Exception* exception = catchScope.exception();
    ASSERT(exception);
    catchScope.clearException();
    promise->reject(exec, exception->value());
    CLEAR_AND_RETURN_IF_EXCEPTION(catchScope, void());
}

[Mark Lam]

Comment on attachment 327201 [details]
patch

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

r=me with the remaining fix.

> Source/JavaScriptCore/wasm/js/WebAssemblyPrototype.cpp:205
> +            RETURN_IF_EXCEPTION(catchScope, encodedJSValue());

This is still wrong.  For CatchScope, you should use clearException() here and just return JSValue::encode(promise->promise()) below.  The client of this function would expect to get the promise even if it's rejected, right?  In any case, returning encodedJSValue() means returning a nullptr, and this will cause crashes for when there are no exceptions.  You should never exit a CatchScope with a pending exception.

[JF Bastien]

Created attachment 327220 [details]
patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 327220 [details]:

http/tests/xmlhttprequest/redirect-cross-origin-tripmine.html bug 179840 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 327220 [details]
patch

Clearing flags on attachment: 327220

Committed r224985: <https://trac.webkit.org/changeset/224985>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.