Changeset 276106 in webkit

Timestamp:

Apr 15, 2021 7:06:31 PM (3 years ago)

Author:

mark.lam@apple.com

Message:

HashMapImpl::rehash() should use a version of jsMapHash that cannot throw.
​https://bugs.webkit.org/show_bug.cgi?id=224610
rdar://76698910

Reviewed by Yusuke Suzuki.

JSTests:

• stress/suppress-TerminationException-in-HashMapImpl-rehash.js: Added.

Source/JavaScriptCore:

For context, HashMapImpl::rehash()'s rehash operation relies on jsMapHash().
jsMapHash() can be interrupted by a TerminationException, and as a result, may
not return the string hash we are expecting. This in turn can lead to the
rehash operation hashing with wrong keys.

However, all the keys should have already been hashed. Hence, rehash() should
never see an exception thrown there. We can avoid this complication with the
TerminationException by simply calling an alternate version of jsMapHash() that
is guaranteed to never throw e.g. a jsMapHashForAlreadyHashedValue() function.

• runtime/ExceptionHelpers.h:
• runtime/HashMapImplInlines.h:

(JSC::jsMapHashImpl):
(JSC::jsMapHash):
(JSC::jsMapHashForAlreadyHashedValue):
(JSC::HashMapImpl<HashMapBucketType>::rehash):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/suppress-TerminationException-in-HashMapImpl-rehash.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/ExceptionHelpers.h (modified) (1 diff)
• Source/JavaScriptCore/runtime/HashMapImplInlines.h (modified) (4 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2021-04-15  Mark Lam  <mark.lam@apple.com>
+
+        HashMapImpl::rehash() should use a version of jsMapHash that cannot throw.
+        https://bugs.webkit.org/show_bug.cgi?id=224610
+        rdar://76698910
+
+        Reviewed by Yusuke Suzuki.
+
+        * stress/suppress-TerminationException-in-HashMapImpl-rehash.js: Added.
+
 2021-04-14  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2021-04-15  Mark Lam  <mark.lam@apple.com>
+
+        HashMapImpl::rehash() should use a version of jsMapHash that cannot throw.
+        https://bugs.webkit.org/show_bug.cgi?id=224610
+        rdar://76698910
+
+        Reviewed by Yusuke Suzuki.
+
+        For context, HashMapImpl::rehash()'s rehash operation relies on jsMapHash().
+        jsMapHash() can be interrupted by a TerminationException, and as a result, may
+        not return the string hash we are expecting.  This in turn can lead to the
+        rehash operation hashing with wrong keys.
+
+        However, all the keys should have already been hashed.  Hence, rehash() should
+        never see an exception thrown there.  We can avoid this complication with the
+        TerminationException by simply calling an alternate version of jsMapHash() that
+        is guaranteed to never throw e.g. a jsMapHashForAlreadyHashedValue() function.
+
+        * runtime/ExceptionHelpers.h:
+        * runtime/HashMapImplInlines.h:
+        (JSC::jsMapHashImpl):
+        (JSC::jsMapHash):
+        (JSC::jsMapHashForAlreadyHashedValue):
+        (JSC::HashMapImpl<HashMapBucketType>::rehash):
+
 2021-04-14  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/runtime/ExceptionHelpers.h

@@ -36 +36 @@
 
 namespace JSC {
+
+enum class ExceptionExpectation {
+    CanThrow,
+    ShouldNotThrow
+};
 
 typedef JSObject* (*ErrorFactory)(JSGlobalObject*, const String&, ErrorInstance::SourceAppender);

trunk/Source/JavaScriptCore/runtime/HashMapImplInlines.h

@@ -85 +85 @@
 }
 
-ALWAYS_INLINE uint32_t jsMapHash(JSGlobalObject* globalObject, VM& vm, JSValue value)
+template<ExceptionExpectation expection>
+ALWAYS_INLINE uint32_t jsMapHashImpl(JSGlobalObject* globalObject, VM& vm, JSValue value)
 {
     ASSERT_WITH_MESSAGE(normalizeMapKey(value) == value, "We expect normalized values flowing into this function.");
@@ -92 +93 @@
         auto scope = DECLARE_THROW_SCOPE(vm);
         const String& wtfString = asString(value)->value(globalObject);
-        RETURN_IF_EXCEPTION(scope, UINT_MAX);
+        if constexpr (expection == ExceptionExpectation::CanThrow)
+            RETURN_IF_EXCEPTION(scope, UINT_MAX);
+        else
+            EXCEPTION_ASSERT(!scope.exception());
         return wtfString.impl()->hash();
     }
@@ -100 +104 @@
 
     return wangsInt64Hash(JSValue::encode(value));
+}
+
+ALWAYS_INLINE uint32_t jsMapHash(JSGlobalObject* globalObject, VM& vm, JSValue value)
+{
+    return jsMapHashImpl<ExceptionExpectation::CanThrow>(globalObject, vm, value);
+}
+
+ALWAYS_INLINE uint32_t jsMapHashForAlreadyHashedValue(JSGlobalObject* globalObject, VM& vm, JSValue value)
+{
+    return jsMapHashImpl<ExceptionExpectation::ShouldNotThrow>(globalObject, vm, value);
 }
 
@@ -417 +431 @@
     HashMapBucketType** buffer = this->buffer();
     while (iter != end) {
-        uint32_t index = jsMapHash(globalObject, vm, iter->key()) & mask;
+        uint32_t index = jsMapHashForAlreadyHashedValue(globalObject, vm, iter->key()) & mask;
         EXCEPTION_ASSERT_WITH_MESSAGE(!scope.exception(), "All keys should already be hashed before, so this should not throw because it won't resolve ropes.");
         {