Title: [230726] trunk/Source/_javascript_Core
- Revision
- 230726
- Author
- fpi...@apple.com
- Date
- 2018-04-17 12:56:33 -0700 (Tue, 17 Apr 2018)
Log Message
JSGenericTypedArrayView<>::visitChildren has a race condition reading m_mode and m_vector
https://bugs.webkit.org/show_bug.cgi?id=184705
Reviewed by Michael Saboff.
My old multisocket Mac Pro is amazing at catching race conditions in the GC. Earlier today
while testing an unrelated patch, a concurrent GC thread crashed inside
JSGenericTypedArrayView<>::visitChildren() calling markAuxiliary(). I'm pretty sure it's
because a typed array became wasteful concurrently to the GC. So, visitChildren() read one
mode and another vector.
The fix is to lock inside visitChildren and anyone who changes those fields.
I'm not even going to try to write a test. I think it's super lucky that my Mac Pro caught
this.
* runtime/JSArrayBufferView.cpp:
(JSC::JSArrayBufferView::neuter):
* runtime/JSGenericTypedArrayViewInlines.h:
(JSC::JSGenericTypedArrayView<Adaptor>::visitChildren):
(JSC::JSGenericTypedArrayView<Adaptor>::slowDownAndWasteMemory):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (230725 => 230726)
--- trunk/Source/_javascript_Core/ChangeLog 2018-04-17 19:53:30 UTC (rev 230725)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-04-17 19:56:33 UTC (rev 230726)
@@ -1,3 +1,27 @@
+2018-04-17 Filip Pizlo <fpi...@apple.com>
+
+ JSGenericTypedArrayView<>::visitChildren has a race condition reading m_mode and m_vector
+ https://bugs.webkit.org/show_bug.cgi?id=184705
+
+ Reviewed by Michael Saboff.
+
+ My old multisocket Mac Pro is amazing at catching race conditions in the GC. Earlier today
+ while testing an unrelated patch, a concurrent GC thread crashed inside
+ JSGenericTypedArrayView<>::visitChildren() calling markAuxiliary(). I'm pretty sure it's
+ because a typed array became wasteful concurrently to the GC. So, visitChildren() read one
+ mode and another vector.
+
+ The fix is to lock inside visitChildren and anyone who changes those fields.
+
+ I'm not even going to try to write a test. I think it's super lucky that my Mac Pro caught
+ this.
+
+ * runtime/JSArrayBufferView.cpp:
+ (JSC::JSArrayBufferView::neuter):
+ * runtime/JSGenericTypedArrayViewInlines.h:
+ (JSC::JSGenericTypedArrayView<Adaptor>::visitChildren):
+ (JSC::JSGenericTypedArrayView<Adaptor>::slowDownAndWasteMemory):
+
2018-04-16 Filip Pizlo <fpi...@apple.com>
PutStackSinkingPhase should know that KillStack means ConflictingFlush
Modified: trunk/Source/_javascript_Core/runtime/JSArrayBufferView.cpp (230725 => 230726)
--- trunk/Source/_javascript_Core/runtime/JSArrayBufferView.cpp 2018-04-17 19:53:30 UTC (rev 230725)
+++ trunk/Source/_javascript_Core/runtime/JSArrayBufferView.cpp 2018-04-17 19:56:33 UTC (rev 230726)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2018 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -209,6 +209,7 @@
void JSArrayBufferView::neuter()
{
+ auto locker = holdLock(cellLock());
RELEASE_ASSERT(hasArrayBuffer());
RELEASE_ASSERT(!isShared());
m_length = 0;
Modified: trunk/Source/_javascript_Core/runtime/JSGenericTypedArrayViewInlines.h (230725 => 230726)
--- trunk/Source/_javascript_Core/runtime/JSGenericTypedArrayViewInlines.h 2018-04-17 19:53:30 UTC (rev 230725)
+++ trunk/Source/_javascript_Core/runtime/JSGenericTypedArrayViewInlines.h 2018-04-17 19:56:33 UTC (rev 230726)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2013, 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2018 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -515,15 +515,26 @@
{
JSGenericTypedArrayView* thisObject = jsCast<JSGenericTypedArrayView*>(cell);
- switch (thisObject->m_mode) {
+ TypedArrayMode mode;
+ void* vector;
+ size_t byteSize;
+
+ {
+ auto locker = holdLock(thisObject->cellLock());
+ mode = thisObject->m_mode;
+ vector = thisObject->m_vector.getMayBeNull();
+ byteSize = thisObject->byteSize();
+ }
+
+ switch (mode) {
case FastTypedArray: {
- if (void* vector = thisObject->m_vector.getMayBeNull())
+ if (vector)
visitor.markAuxiliary(vector);
break;
}
case OversizeTypedArray: {
- visitor.reportExtraMemoryVisited(thisObject->byteSize());
+ visitor.reportExtraMemoryVisited(byteSize);
break;
}
@@ -583,10 +594,13 @@
break;
}
- thisObject->butterfly()->indexingHeader()->setArrayBuffer(buffer.get());
- thisObject->m_vector.setWithoutBarrier(buffer->data());
- WTF::storeStoreFence();
- thisObject->m_mode = WastefulTypedArray;
+ {
+ auto locker = holdLock(thisObject->cellLock());
+ thisObject->butterfly()->indexingHeader()->setArrayBuffer(buffer.get());
+ thisObject->m_vector.setWithoutBarrier(buffer->data());
+ WTF::storeStoreFence();
+ thisObject->m_mode = WastefulTypedArray;
+ }
heap->addReference(thisObject, buffer.get());
return buffer.get();
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
