IndexedDB binding utilities miss exception checks
Created attachment 405265 [details] Patch
<rdar://problem/66152374>
Comment on attachment 405265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405265&action=review Can you check if the api-gtk bot failure is real?o > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:64 > + auto scope = DECLARE_CATCH_SCOPE(vm); Why is this a CatchScope? Is it because this is the outermost scope that should handle exceptions? > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:195 > + auto scope = DECLARE_CATCH_SCOPE(vm); Ditto: what’s the rationale here?
Comment on attachment 405265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405265&action=review >> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:64 >> + auto scope = DECLARE_CATCH_SCOPE(vm); > > Why is this a CatchScope? Is it because this is the outermost scope that should handle exceptions? After trying out your patch and investigating the issue, it looks like the expected behavior is to propagate the exception to the caller. That would mean that we should be using a ThrowScope here instead of a CatchScope. So, I tried that and found that IDBKeyRange::bound() needs some changes in order to placate the exception check validator. I understand that we may not want to add explicit exception checks there because it is redundant to the isValid() checks. We can address that using an exception. After I applied the following patch (in addition to yours), the 2 DECLARE_CATCH_SCOPEs can now be DECLARE_THROW_SCOPEs. Can you take another look? Thanks. Index: Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp =================================================================== --- Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp (revision 264901) +++ Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp (working copy) @@ -1,5 +1,6 @@ /* * Copyright (C) 2010 Google Inc. All rights reserved. + * Copyright (C) 2020 Apple Inc. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -32,7 +33,9 @@ #include "IDBKey.h" #include "IDBKeyData.h" #include "ScriptExecutionContext.h" +#include <JavaScriptCore/JSGlobalObject.h> #include <JavaScriptCore/JSCJSValue.h> +#include <JavaScriptCore/ThrowScope.h> #include <wtf/IsoMallocInlines.h> namespace WebCore { @@ -94,10 +97,14 @@ ExceptionOr<Ref<IDBKeyRange>> IDBKeyRange::bound(JSGlobalObject& state, JSValue lowerValue, JSValue upperValue, bool lowerOpen, bool upperOpen) { + VM& vm = state.vm(); + auto scope = DECLARE_THROW_SCOPE(vm); auto lower = scriptValueToIDBKey(state, lowerValue); + EXCEPTION_ASSERT_UNUSED(scope, !!scope.exception() == !lower->isValid()); if (!lower->isValid()) return Exception { DataError }; auto upper = scriptValueToIDBKey(state, upperValue); + EXCEPTION_ASSERT(!!scope.exception() == !lower->isValid()); if (!upper->isValid()) return Exception { DataError }; if (upper->isLessThan(lower.get()))
Comment on attachment 405265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405265&action=review >>> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:64 >>> + auto scope = DECLARE_CATCH_SCOPE(vm); >> >> Why is this a CatchScope? Is it because this is the outermost scope that should handle exceptions? > > After trying out your patch and investigating the issue, it looks like the expected behavior is to propagate the exception to the caller. That would mean that we should be using a ThrowScope here instead of a CatchScope. So, I tried that and found that IDBKeyRange::bound() needs some changes in order to placate the exception check validator. I understand that we may not want to add explicit exception checks there because it is redundant to the isValid() checks. We can address that using an exception. After I applied the following patch (in addition to yours), the 2 DECLARE_CATCH_SCOPEs can now be DECLARE_THROW_SCOPEs. Can you take another look? Thanks. > > Index: Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp > =================================================================== > --- Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp (revision 264901) > +++ Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp (working copy) > @@ -1,5 +1,6 @@ > /* > * Copyright (C) 2010 Google Inc. All rights reserved. > + * Copyright (C) 2020 Apple Inc. All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions > @@ -32,7 +33,9 @@ > #include "IDBKey.h" > #include "IDBKeyData.h" > #include "ScriptExecutionContext.h" > +#include <JavaScriptCore/JSGlobalObject.h> > #include <JavaScriptCore/JSCJSValue.h> > +#include <JavaScriptCore/ThrowScope.h> > #include <wtf/IsoMallocInlines.h> > > namespace WebCore { > @@ -94,10 +97,14 @@ > > ExceptionOr<Ref<IDBKeyRange>> IDBKeyRange::bound(JSGlobalObject& state, JSValue lowerValue, JSValue upperValue, bool lowerOpen, bool upperOpen) > { > + VM& vm = state.vm(); > + auto scope = DECLARE_THROW_SCOPE(vm); > auto lower = scriptValueToIDBKey(state, lowerValue); > + EXCEPTION_ASSERT_UNUSED(scope, !!scope.exception() == !lower->isValid()); > if (!lower->isValid()) > return Exception { DataError }; > auto upper = scriptValueToIDBKey(state, upperValue); > + EXCEPTION_ASSERT(!!scope.exception() == !lower->isValid()); > if (!upper->isValid()) > return Exception { DataError }; > if (upper->isLessThan(lower.get())) OK, I've revised and updated.
Created attachment 405326 [details] Patch
Created attachment 405328 [details] Patch
Comment on attachment 405328 [details] Patch r=me
Comment on attachment 405328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405328&action=review > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:78 > + EXCEPTION_ASSERT_UNUSED(scope, !!scope.exception() == !key->isValid()); These assertions should be `!scope.exception() || !key->isValid()` since key can be invalid even if JS exception is not thrown.
Created attachment 405336 [details] Patch
(In reply to Yusuke Suzuki from comment #9) > Comment on attachment 405328 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405328&action=review > > > Source/WebCore/Modules/indexeddb/IDBKeyRange.cpp:78 > > + EXCEPTION_ASSERT_UNUSED(scope, !!scope.exception() == !key->isValid()); > > These assertions should be `!scope.exception() || !key->isValid()` since key > can be invalid even if JS exception is not thrown. Good point!
Committed r264988: <https://trac.webkit.org/changeset/264988> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405336 [details].