Bug 179826

Created attachment 327190 [details] patch

<rdar://problem/35455813>

Created attachment 327191 [details] patch Missing radar in ChangeLog.

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.

Created attachment 327201 [details] patch Updated as discussed offline with mlam.

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().

(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()); }

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.

Created attachment 327220 [details] patch

The commit-queue encountered the following flaky tests while processing attachment 327220 [details]: http/tests/xmlhttprequest/redirect-cross-origin-tripmine.html bug 179840 (authors: ap@webkit.org and rniwa@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 327220 [details] patch Clearing flags on attachment: 327220 Committed r224985: <https://trac.webkit.org/changeset/224985>

All reviewed patches have been landed. Closing bug.