Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed deadlock when sealing objects with a LazyCtor #1476

Merged
merged 1 commit into from
May 30, 2024
Merged

Fixed deadlock when sealing objects with a LazyCtor #1476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 30 additions & 9 deletions src/org/mozilla/javascript/ScriptableObject.java
View file
Open in desktop
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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we're reading the "value" field of the slot without a lock being held.

if (value instanceof LazilyLoadedCtor) {
LazilyLoadedCtor initializer = (LazilyLoadedCtor) value;
try {
initializer.init();
} finally {
slot.value = initializer.getValue();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we're setting the value of the slot without the lock being held.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a bit safer if it were instead:

long stamp = slotMap.writeLock()
slot.value = initializer.getValue();
slotMap.unlockWrite(stamp)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this doesn't seem consistent with the way locks are used elsewhere in the code. Specifically the locks on slot maps are only for reading and mutation of the slot map, not the values of the slots. The reason we hit the deadlock isn't that we're setting the value for the slot being initialised, but rather because the lazy constructors for Java packages create additional slots.

Setting the value on a normal slot is thread safe, it's just setting a field, though I guess we could add var handle support if concurrent operations are needed with specified ordering, but since setting the value doesn't change the slot map no lock is required to ensure thread safety.

Lazy constructors themselves appear to be thread safe since they are synchronised, but there is an amusing possible ordering problem because a lazy constructor could set its own slot value to something, seal the scope, and then return a completely different value which would then be set on the slot. That's technically fine because the object is sealed rather than frozen, but Rhino seems to already be wrong in its treatment of sealed and the mutation of properties in several places.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right and I misremembered what we were trying to do here. The lock protects the map and not the value, which means that this change doesn't change the purpose of what the lock is trying to do.

I still maintain that Rhino was never really designed properly for the case of multiple threads executing in the context of the same script in parallel, and if it did we'd have to make up lots of new rules because the standard doesn't contemplate that use case. But for projects that have done this in the past, this particular support at least helps a bit.

}
}
}
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
View file
Open in desktop
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));
}
}
}
Loading