Changeset 251456 in webkit

Timestamp:

Oct 22, 2019, 2:18:29 PM (5 years ago)

Author:

mark.lam@apple.com

Message:

Clients of JSArray::tryCreateUninitializedRestricted() should invoke the mutatorFence().
​https://bugs.webkit.org/show_bug.cgi?id=203231
<rdar://problem/56486552>

Reviewed by Saam Barati.

Clients of JSArray::tryCreateUninitializedRestricted() creates a partially
initialized JSArray butterfly, with the contract that it (the client) will take
care of filling in all the missing indexed properties before setting the newly
created array loose in the world. We intentionally do not unconditionally write
barrier the newly created array but, instead, rely on an owner object (or GC root)
that it gets put into to scan it.

That said, we do need to ensure that all the stores are completed before this
array is put in an owner object (or GC root) which makes it scannable by the GC.
This ensures that the GC will not be scanning a partially initialized array
butterfly. To achieve this, we should invoke the mutatorFence after the clients
of JSArray::tryCreateUninitializedRestricted() finish initializing the array.

By design, all clients of tryCreateUninitializedRestricted() must instantiate an
ObjectInitializationScope RAII object. This patch makes use of the
ObjectInitializationScope destructor to invoke the mutatorFence.

Note: we technically only need to invoke the fence if we succeeded in allocating
the array. However, we just invoke the fence unconditionally because we expect
that in the common path, we will succeed in allocating the array. The release
build version of ObjectInitializationScope does not keep record of whether we
succeed in allocating the array anyway. To keep the behavior consistent, the
debug build version of ObjectInitializationScope will also unconditionally
invoke the fence even if we failed to allocate the array.

This patch also does the following:

1. Replaced the setting of the public length in arrayProtoPrivateFuncConcatMemcpy() with an assertion. The public length was already set by tryCreateUninitializedRestricted() earlier.

Ditto for JSArray::fastSlice().

2. Removed a redundant instance of ObjectInitializationScope in createEmptyRegExpMatchesArray().

• runtime/ArrayPrototype.cpp:

(JSC::arrayProtoPrivateFuncConcatMemcpy):

• runtime/JSArray.cpp:

(JSC::JSArray::fastSlice):

• runtime/ObjectInitializationScope.cpp:

(JSC::ObjectInitializationScope::~ObjectInitializationScope):

• runtime/ObjectInitializationScope.h:

(JSC::ObjectInitializationScope::~ObjectInitializationScope):

• runtime/RegExpMatchesArray.cpp:

(JSC::createEmptyRegExpMatchesArray):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• runtime/ArrayPrototype.cpp (modified) (1 diff)
• runtime/JSArray.cpp (modified) (1 diff)
• runtime/ObjectInitializationScope.cpp (modified) (2 diffs)
• runtime/ObjectInitializationScope.h (modified) (2 diffs)
• runtime/RegExpMatchesArray.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-10-22  Mark Lam  <mark.lam@apple.com>
+
+        Clients of JSArray::tryCreateUninitializedRestricted() should invoke the mutatorFence().
+        https://bugs.webkit.org/show_bug.cgi?id=203231
+        <rdar://problem/56486552>
+
+        Reviewed by Saam Barati.
+
+        Clients of JSArray::tryCreateUninitializedRestricted() creates a partially
+        initialized JSArray butterfly, with the contract that it (the client) will take
+        care of filling in all the missing indexed properties before setting the newly
+        created array loose in the world.  We intentionally do not unconditionally write
+        barrier the newly created array but, instead, rely on an owner object (or GC root)
+        that it gets put into to scan it.
+
+        That said, we do need to ensure that all the stores are completed before this
+        array is put in an owner object (or GC root) which makes it scannable by the GC.
+        This ensures that the GC will not be scanning a partially initialized array
+        butterfly.  To achieve this, we should invoke the mutatorFence after the clients
+        of JSArray::tryCreateUninitializedRestricted() finish initializing the array.
+
+        By design, all clients of tryCreateUninitializedRestricted() must instantiate an
+        ObjectInitializationScope RAII object.  This patch makes use of the
+        ObjectInitializationScope destructor to invoke the mutatorFence.
+
+        Note: we technically only need to invoke the fence if we succeeded in allocating
+        the array.  However, we just invoke the fence unconditionally because we expect
+        that in the common path, we will succeed in allocating the array.  The release
+        build version of ObjectInitializationScope does not keep record of whether we
+        succeed in allocating the array anyway.  To keep the behavior consistent, the
+        debug build version of ObjectInitializationScope will also unconditionally
+        invoke the fence even if we failed to allocate the array.
+
+        This patch also does the following:
+
+        1. Replaced the setting of the public length in arrayProtoPrivateFuncConcatMemcpy()
+           with an assertion.  The public length was already set by
+           tryCreateUninitializedRestricted() earlier.
+
+           Ditto for JSArray::fastSlice().
+
+        2. Removed a redundant instance of ObjectInitializationScope in
+           createEmptyRegExpMatchesArray().
+
+        * runtime/ArrayPrototype.cpp:
+        (JSC::arrayProtoPrivateFuncConcatMemcpy):
+        * runtime/JSArray.cpp:
+        (JSC::JSArray::fastSlice):
+        * runtime/ObjectInitializationScope.cpp:
+        (JSC::ObjectInitializationScope::~ObjectInitializationScope):
+        * runtime/ObjectInitializationScope.h:
+        (JSC::ObjectInitializationScope::~ObjectInitializationScope):
+        * runtime/RegExpMatchesArray.cpp:
+        (JSC::createEmptyRegExpMatchesArray):
+
 2019-10-22  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/runtime/ArrayPrototype.cpp

@@ -1582 +1582 @@
     }
 
-    result->butterfly()->setPublicLength(resultSize);
+    ASSERT(result->butterfly()->publicLength() == resultSize);
     return JSValue::encode(result);
 }

trunk/Source/JavaScriptCore/runtime/JSArray.cpp

@@ -793 +793 @@
         else
             memcpy(resultButterfly.contiguous().data(), butterfly()->contiguous().data() + startIndex, sizeof(JSValue) * count);
-        resultButterfly.setPublicLength(count);
-
+
+        ASSERT(resultButterfly.publicLength() == count);
         return resultArray;
     }

trunk/Source/JavaScriptCore/runtime/ObjectInitializationScope.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2017-2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2017-2019 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -43 +43 @@
 ObjectInitializationScope::~ObjectInitializationScope()
 {
+    m_vm.heap.mutatorFence();
     if (!m_object)
         return;

trunk/Source/JavaScriptCore/runtime/ObjectInitializationScope.h

@@ -1 +1 @@
 /*
- * Copyright (C) 2017-2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2017-2019 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -41 +41 @@
         : m_vm(vm)
     { }
+    ALWAYS_INLINE ~ObjectInitializationScope()
+    {
+        m_vm.heap.mutatorFence();
+    }
 
     ALWAYS_INLINE VM& vm() const { return m_vm; }

trunk/Source/JavaScriptCore/runtime/RegExpMatchesArray.cpp

@@ -55 +55 @@
         }
     } else {
-        ObjectInitializationScope scope(vm);
         array = tryCreateUninitializedRegExpMatchesArray(scope, &deferralContext, globalObject->regExpMatchesArrayStructure(), regExp->numSubpatterns() + 1);
         RELEASE_ASSERT(array);