From 94246fba047be5c4faf9f87bacbedd4a8fe62191 Mon Sep 17 00:00:00 2001
From: "andrea.bergia" <andrea.bergia@servicenow.com>
Date: Fri, 17 May 2024 14:06:08 +0200
Subject: [PATCH] Fixed deadlock when sealing objects with a LazyCtor

The reason is that ScriptableObject::sealObject will initialize eagerly
any LazyLoadedCtor that are stored in the slot map - it needs to that
because some of those will actually mutate the object (such as
NativeJavaTopPackage), and that cannot work if the object is already
sealed. However, it will also acquire a read lock on the slot map
before iterating on it. But the mutations will try to acquire a write
lock, causing a deadlock with just one thread.

This change fixes it by not holding the read lock while mutating, and
also avoids marking the object as sealed until it is certainly correct
to do so.
---
 .../mozilla/javascript/ScriptableObject.java  | 39 ++++++++++++++-----
 .../ThreadSafeScriptableObjectTest.java       | 30 ++++++++++++++
 2 files changed, 60 insertions(+), 9 deletions(-)
 create mode 100644 testsrc/org/mozilla/javascript/ThreadSafeScriptableObjectTest.java

diff --git a/src/org/mozilla/javascript/ScriptableObject.java b/src/org/mozilla/javascript/ScriptableObject.java
index 6ff475eaab..a6cefa07c4 100644
--- a/src/org/mozilla/javascript/ScriptableObject.java
+++ b/src/org/mozilla/javascript/ScriptableObject.java
@@ -19,10 +19,12 @@
 import java.lang.reflect.Member;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.function.Consumer;
 import java.util.function.Supplier;
@@ -1978,21 +1980,40 @@ public void preventExtensions() {
      * @since 1.4R3
      */
     public void sealObject() {
-        if (!isSealed) {
-            final long stamp = slotMap.readLock();
+        // We cannot initialize LazyLoadedCtor while holding the read lock, since they will mutate
+        // the current object (case in point: NativeJavaTopPackage), which in turn would cause the
+        // code
+        // to try to acquire a write lock while holding the read lock... and deadlock.
+        // We do this in a loop because an initialization could add _other_ stuff to initialize.
+
+        List<Slot> toInitialize = new ArrayList<>();
+        while (!isSealed) {
+            for (Slot slot : toInitialize) {
+                // Need to check the type again, because initializing one slot _could_ have
+                // initialized another one
+                Object value = slot.value;
+                if (value instanceof LazilyLoadedCtor) {
+                    LazilyLoadedCtor initializer = (LazilyLoadedCtor) value;
+                    try {
+                        initializer.init();
+                    } finally {
+                        slot.value = initializer.getValue();
+                    }
+                }
+            }
+            toInitialize.clear();
+
+            long stamp = slotMap.readLock();
             try {
                 for (Slot slot : slotMap) {
                     Object value = slot.value;
                     if (value instanceof LazilyLoadedCtor) {
-                        LazilyLoadedCtor initializer = (LazilyLoadedCtor) value;
-                        try {
-                            initializer.init();
-                        } finally {
-                            slot.value = initializer.getValue();
-                        }
+                        toInitialize.add(slot);
                     }
                 }
-                isSealed = true;
+                if (toInitialize.isEmpty()) {
+                    isSealed = true;
+                }
             } finally {
                 slotMap.unlockRead(stamp);
             }
diff --git a/testsrc/org/mozilla/javascript/ThreadSafeScriptableObjectTest.java b/testsrc/org/mozilla/javascript/ThreadSafeScriptableObjectTest.java
new file mode 100644
index 0000000000..e1edb993c0
--- /dev/null
+++ b/testsrc/org/mozilla/javascript/ThreadSafeScriptableObjectTest.java
@@ -0,0 +1,30 @@
+package org.mozilla.javascript;
+
+import static org.junit.Assert.assertNotNull;
+
+import org.junit.Test;
+
+public class ThreadSafeScriptableObjectTest {
+    @Test
+    public void canSealGlobalObjectWithoutDeadlock() {
+        ContextFactory.getGlobalSetter()
+                .setContextFactoryGlobal(
+                        new ContextFactory() {
+                            @Override
+                            protected boolean hasFeature(Context cx, int featureIndex) {
+                                if (featureIndex == Context.FEATURE_THREAD_SAFE_OBJECTS) {
+                                    return true;
+                                }
+                                return super.hasFeature(cx, featureIndex);
+                            }
+                        });
+
+        try (Context cx = Context.enter()) {
+            ScriptableObject global = cx.initStandardObjects();
+            global.sealObject();
+
+            // Registered by NativeJavaTopPackage
+            assertNotNull(global.get("Packages", global));
+        }
+    }
+}