Diff
Modified: trunk/Source/WebCore/ChangeLog (289475 => 289476)
--- trunk/Source/WebCore/ChangeLog 2022-02-09 16:56:45 UTC (rev 289475)
+++ trunk/Source/WebCore/ChangeLog 2022-02-09 17:06:14 UTC (rev 289476)
@@ -1,3 +1,46 @@
+2022-02-09 Yusuke Suzuki <[email protected]>
+
+ AudioBuffer should take a lock while visiting m_channelWrappers
+ https://bugs.webkit.org/show_bug.cgi?id=236279
+
+ Reviewed by Keith Miller.
+
+ This patch fixes AudioBuffer's m_channelWrappers concurrency bug and related issues.
+
+ 1. This patch removes problematic (and almost always wrong) move operator of JSValueInWrappedObject.
+ To do that, we fixed AudioBuffer's concurrency issue where we access m_channelWrappers while it
+ can be cleared concurrently in AudioBuffer::releaseMemory.
+ 2. MessageEvent's m_data access is broken with concurrent GC thread. We must take a lock. And we must
+ not use JSValueInWrappedObject in std::variant if it can be changed after constructor invocation.
+ 3. Use JSValueInWrappedObject::clear instead of move with empty value.
+ 4. File https://bugs.webkit.org/show_bug.cgi?id=236353. AbortSignal, MessageEvent, and CustomEvent miss
+ write-barrier, which is semantically wrong.
+
+ * Modules/webaudio/AudioBuffer.cpp:
+ (WebCore::AudioBuffer::AudioBuffer):
+ (WebCore::AudioBuffer::releaseMemory):
+ (WebCore::AudioBuffer::visitChannelWrappers):
+ * Modules/webaudio/AudioBuffer.h:
+ * Modules/webaudio/AudioWorkletProcessor.cpp:
+ (WebCore::AudioWorkletProcessor::buildJSArguments):
+ * bindings/js/JSMessageEventCustom.cpp:
+ (WebCore::JSMessageEvent::data const):
+ (WebCore::JSMessageEvent::visitAdditionalChildren):
+ * bindings/js/JSValueInWrappedObject.h:
+ * dom/AbortSignal.cpp:
+ (WebCore::AbortSignal::signalAbort):
+ * dom/CustomEvent.cpp:
+ (WebCore::CustomEvent::initCustomEvent):
+ * dom/MessageEvent.cpp:
+ (WebCore::MessageEvent::MessageEvent):
+ (WebCore::m_jsData):
+ (WebCore::MessageEvent::initMessageEvent):
+ (WebCore::MessageEvent::memoryCost const):
+ (WebCore::m_ports): Deleted.
+ * dom/MessageEvent.h:
+ * page/History.cpp:
+ (WebCore::History::stateObjectAdded):
+
2022-02-09 Sihui Liu <[email protected]>
Manage IndexedDB storage by origin
Modified: trunk/Source/WebCore/Modules/webaudio/AudioBuffer.cpp (289475 => 289476)
--- trunk/Source/WebCore/Modules/webaudio/AudioBuffer.cpp 2022-02-09 16:56:45 UTC (rev 289475)
+++ trunk/Source/WebCore/Modules/webaudio/AudioBuffer.cpp 2022-02-09 17:06:14 UTC (rev 289476)
@@ -91,21 +91,24 @@
return;
}
- m_channels.reserveCapacity(numberOfChannels);
+ Vector<RefPtr<Float32Array>> channels;
+ channels.reserveInitialCapacity(numberOfChannels);
for (unsigned i = 0; i < numberOfChannels; ++i) {
auto channelDataArray = Float32Array::tryCreate(m_originalLength);
if (!channelDataArray) {
invalidate();
- break;
+ return;
}
if (preventDetaching == LegacyPreventDetaching::Yes)
channelDataArray->setDetachable(false);
- m_channels.append(WTFMove(channelDataArray));
+ channels.uncheckedAppend(WTFMove(channelDataArray));
}
- m_channelWrappers.resize(m_channels.size());
+
+ m_channels = WTFMove(channels);
+ m_channelWrappers = FixedVector<JSValueInWrappedObject> { m_channels.size() };
}
AudioBuffer::AudioBuffer(AudioBus& bus)
@@ -119,18 +122,21 @@
// Copy audio data from the bus to the Float32Arrays we manage.
unsigned numberOfChannels = bus.numberOfChannels();
- m_channels.reserveCapacity(numberOfChannels);
+ Vector<RefPtr<Float32Array>> channels;
+ channels.reserveInitialCapacity(numberOfChannels);
for (unsigned i = 0; i < numberOfChannels; ++i) {
auto channelDataArray = Float32Array::tryCreate(m_originalLength);
if (!channelDataArray) {
invalidate();
- break;
+ return;
}
channelDataArray->setRange(bus.channel(i)->data(), m_originalLength, 0);
- m_channels.append(WTFMove(channelDataArray));
+ channels.uncheckedAppend(WTFMove(channelDataArray));
}
- m_channelWrappers.resize(m_channels.size());
+
+ m_channels = WTFMove(channels);
+ m_channelWrappers = FixedVector<JSValueInWrappedObject> { m_channels.size() };
}
void AudioBuffer::invalidate()
@@ -142,8 +148,8 @@
void AudioBuffer::releaseMemory()
{
Locker locker { m_channelsLock };
- m_channels.clear();
- m_channelWrappers.clear();
+ m_channels = { };
+ m_channelWrappers = { };
}
ExceptionOr<JSC::JSValue> AudioBuffer::getChannelData(JSDOMGlobalObject& globalObject, unsigned channelIndex)
@@ -168,8 +174,7 @@
template<typename Visitor>
void AudioBuffer::visitChannelWrappers(Visitor& visitor)
{
- // FIXME: AudioBuffer::releaseMemory can clear this buffer while visiting it from concurrent GC thread.
- // https://bugs.webkit.org/show_bug.cgi?id=236279
+ Locker locker { m_channelsLock };
for (auto& channelWrapper : m_channelWrappers)
channelWrapper.visit(visitor);
}
Modified: trunk/Source/WebCore/Modules/webaudio/AudioBuffer.h (289475 => 289476)
--- trunk/Source/WebCore/Modules/webaudio/AudioBuffer.h 2022-02-09 16:56:45 UTC (rev 289475)
+++ trunk/Source/WebCore/Modules/webaudio/AudioBuffer.h 2022-02-09 17:06:14 UTC (rev 289476)
@@ -98,11 +98,11 @@
static constexpr uint64_t s_maxLength = (1ull << 32) / sizeof(float);
float m_sampleRate;
- mutable Lock m_channelsLock;
size_t m_originalLength;
- Vector<RefPtr<Float32Array>> m_channels;
- Vector<JSValueInWrappedObject> m_channelWrappers;
+ FixedVector<RefPtr<Float32Array>> m_channels;
+ FixedVector<JSValueInWrappedObject> m_channelWrappers;
bool m_isDetachable { true };
+ mutable Lock m_channelsLock;
};
} // namespace WebCore
Modified: trunk/Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp (289475 => 289476)
--- trunk/Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp 2022-02-09 16:56:45 UTC (rev 289475)
+++ trunk/Source/WebCore/Modules/webaudio/AudioWorkletProcessor.cpp 2022-02-09 17:06:14 UTC (rev 289476)
@@ -222,15 +222,15 @@
{
// For performance reasons, we cache the arrays passed to JS and reconstruct them only when the topology changes.
if (!copyDataFromBusesToJSArray(vm, globalObject, inputs, toJSArray(m_jsInputs)))
- m_jsInputs = { constructFrozenJSArray(vm, globalObject, inputs, ShouldPopulateWithBusData::Yes) };
+ m_jsInputs.setWeakly(constructFrozenJSArray(vm, globalObject, inputs, ShouldPopulateWithBusData::Yes));
args.append(m_jsInputs.getValue());
if (!zeroJSArray(vm, globalObject, outputs, toJSArray(m_jsOutputs)))
- m_jsOutputs = { constructFrozenJSArray(vm, globalObject, outputs, ShouldPopulateWithBusData::No) };
+ m_jsOutputs.setWeakly(constructFrozenJSArray(vm, globalObject, outputs, ShouldPopulateWithBusData::No));
args.append(m_jsOutputs.getValue());
if (!copyDataFromParameterMapToJSObject(vm, globalObject, paramValuesMap, toJSObject(m_jsParamValues)))
- m_jsParamValues = { constructFrozenKeyValueObject(vm, globalObject, paramValuesMap) };
+ m_jsParamValues.setWeakly(constructFrozenKeyValueObject(vm, globalObject, paramValuesMap));
args.append(m_jsParamValues.getValue());
}
Modified: trunk/Source/WebCore/bindings/js/JSMessageEventCustom.cpp (289475 => 289476)
--- trunk/Source/WebCore/bindings/js/JSMessageEventCustom.cpp 2022-02-09 16:56:45 UTC (rev 289475)
+++ trunk/Source/WebCore/bindings/js/JSMessageEventCustom.cpp 2022-02-09 17:06:14 UTC (rev 289476)
@@ -55,8 +55,8 @@
JSC::JSValue JSMessageEvent::data(JSC::JSGlobalObject& lexicalGlobalObject) const
{
return cachedPropertyValue(lexicalGlobalObject, *this, wrapped().cachedData(), [this, &lexicalGlobalObject] {
- return WTF::switchOn(wrapped().data(), [] (const JSValueInWrappedObject& data) {
- return data.getValue(JSC::jsNull());
+ return WTF::switchOn(wrapped().data(), [this] (MessageEvent::JSValueTag) -> JSC::JSValue {
+ return wrapped().jsData().getValue(JSC::jsNull());
}, [this, &lexicalGlobalObject] (const Ref<SerializedScriptValue>& data) {
// FIXME: Is it best to handle errors by returning null rather than throwing an exception?
return data->deserialize(lexicalGlobalObject, globalObject(), wrapped().ports(), SerializationErrorMode::NonThrowing);
@@ -73,14 +73,7 @@
template<typename Visitor>
void JSMessageEvent::visitAdditionalChildren(Visitor& visitor)
{
- WTF::switchOn(wrapped().data(), [&visitor] (const JSValueInWrappedObject& data) {
- data.visit(visitor);
- }, [] (const Ref<SerializedScriptValue>&) {
- }, [] (const String&) {
- }, [] (const Ref<Blob>&) {
- }, [] (const Ref<ArrayBuffer>&) {
- });
-
+ wrapped().jsData().visit(visitor);
wrapped().cachedData().visit(visitor);
wrapped().cachedPorts().visit(visitor);
}
Modified: trunk/Source/WebCore/bindings/js/JSValueInWrappedObject.h (289475 => 289476)
--- trunk/Source/WebCore/bindings/js/JSValueInWrappedObject.h 2022-02-09 16:56:45 UTC (rev 289475)
+++ trunk/Source/WebCore/bindings/js/JSValueInWrappedObject.h 2022-02-09 17:06:14 UTC (rev 289476)
@@ -33,17 +33,15 @@
namespace WebCore {
+// This class includes a lot of GC related subtle things, and changing this class easily causes GC crashes.
+// Any changes on this class must be reviewed by _javascript_Core reviewers too.
class JSValueInWrappedObject {
- // It must not be copyable. Changing this will break concurrent GC.
+ // It must be neither copyable nor movable. Changing this will break concurrent GC.
WTF_MAKE_NONCOPYABLE(JSValueInWrappedObject);
+ WTF_MAKE_NONMOVABLE(JSValueInWrappedObject);
public:
JSValueInWrappedObject(JSC::JSValue = { });
- // FIXME: Remove them once AudioBuffer's m_channelWrappers bug is fixed.
- // https://bugs.webkit.org/show_bug.cgi?id=236279
- JSValueInWrappedObject(JSValueInWrappedObject&&) = default;
- JSValueInWrappedObject& operator=(JSValueInWrappedObject&&) = default;
-
explicit operator bool() const;
template<typename Visitor> void visit(Visitor&) const;
void clear();
Modified: trunk/Source/WebCore/dom/AbortSignal.cpp (289475 => 289476)
--- trunk/Source/WebCore/dom/AbortSignal.cpp 2022-02-09 16:56:45 UTC (rev 289475)
+++ trunk/Source/WebCore/dom/AbortSignal.cpp 2022-02-09 17:06:14 UTC (rev 289476)
@@ -95,8 +95,10 @@
// 2. Set signal’s aborted flag.
m_aborted = true;
+ // FIXME: This code is wrong: we should emit a write-barrier. Otherwise, GC can collect it.
+ // https://bugs.webkit.org/show_bug.cgi?id=236353
ASSERT(reason);
- m_reason = reason;
+ m_reason.setWeakly(reason);
Ref protectedThis { *this };
auto algorithms = std::exchange(m_algorithms, { });
Modified: trunk/Source/WebCore/dom/CustomEvent.cpp (289475 => 289476)
--- trunk/Source/WebCore/dom/CustomEvent.cpp 2022-02-09 16:56:45 UTC (rev 289475)
+++ trunk/Source/WebCore/dom/CustomEvent.cpp 2022-02-09 17:06:14 UTC (rev 289476)
@@ -64,8 +64,10 @@
initEvent(type, canBubble, cancelable);
- m_detail = detail;
- m_cachedDetail = { };
+ // FIXME: This code is wrong: we should emit a write-barrier. Otherwise, GC can collect it.
+ // https://bugs.webkit.org/show_bug.cgi?id=236353
+ m_detail.setWeakly(detail);
+ m_cachedDetail.clear();
}
EventInterface CustomEvent::eventInterface() const
Modified: trunk/Source/WebCore/dom/MessageEvent.cpp (289475 => 289476)
--- trunk/Source/WebCore/dom/MessageEvent.cpp 2022-02-09 16:56:45 UTC (rev 289475)
+++ trunk/Source/WebCore/dom/MessageEvent.cpp 2022-02-09 17:06:14 UTC (rev 289476)
@@ -43,11 +43,12 @@
inline MessageEvent::MessageEvent(const AtomString& type, Init&& initializer, IsTrusted isTrusted)
: Event(type, initializer, isTrusted)
- , m_data(JSValueInWrappedObject { initializer.data })
+ , m_data(JSValueTag { })
, m_origin(initializer.origin)
, m_lastEventId(initializer.lastEventId)
, m_source(WTFMove(initializer.source))
, m_ports(WTFMove(initializer.ports))
+ , m_jsData(initializer.data)
{
}
@@ -113,13 +114,19 @@
initEvent(type, canBubble, cancelable);
- m_data = JSValueInWrappedObject { data };
- m_cachedData = { };
+ {
+ Locker { m_concurrentDataAccessLock };
+ m_data = JSValueTag { };
+ }
+ // FIXME: This code is wrong: we should emit a write-barrier. Otherwise, GC can collect it.
+ // https://bugs.webkit.org/show_bug.cgi?id=236353
+ m_jsData.setWeakly(data);
+ m_cachedData.clear();
m_origin = origin;
m_lastEventId = lastEventId;
m_source = WTFMove(source);
m_ports = WTFMove(ports);
- m_cachedPorts = { };
+ m_cachedPorts.clear();
}
EventInterface MessageEvent::eventInterface() const
@@ -129,7 +136,8 @@
size_t MessageEvent::memoryCost() const
{
- return WTF::switchOn(m_data, [] (const JSValueInWrappedObject&) -> size_t {
+ Locker { m_concurrentDataAccessLock };
+ return WTF::switchOn(m_data, [] (JSValueTag) -> size_t {
return 0;
}, [] (const Ref<SerializedScriptValue>& data) -> size_t {
return data->memoryCost();
Modified: trunk/Source/WebCore/dom/MessageEvent.h (289475 => 289476)
--- trunk/Source/WebCore/dom/MessageEvent.h 2022-02-09 16:56:45 UTC (rev 289475)
+++ trunk/Source/WebCore/dom/MessageEvent.h 2022-02-09 17:06:14 UTC (rev 289476)
@@ -73,9 +73,11 @@
const std::optional<MessageEventSource>& source() const { return m_source; }
const Vector<RefPtr<MessagePort>>& ports() const { return m_ports; }
- using DataType = std::variant<JSValueInWrappedObject, Ref<SerializedScriptValue>, String, Ref<Blob>, Ref<ArrayBuffer>>;
+ struct JSValueTag { };
+ using DataType = std::variant<JSValueTag, Ref<SerializedScriptValue>, String, Ref<Blob>, Ref<ArrayBuffer>>;
const DataType& data() const { return m_data; }
+ JSValueInWrappedObject& jsData() { return m_jsData; }
JSValueInWrappedObject& cachedData() { return m_cachedData; }
JSValueInWrappedObject& cachedPorts() { return m_cachedPorts; }
@@ -95,8 +97,11 @@
std::optional<MessageEventSource> m_source;
Vector<RefPtr<MessagePort>> m_ports;
+ JSValueInWrappedObject m_jsData;
JSValueInWrappedObject m_cachedData;
JSValueInWrappedObject m_cachedPorts;
+
+ mutable Lock m_concurrentDataAccessLock;
};
} // namespace WebCore
Modified: trunk/Source/WebCore/page/History.cpp (289475 => 289476)
--- trunk/Source/WebCore/page/History.cpp 2022-02-09 16:56:45 UTC (rev 289475)
+++ trunk/Source/WebCore/page/History.cpp 2022-02-09 17:06:14 UTC (rev 289476)
@@ -180,7 +180,7 @@
ExceptionOr<void> History::stateObjectAdded(RefPtr<SerializedScriptValue>&& data, const String& title, const String& urlString, StateObjectType stateObjectType)
{
- m_cachedState = { };
+ m_cachedState.clear();
// Each unique main-frame document is only allowed to send 64MB of state object payload to the UI client/process.
static uint32_t totalStateObjectPayloadLimit = 0x4000000;
