[WasmFS] Fix a deadlock on ProxyWorker creation (#18229)

It was previously possible for a deadlock to occur when a background thread
constructed the OPFS backend then tried to use it. If the main thread was also
trying to do a WasmFS operation at the same time, it could have been blocked on
a lock held by the background thread and therefore would never have received the
request for it to spawn the OPFS dedicated worker. Neither thread can make
progress in this situation and the page will become unresponsive.

To fix the problem in the simplest possible way, wait for the dedicated worker
to start running before returning from the `ProxyWorker` constructor. This
guarantees that the worker will already be running for subsequent OPFS backend
operations, so they will not be blocked from making progress.

Unfortunately this fix also means that trying to create a `ProxyWorker` from the
main thread will deadlock, so in particular the OPFS backend can no longer be
constructed on the main thread. This restriction should be fine for now because
in practice many applications are using -sPROXY_TO_PTHREAD and are already
creating their OPFS backends on background threads.

Author: tlively

system/include/emscripten/wasmfs.h

@@ -42,10 +42,28 @@ typedef backend_t (*backend_constructor_t)(void*);
 
 backend_t wasmfs_create_memory_backend(void);
 
+// Note: this cannot be called on the browser main thread because it might
+// deadlock while waiting for its dedicated worker thread to be spawned.
+//
+// Note: This function blocks on the main browser thread returning to its event
+// loop. Calling this function while holding a lock the main thread is waiting
+// to acquire will cause a deadlock.
+//
+// TODO: Add an async version of this function that will work on the main
+// thread.
 backend_t wasmfs_create_fetch_backend(const char* base_url);
 
 backend_t wasmfs_create_node_backend(const char* root);
 
+// Note: this cannot be called on the browser main thread because it might
+// deadlock while waiting for the OPFS dedicated worker thread to be spawned.
+//
+// Note: This function blocks on the main browser thread returning to its event
+// loop. Calling this function while holding a lock the main thread is waiting
+// to acquire will cause a deadlock.
+//
+// TODO: Add an async version of this function that will work on the main
+// thread.
 backend_t wasmfs_create_opfs_backend(void);
 
 backend_t wasmfs_create_icase_backend(backend_constructor_t create_backend,

system/lib/wasmfs/backends/fetch_backend.cpp

@@ -84,6 +84,10 @@ class FetchBackend : public ProxiedAsyncJSBackend {
 
 extern "C" {
 backend_t wasmfs_create_fetch_backend(const char* base_url) {
+  // ProxyWorker cannot safely be synchronously spawned from the main browser
+  // thread. See comment in thread_utils.h for more details.
+  assert(!emscripten_is_main_browser_thread() &&
+         "Cannot safely create fetch backend on main browser thread");
   return wasmFS.addBackend(std::make_unique<FetchBackend>(
     base_url ? base_url : "",
     [](backend_t backend) { _wasmfs_create_fetch_backend_js(backend); }));

system/lib/wasmfs/backends/opfs_backend.cpp

@@ -3,12 +3,14 @@
 // University of Illinois/NCSA Open Source License.  Both these licenses can be
 // found in the LICENSE file.
 
+#include <emscripten/threading.h>
+#include <stdlib.h>
+
 #include "backend.h"
 #include "file.h"
 #include "support.h"
 #include "thread_utils.h"
 #include "wasmfs.h"
-#include <stdlib.h>
 
 using namespace wasmfs;
 
@@ -470,6 +472,10 @@ class OPFSBackend : public Backend {
 extern "C" {
 
 backend_t wasmfs_create_opfs_backend() {
+  // ProxyWorker cannot safely be synchronously spawned from the main browser
+  // thread. See comment in thread_utils.h for more details.
+  assert(!emscripten_is_main_browser_thread() &&
+         "Cannot safely create OPFS backend on main browser thread");
   return wasmFS.addBackend(std::make_unique<OPFSBackend>());
 }
 

system/lib/wasmfs/thread_utils.h

@@ -11,6 +11,7 @@
 #include <thread>
 
 #include <emscripten/proxying.h>
+#include <emscripten/threading.h>
 
 namespace emscripten {
 
@@ -21,10 +22,22 @@ class ProxyWorker {
   ProxyingQueue queue;
   std::thread thread;
 
+  // Used to notify the calling thread once the worker has been started.
+  bool started = false;
+  std::mutex mutex;
+  std::condition_variable cond;
+
 public:
   // Spawn the worker thread.
   ProxyWorker()
     : queue(), thread([&]() {
+        // Notify the caller that we have started.
+        {
+          std::unique_lock<std::mutex> lock(mutex);
+          started = true;
+        }
+        cond.notify_all();
+
         // Sometimes the main thread is spinning, waiting on a WasmFS lock held
         // by a thread trying to proxy work to this dedicated worker. In that
         // case, the proxying message won't be relayed by the main thread and
@@ -50,7 +63,27 @@ class ProxyWorker {
 
         // Sit in the event loop performing work as it comes in.
         emscripten_exit_with_live_runtime();
-      }) {}
+      }) {
+
+    // Make sure the thread has actually started before returning. This allows
+    // subsequent code to assume the thread has already been spawned and not
+    // worry about potential deadlocks where it holds a lock while proxying an
+    // operation and meanwhile the main thread is blocked trying to acqure the
+    // same lock so is never able to spawn the worker thread.
+    //
+    // Unfortunately, this solution would cause the main thread to deadlock on
+    // itself, so for now assert that we are not on the main thread. In the
+    // future, we could provide an asynchronous version of this utility that
+    // calls a user callback once the worker has been started. This asynchronous
+    // version would be safe to use on the main thread.
+    assert(
+      !emscripten_is_main_browser_thread() &&
+      "cannot safely spawn dedicated workers from the main browser thread");
+    {
+      std::unique_lock<std::mutex> lock(mutex);
+      cond.wait(lock, [&]() { return started; });
+    }
+  }
 
   // Kill the worker thread.
   ~ProxyWorker() {

test/test_browser.py

@@ -5344,8 +5344,6 @@ def test_dlmalloc_3GB(self):
   })
   @requires_threads
   def test_wasmfs_fetch_backend(self, args):
-    if is_firefox() and '-sPROXY_TO_PTHREAD' not in args:
-      return self.skipTest('ff hangs on the main_thread version. browser bug?')
     create_file('data.dat', 'hello, fetch')
     create_file('small.dat', 'hello')
     create_file('test.txt', 'fetch 2')
@@ -5354,7 +5352,8 @@ def test_wasmfs_fetch_backend(self, args):
     create_file('subdir/backendfile', 'file 1')
     create_file('subdir/backendfile2', 'file 2')
     self.btest_exit(test_file('wasmfs/wasmfs_fetch.c'),
-                    args=['-sWASMFS', '-sUSE_PTHREADS', '--js-library', test_file('wasmfs/wasmfs_fetch.js')] + args)
+                    args=['-sWASMFS', '-sUSE_PTHREADS', '-sPROXY_TO_PTHREAD', '-sINITIAL_MEMORY=32MB',
+                          '--js-library', test_file('wasmfs/wasmfs_fetch.js')] + args)
 
   @requires_threads
   @no_firefox('no OPFS support yet')