Crash under FFTFrame::fftSetupForSize()

cdumez · cdumez · commit 12df326450b8 · 2021-01-22T18:42:32.000Z
https://bugs.webkit.org/show_bug.cgi?id=220866 <rdar://73199504> Reviewed by Eric Carlson. Source/WebCore: The crash was caused by FFTFrame::fftSetupForSize() but being called concurrently from "HRTF database loader" threads. This patch makes FFTFrame::fftSetupForSize() thread safe to address the issue. Test: webaudio/Panner/PannerNode-crash.html * platform/audio/mac/FFTFrameMac.cpp: (WebCore::fftSetups): (WebCore::FFTFrame::fftSetupForSize): LayoutTests: Add layout test coverage. * webaudio/Panner/PannerNode-crash-expected.txt: Added. * webaudio/Panner/PannerNode-crash.html: Added. Canonical link: https://commits.webkit.org/233258@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@271751 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,16 @@
+2021-01-22  Chris Dumez  <cdumez@apple.com>
+
+        Crash under FFTFrame::fftSetupForSize()
+        https://bugs.webkit.org/show_bug.cgi?id=220866
+        <rdar://73199504>
+
+        Reviewed by Eric Carlson.
+
+        Add layout test coverage.
+
+        * webaudio/Panner/PannerNode-crash-expected.txt: Added.
+        * webaudio/Panner/PannerNode-crash.html: Added.
+
 2021-01-22  Youenn Fablet  <youenn@apple.com>
 
         Add more descriptive messages to setLocalDescription/setRemoteDescription error cases
diff --git a/LayoutTests/webaudio/Panner/PannerNode-crash-expected.txt b/LayoutTests/webaudio/Panner/PannerNode-crash-expected.txt
@@ -0,0 +1,9 @@
+This test passes if it does not crash.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/webaudio/Panner/PannerNode-crash.html b/LayoutTests/webaudio/Panner/PannerNode-crash.html
@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test.js"></script>
+<script>
+description("This test passes if it does not crash.");
+jsTestIsAsync = true;
+
+const random = (min, max) => {
+    let num = Math.random() * (max - min) + min;
+
+    return Math.round(num);
+};
+
+onload = () => {
+    for (let i = 0; i < 50; i++) {
+        let sampleRate = random(3000, 384000);
+        new PannerNode(new OfflineAudioContext({length: 128, sampleRate: sampleRate}));
+        new OfflineAudioContext({length: 128, sampleRate: sampleRate}).createPanner().disconnect();
+    }
+    
+    setTimeout(finishJSTest, 100);
+};
+</script>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,21 @@
+2021-01-22  Chris Dumez  <cdumez@apple.com>
+
+        Crash under FFTFrame::fftSetupForSize()
+        https://bugs.webkit.org/show_bug.cgi?id=220866
+        <rdar://73199504>
+
+        Reviewed by Eric Carlson.
+
+        The crash was caused by FFTFrame::fftSetupForSize() but being called concurrently
+        from "HRTF database loader" threads. This patch makes FFTFrame::fftSetupForSize()
+        thread safe to address the issue.
+
+        Test: webaudio/Panner/PannerNode-crash.html
+
+        * platform/audio/mac/FFTFrameMac.cpp:
+        (WebCore::fftSetups):
+        (WebCore::FFTFrame::fftSetupForSize):
+
 2021-01-22  Youenn Fablet  <youenn@apple.com>
 
         Add more descriptive messages to setLocalDescription/setRemoteDescription error cases
diff --git a/Source/WebCore/platform/audio/mac/FFTFrameMac.cpp b/Source/WebCore/platform/audio/mac/FFTFrameMac.cpp
@@ -37,6 +37,7 @@
 #include "FFTFrame.h"
 
 #include "VectorMath.h"
+#include <wtf/Lock.h>
 #include <wtf/NeverDestroyed.h>
 #include <wtf/Vector.h>
 
@@ -121,15 +122,29 @@ void FFTFrame::doInverseFFT(float* data)
     VectorMath::multiplyByScalar(data, 1.0f / m_FFTSize, data, m_FFTSize);
 }
 
+static Vector<FFTSetup>& fftSetups()
+{
+    static LazyNeverDestroyed<Vector<FFTSetup>> fftSetups;
+    static std::once_flag onceKey;
+    std::call_once(onceKey, [&] {
+        fftSetups.construct(kMaxFFTPow2Size, nullptr);
+    });
+    return fftSetups;
+}
+
 FFTSetup FFTFrame::fftSetupForSize(unsigned fftSize)
 {
-    static NeverDestroyed<Vector<FFTSetup>> fftSetups(kMaxFFTPow2Size, nullptr);
+    static Lock fftSetupsLock;
 
     auto pow2size = static_cast<size_t>(log2(fftSize));
     ASSERT(pow2size < kMaxFFTPow2Size);
-    auto& fftSetup = fftSetups->at(pow2size);
-    if (!fftSetup)
-        fftSetup = vDSP_create_fftsetup(pow2size, FFT_RADIX2);
+
+    auto& fftSetup = fftSetups().at(pow2size);
+    if (!fftSetup) {
+        auto locker = holdLock(fftSetupsLock);
+        if (!fftSetup)
+            fftSetup = vDSP_create_fftsetup(pow2size, FFT_RADIX2);
+    }
 
     return fftSetup;
 }
