<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[210553] trunk/Source/JavaScriptCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/210553">210553</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2017-01-10 10:44:45 -0800 (Tue, 10 Jan 2017)</dd>
</dl>
<h3>Log Message</h3>
<pre>JSArray has some object scanning races
https://bugs.webkit.org/show_bug.cgi?id=166874
Reviewed by Mark Lam.
This fixes two separate bugs, both of which I detected by running
array-splice-contiguous.js in extreme anger:
1) Some of the paths of shifting and unshifting were not grabbing the internal cell
lock. This was causing the array storage scan to crash, even though it was well
synchronized (the scan does hold the lock). The fix is just to hold the lock anywhere
that memmoves the innards of the butterfly.
2) Out of line property scanning was synchronized using double collect snapshot. Array
storage scanning was synchronized using locks. But what if array storage
transformations messed up the out of line properties? It turns out that we actually
need to hoist the array storage scanner's locking up into the double collect
snapshot.
I don't know how to write a test that does any better of a job of catching this than
array-splice-contiguous.js.
* heap/DeferGC.h: Make DisallowGC usable even if NDEBUG.
* runtime/JSArray.cpp:
(JSC::JSArray::unshiftCountSlowCase):
(JSC::JSArray::shiftCountWithArrayStorage):
(JSC::JSArray::unshiftCountWithArrayStorage):
* runtime/JSObject.cpp:
(JSC::JSObject::visitButterflyImpl):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreheapDeferGCh">trunk/Source/JavaScriptCore/heap/DeferGC.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeJSArraycpp">trunk/Source/JavaScriptCore/runtime/JSArray.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeJSArrayh">trunk/Source/JavaScriptCore/runtime/JSArray.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeJSObjectcpp">trunk/Source/JavaScriptCore/runtime/JSObject.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (210552 => 210553)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2017-01-10 18:41:31 UTC (rev 210552)
+++ trunk/Source/JavaScriptCore/ChangeLog 2017-01-10 18:44:45 UTC (rev 210553)
</span><span class="lines">@@ -1,3 +1,35 @@
</span><ins>+2017-01-09 Filip Pizlo <fpizlo@apple.com>
+
+ JSArray has some object scanning races
+ https://bugs.webkit.org/show_bug.cgi?id=166874
+
+ Reviewed by Mark Lam.
+
+ This fixes two separate bugs, both of which I detected by running
+ array-splice-contiguous.js in extreme anger:
+
+ 1) Some of the paths of shifting and unshifting were not grabbing the internal cell
+ lock. This was causing the array storage scan to crash, even though it was well
+ synchronized (the scan does hold the lock). The fix is just to hold the lock anywhere
+ that memmoves the innards of the butterfly.
+
+ 2) Out of line property scanning was synchronized using double collect snapshot. Array
+ storage scanning was synchronized using locks. But what if array storage
+ transformations messed up the out of line properties? It turns out that we actually
+ need to hoist the array storage scanner's locking up into the double collect
+ snapshot.
+
+ I don't know how to write a test that does any better of a job of catching this than
+ array-splice-contiguous.js.
+
+ * heap/DeferGC.h: Make DisallowGC usable even if NDEBUG.
+ * runtime/JSArray.cpp:
+ (JSC::JSArray::unshiftCountSlowCase):
+ (JSC::JSArray::shiftCountWithArrayStorage):
+ (JSC::JSArray::unshiftCountWithArrayStorage):
+ * runtime/JSObject.cpp:
+ (JSC::JSObject::visitButterflyImpl):
+
</ins><span class="cx"> 2017-01-10 Andreas Kling <akling@apple.com>
</span><span class="cx">
</span><span class="cx"> Crash when GC heap grows way too large.
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreheapDeferGCh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/heap/DeferGC.h (210552 => 210553)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/heap/DeferGC.h 2017-01-10 18:41:31 UTC (rev 210552)
+++ trunk/Source/JavaScriptCore/heap/DeferGC.h 2017-01-10 18:44:45 UTC (rev 210553)
</span><span class="lines">@@ -1,5 +1,5 @@
</span><span class="cx"> /*
</span><del>- * Copyright (C) 2013 Apple Inc. All rights reserved.
</del><ins>+ * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
</ins><span class="cx"> *
</span><span class="cx"> * Redistribution and use in source and binary forms, with or without
</span><span class="cx"> * modification, are permitted provided that the following conditions
</span><span class="lines">@@ -67,31 +67,41 @@
</span><span class="cx"> Heap& m_heap;
</span><span class="cx"> };
</span><span class="cx">
</span><del>-#ifndef NDEBUG
</del><span class="cx"> class DisallowGC {
</span><span class="cx"> WTF_MAKE_NONCOPYABLE(DisallowGC);
</span><span class="cx"> public:
</span><span class="cx"> DisallowGC()
</span><span class="cx"> {
</span><ins>+#ifndef NDEBUG
</ins><span class="cx"> WTF::threadSpecificSet(s_isGCDisallowedOnCurrentThread, reinterpret_cast<void*>(true));
</span><ins>+#endif
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> ~DisallowGC()
</span><span class="cx"> {
</span><ins>+#ifndef NDEBUG
</ins><span class="cx"> WTF::threadSpecificSet(s_isGCDisallowedOnCurrentThread, reinterpret_cast<void*>(false));
</span><ins>+#endif
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> static bool isGCDisallowedOnCurrentThread()
</span><span class="cx"> {
</span><ins>+#ifndef NDEBUG
</ins><span class="cx"> return !!WTF::threadSpecificGet(s_isGCDisallowedOnCurrentThread);
</span><ins>+#else
+ return false;
+#endif
</ins><span class="cx"> }
</span><span class="cx"> static void initialize()
</span><span class="cx"> {
</span><ins>+#ifndef NDEBUG
</ins><span class="cx"> WTF::threadSpecificKeyCreate(&s_isGCDisallowedOnCurrentThread, 0);
</span><ins>+#endif
</ins><span class="cx"> }
</span><span class="cx">
</span><ins>+#ifndef NDEBUG
</ins><span class="cx"> JS_EXPORT_PRIVATE static WTF::ThreadSpecificKey s_isGCDisallowedOnCurrentThread;
</span><ins>+#endif
</ins><span class="cx"> };
</span><del>-#endif // NDEBUG
</del><span class="cx">
</span><span class="cx"> } // namespace JSC
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeJSArraycpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/JSArray.cpp (210552 => 210553)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/JSArray.cpp 2017-01-10 18:41:31 UTC (rev 210552)
+++ trunk/Source/JavaScriptCore/runtime/JSArray.cpp 2017-01-10 18:44:45 UTC (rev 210553)
</span><span class="lines">@@ -1,6 +1,6 @@
</span><span class="cx"> /*
</span><span class="cx"> * Copyright (C) 1999-2000 Harri Porten (porten@kde.org)
</span><del>- * Copyright (C) 2003, 2007-2009, 2012-2013, 2015-2016 Apple Inc. All rights reserved.
</del><ins>+ * Copyright (C) 2003-2017 Apple Inc. All rights reserved.
</ins><span class="cx"> * Copyright (C) 2003 Peter Kelly (pmk@post.com)
</span><span class="cx"> * Copyright (C) 2006 Alexey Proskuryakov (ap@nypop.com)
</span><span class="cx"> *
</span><span class="lines">@@ -298,7 +298,7 @@
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> // This method makes room in the vector, but leaves the new space for count slots uncleared.
</span><del>-bool JSArray::unshiftCountSlowCase(VM& vm, DeferGC&, bool addToFront, unsigned count)
</del><ins>+bool JSArray::unshiftCountSlowCase(const AbstractLocker&, VM& vm, DeferGC&, bool addToFront, unsigned count)
</ins><span class="cx"> {
</span><span class="cx"> ArrayStorage* storage = ensureArrayStorage(vm);
</span><span class="cx"> Butterfly* butterfly = storage->butterfly();
</span><span class="lines">@@ -892,6 +892,9 @@
</span><span class="cx"> if (startIndex >= vectorLength)
</span><span class="cx"> return true;
</span><span class="cx">
</span><ins>+ DisallowGC disallowGC;
+ auto locker = holdLock(*this);
+
</ins><span class="cx"> if (startIndex + count > vectorLength)
</span><span class="cx"> count = vectorLength - startIndex;
</span><span class="cx">
</span><span class="lines">@@ -930,7 +933,7 @@
</span><span class="cx"> // the start of the Butterfly, which needs to point at the first indexed property in the used
</span><span class="cx"> // portion of the vector.
</span><span class="cx"> Butterfly* butterfly = m_butterfly.get()->shift(structure(), count);
</span><del>- m_butterfly.setWithoutBarrier(butterfly);
</del><ins>+ setButterfly(vm, butterfly);
</ins><span class="cx"> storage = butterfly->arrayStorage();
</span><span class="cx"> storage->m_indexBias += count;
</span><span class="cx">
</span><span class="lines">@@ -1098,7 +1101,7 @@
</span><span class="cx"> setButterfly(vm, newButterfly);
</span><span class="cx"> } else if (!moveFront && vectorLength - length >= count)
</span><span class="cx"> storage = storage->butterfly()->arrayStorage();
</span><del>- else if (unshiftCountSlowCase(vm, deferGC, moveFront, count))
</del><ins>+ else if (unshiftCountSlowCase(locker, vm, deferGC, moveFront, count))
</ins><span class="cx"> storage = arrayStorage();
</span><span class="cx"> else {
</span><span class="cx"> throwOutOfMemoryError(exec, scope);
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeJSArrayh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/JSArray.h (210552 => 210553)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/JSArray.h 2017-01-10 18:41:31 UTC (rev 210552)
+++ trunk/Source/JavaScriptCore/runtime/JSArray.h 2017-01-10 18:44:45 UTC (rev 210553)
</span><span class="lines">@@ -1,6 +1,6 @@
</span><span class="cx"> /*
</span><span class="cx"> * Copyright (C) 1999-2000 Harri Porten (porten@kde.org)
</span><del>- * Copyright (C) 2003, 2007, 2008, 2009, 2012, 2015-2016 Apple Inc. All rights reserved.
</del><ins>+ * Copyright (C) 2003-2017 Apple Inc. All rights reserved.
</ins><span class="cx"> *
</span><span class="cx"> * This library is free software; you can redistribute it and/or
</span><span class="cx"> * modify it under the terms of the GNU Lesser General Public
</span><span class="lines">@@ -177,7 +177,7 @@
</span><span class="cx">
</span><span class="cx"> bool unshiftCountWithAnyIndexingType(ExecState*, unsigned startIndex, unsigned count);
</span><span class="cx"> bool unshiftCountWithArrayStorage(ExecState*, unsigned startIndex, unsigned count, ArrayStorage*);
</span><del>- bool unshiftCountSlowCase(VM&, DeferGC&, bool, unsigned);
</del><ins>+ bool unshiftCountSlowCase(const AbstractLocker&, VM&, DeferGC&, bool, unsigned);
</ins><span class="cx">
</span><span class="cx"> bool setLengthWithArrayStorage(ExecState*, unsigned newLength, bool throwException, ArrayStorage*);
</span><span class="cx"> void setLengthWritable(ExecState*, bool writable);
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeJSObjectcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/JSObject.cpp (210552 => 210553)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/JSObject.cpp 2017-01-10 18:41:31 UTC (rev 210552)
+++ trunk/Source/JavaScriptCore/runtime/JSObject.cpp 2017-01-10 18:44:45 UTC (rev 210553)
</span><span class="lines">@@ -383,6 +383,20 @@
</span><span class="cx"> return nullptr;
</span><span class="cx"> structure = vm.getStructure(structureID);
</span><span class="cx"> lastOffset = structure->lastOffset();
</span><ins>+ IndexingType indexingType = structure->indexingType();
+ Locker<JSCell> locker(NoLockingNecessary);
+ switch (indexingType) {
+ case ALL_CONTIGUOUS_INDEXING_TYPES:
+ case ALL_ARRAY_STORAGE_INDEXING_TYPES:
+ // We need to hold this lock to protect against changes to the innards of the butterfly
+ // that can happen when the butterfly is used for array storage. We conservatively
+ // assume that a contiguous butterfly may transform into an array storage one, though
+ // this is probably more conservative than necessary.
+ locker = Locker<JSCell>(*this);
+ break;
+ default:
+ break;
+ }
</ins><span class="cx"> WTF::loadLoadFence();
</span><span class="cx"> butterfly = this->butterfly();
</span><span class="cx"> if (!butterfly)
</span><span class="lines">@@ -395,27 +409,17 @@
</span><span class="cx">
</span><span class="cx"> markAuxiliaryAndVisitOutOfLineProperties(visitor, butterfly, structure, lastOffset);
</span><span class="cx">
</span><del>- IndexingType oldType = structure->indexingType();
- switch (oldType) {
</del><ins>+ ASSERT(indexingType == structure->indexingType());
+
+ switch (indexingType) {
</ins><span class="cx"> case ALL_CONTIGUOUS_INDEXING_TYPES:
</span><del>- case ALL_ARRAY_STORAGE_INDEXING_TYPES: {
- // This lock is here to protect Contiguous->ArrayStorage transitions, but we could make that
- // race work if we needed to.
- auto locker = holdLock(*this);
- IndexingType newType = this->indexingType();
- butterfly = this->butterfly();
- switch (newType) {
- case ALL_CONTIGUOUS_INDEXING_TYPES:
- visitor.appendValuesHidden(butterfly->contiguous().data(), butterfly->publicLength());
- break;
- default: // ALL_ARRAY_STORAGE_INDEXING_TYPES
- visitor.appendValuesHidden(butterfly->arrayStorage()->m_vector, butterfly->arrayStorage()->vectorLength());
- if (butterfly->arrayStorage()->m_sparseMap)
- visitor.append(butterfly->arrayStorage()->m_sparseMap);
- break;
- }
</del><ins>+ visitor.appendValuesHidden(butterfly->contiguous().data(), butterfly->publicLength());
</ins><span class="cx"> break;
</span><del>- }
</del><ins>+ case ALL_ARRAY_STORAGE_INDEXING_TYPES:
+ visitor.appendValuesHidden(butterfly->arrayStorage()->m_vector, butterfly->arrayStorage()->vectorLength());
+ if (butterfly->arrayStorage()->m_sparseMap)
+ visitor.append(butterfly->arrayStorage()->m_sparseMap);
+ break;
</ins><span class="cx"> default:
</span><span class="cx"> break;
</span><span class="cx"> }
</span></span></pre>
</div>
</div>
</body>
</html>