Skip to content

Commit

Permalink
Fixed deadlock when sealing objects with a LazyCtor
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
andreabergia authored and gbrail committed May 30, 2024
1 parent 085d561 commit c6a873f
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 9 deletions.
39 changes: 30 additions & 9 deletions src/org/mozilla/javascript/ScriptableObject.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
30 changes: 30 additions & 0 deletions testsrc/org/mozilla/javascript/ThreadSafeScriptableObjectTest.java
Original file line number Diff line number Diff line change
@@ -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));
}
}
}

0 comments on commit c6a873f

Please sign in to comment.