Attachment 342731 Details for Bug 186602 – Patch

[patch] Patch

bug-186602-20180614203514.patch (text/plain), 9.48 KB, created by Yusuke Suzuki on 2018-06-14 04:35:15 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2018-06-14 04:35:15 PDT

Size: 9.48 KB

patch

obsolete

>Subversion Revision: 232833
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index a4ea1353632f5792b8f065d7d7b1c63fbc22ec8d..7fb202e32dda01ec27250058db1397c784beed72 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,17 @@
>+2018-06-14  Yusuke Suzuki  <utatane.tea@gmail.com>
>+
>+        [JSC] Remove cellLock in JSObject::convertContiguousToArrayStorage
>+        https://bugs.webkit.org/show_bug.cgi?id=186602
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        JSObject::convertContiguousToArrayStorage's cellLock() is not necessary since we do not
>+        change the part of the butterfly, length etc. We prove that our procedure is safe, and
>+        drop the cellLock() here.
>+
>+        * runtime/JSObject.cpp:
>+        (JSC::JSObject::convertContiguousToArrayStorage):
>+
> 2018-06-14  Carlos Garcia Campos  <cgarcia@igalia.com>
> 
>         [GTK][WPE] WebDriver: handle acceptInsecureCertificates capability
>diff --git a/Source/JavaScriptCore/runtime/JSObject.cpp b/Source/JavaScriptCore/runtime/JSObject.cpp
>index 696a146ecb5c9d29dda63e730c24a4eef1a242bc..846dfde8ff113711f167e34352d3a6f866c59ba5 100644
>--- a/Source/JavaScriptCore/runtime/JSObject.cpp
>+++ b/Source/JavaScriptCore/runtime/JSObject.cpp
>@@ -1343,26 +1343,63 @@ ArrayStorage* JSObject::convertContiguousToArrayStorage(VM& vm, NonPropertyTrans
>         if (v)
>             newStorage->m_numValuesInVector++;
>     }
>-    
>+
>+    // While we modify the butterfly of Contiguous Array, we do not take any cellLock here. This is because
>+    // (1) the old butterfly is not changed and (2) new butterfly is not changed after it is exposed to
>+    // the collector.
>+    // Our following operations are sequentially executed by using storeStoreFence.
>+    //
>+    //     CreateNewButterfly NukeStructure ChangeButterfly PutNewStructure
>+    //
>+    // Meanwhile the collector performs the following steps sequentially:
>+    //
>+    //     ReadStructureEarly ReadButterfly ReadStructureLate
>+    //
>+    // We list up all the patterns by writing a tiny script, and ensure all the cases are categorized into BEFORE, AFTER, and IGNORE.
>+    //
>+    // CreateNewButterfly NukeStructure ChangeButterfly PutNewStructure ReadStructureEarly ReadButterfly ReadStructureLate: AFTER, trivially
>+    // CreateNewButterfly NukeStructure ChangeButterfly ReadStructureEarly PutNewStructure ReadButterfly ReadStructureLate: IGNORE, because nuked structure read early
>+    // CreateNewButterfly NukeStructure ChangeButterfly ReadStructureEarly ReadButterfly PutNewStructure ReadStructureLate: IGNORE, because nuked structure read early
>+    // CreateNewButterfly NukeStructure ChangeButterfly ReadStructureEarly ReadButterfly ReadStructureLate PutNewStructure: IGNORE, because nuked structure read early
>+    // CreateNewButterfly NukeStructure ReadStructureEarly ChangeButterfly PutNewStructure ReadButterfly ReadStructureLate: IGNORE, because nuked structure read early
>+    // CreateNewButterfly NukeStructure ReadStructureEarly ChangeButterfly ReadButterfly PutNewStructure ReadStructureLate: IGNORE, because nuked structure read early
>+    // CreateNewButterfly NukeStructure ReadStructureEarly ChangeButterfly ReadButterfly ReadStructureLate PutNewStructure: IGNORE, because nuked structure read early
>+    // CreateNewButterfly NukeStructure ReadStructureEarly ReadButterfly ChangeButterfly PutNewStructure ReadStructureLate: IGNORE, because nuked structure read early
>+    // CreateNewButterfly NukeStructure ReadStructureEarly ReadButterfly ChangeButterfly ReadStructureLate PutNewStructure: IGNORE, because nuked structure read early
>+    // CreateNewButterfly NukeStructure ReadStructureEarly ReadButterfly ReadStructureLate ChangeButterfly PutNewStructure: IGNORE, because nuked structure read early
>+    // CreateNewButterfly ReadStructureEarly NukeStructure ChangeButterfly PutNewStructure ReadButterfly ReadStructureLate: IGNORE, because early and late structures don't match
>+    // CreateNewButterfly ReadStructureEarly NukeStructure ChangeButterfly ReadButterfly PutNewStructure ReadStructureLate: IGNORE, because early and late structures don't match
>+    // CreateNewButterfly ReadStructureEarly NukeStructure ChangeButterfly ReadButterfly ReadStructureLate PutNewStructure: IGNORE, because nuked structure read late
>+    // CreateNewButterfly ReadStructureEarly NukeStructure ReadButterfly ChangeButterfly PutNewStructure ReadStructureLate: IGNORE, because early and late structures don't match
>+    // CreateNewButterfly ReadStructureEarly NukeStructure ReadButterfly ChangeButterfly ReadStructureLate PutNewStructure: IGNORE, because nuked structure read late
>+    // CreateNewButterfly ReadStructureEarly NukeStructure ReadButterfly ReadStructureLate ChangeButterfly PutNewStructure: IGNORE, because nuked structure read late
>+    // CreateNewButterfly ReadStructureEarly ReadButterfly NukeStructure ChangeButterfly PutNewStructure ReadStructureLate: IGNORE, because early and late structures don't match
>+    // CreateNewButterfly ReadStructureEarly ReadButterfly NukeStructure ChangeButterfly ReadStructureLate PutNewStructure: IGNORE, because nuked structure read late
>+    // CreateNewButterfly ReadStructureEarly ReadButterfly NukeStructure ReadStructureLate ChangeButterfly PutNewStructure: IGNORE, because nuked structure read late
>+    // CreateNewButterfly ReadStructureEarly ReadButterfly ReadStructureLate NukeStructure ChangeButterfly PutNewStructure: BEFORE, trivially.
>+    // ReadStructureEarly CreateNewButterfly NukeStructure ChangeButterfly PutNewStructure ReadButterfly ReadStructureLate: IGNORE, because early and late structures don't match
>+    // ReadStructureEarly CreateNewButterfly NukeStructure ChangeButterfly ReadButterfly PutNewStructure ReadStructureLate: IGNORE, because early and late structures don't match
>+    // ReadStructureEarly CreateNewButterfly NukeStructure ChangeButterfly ReadButterfly ReadStructureLate PutNewStructure: IGNORE, because nuked structure read late
>+    // ReadStructureEarly CreateNewButterfly NukeStructure ReadButterfly ChangeButterfly PutNewStructure ReadStructureLate: IGNORE, because early and late structures don't match
>+    // ReadStructureEarly CreateNewButterfly NukeStructure ReadButterfly ChangeButterfly ReadStructureLate PutNewStructure: IGNORE, because nuked structure read late
>+    // ReadStructureEarly CreateNewButterfly NukeStructure ReadButterfly ReadStructureLate ChangeButterfly PutNewStructure: IGNORE, because nuked structure read late
>+    // ReadStructureEarly CreateNewButterfly ReadButterfly NukeStructure ChangeButterfly PutNewStructure ReadStructureLate: IGNORE, because early and late structures don't match
>+    // ReadStructureEarly CreateNewButterfly ReadButterfly NukeStructure ChangeButterfly ReadStructureLate PutNewStructure: IGNORE, because nuked structure read late
>+    // ReadStructureEarly CreateNewButterfly ReadButterfly NukeStructure ReadStructureLate ChangeButterfly PutNewStructure: IGNORE, because nuked structure read late
>+    // ReadStructureEarly CreateNewButterfly ReadButterfly ReadStructureLate NukeStructure ChangeButterfly PutNewStructure: BEFORE, CreateNewButterfly is not visible to collector.
>+    // ReadStructureEarly ReadButterfly CreateNewButterfly NukeStructure ChangeButterfly PutNewStructure ReadStructureLate: IGNORE, because early and late structures don't match
>+    // ReadStructureEarly ReadButterfly CreateNewButterfly NukeStructure ChangeButterfly ReadStructureLate PutNewStructure: IGNORE, because nuked structure read late
>+    // ReadStructureEarly ReadButterfly CreateNewButterfly NukeStructure ReadStructureLate ChangeButterfly PutNewStructure: IGNORE, because nuked structure read late
>+    // ReadStructureEarly ReadButterfly CreateNewButterfly ReadStructureLate NukeStructure ChangeButterfly PutNewStructure: BEFORE, CreateNewButterfly is not visible to collector.
>+    // ReadStructureEarly ReadButterfly ReadStructureLate CreateNewButterfly NukeStructure ChangeButterfly PutNewStructure: BEFORE, trivially.
>+
>+    ASSERT(newStorage->butterfly() != butterfly);
>     StructureID oldStructureID = this->structureID();
>     Structure* oldStructure = vm.getStructure(oldStructureID);
>     Structure* newStructure = Structure::nonPropertyTransition(vm, oldStructure, transition);
> 
>-    // This has a crazy race with the garbage collector. When changing the butterfly and structure,
>-    // the mutator always sets the structure last. The collector will always read the structure
>-    // first. We probably have to follow that convention here as well. This means that the collector
>-    // will sometimes see the new butterfly (the one with ArrayStorage) with the only structure (the
>-    // one that says that the butterfly is Contiguous). When scanning Contiguous, the collector will
>-    // mark word at addresses greater than or equal to the butterfly pointer, up to the publicLength
>-    // in the butterfly. But an ArrayStorage has some non-pointer header data at low positive
>-    // offsets from the butterfly - so when this race happens, the collector will surely crash
>-    // because it will fail to decode two consecutive int32s as if it was a JSValue.
>-    //
>-    // Fortunately, we have the JSCell lock for this purpose!
>-
>-    Locker<JSCellLock> locker(NoLockingNecessary);
>-    if (vm.heap.mutatorShouldBeFenced())
>-        locker = holdLock(cellLock());
>+    // Ensure new Butterfly initialization is correctly done before exposing it to the concurrent threads.
>+    WTF::storeStoreFence();
>     nukeStructureAndSetButterfly(vm, oldStructureID, newStorage->butterfly());
>     setStructure(vm, newStructure);
>

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 186602: 342729 | 342731