Skip to content

Commit 9e6dd8c

Browse files
authored
Fix deadlock when promoting to ThreadSafeHashSlotMap
The implementation of thread-safe slot map promotion done in mozilla#1785 has a critical bug: appending enough properties to an object created with the thread-safe slot maps will cause a promotion to `ThreadSafeHashSlotMap`, which will deadlock. I have added a test that reproduces the problem and a fix for it.
1 parent f9f683f commit 9e6dd8c

5 files changed

Lines changed: 90 additions & 5 deletions

File tree

rhino/src/main/java/org/mozilla/javascript/EmbeddedSlotMap.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ private void createNewSlot(SlotMapOwner owner, Slot newSlot) {
131131
// Check if the table is not too full before inserting.
132132
if (4 * (count + 1) > 3 * slots.length) {
133133
// table size must be a power of 2 -- always grow by x2!
134-
if (count > SlotMapOwner.LARGE_HASH_SIZE) {
134+
if (count >= SlotMapOwner.LARGE_HASH_SIZE) {
135135
promoteMap(owner, newSlot);
136136
return;
137137
}

rhino/src/main/java/org/mozilla/javascript/SlotMapOwner.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,12 @@
1010
public abstract class SlotMapOwner {
1111
private static final long serialVersionUID = 1L;
1212

13-
static final int LARGE_HASH_SIZE = 2000;
13+
/**
14+
* Maximum size of an {@link EmbeddedSlotMap} before it is promoted to a {@link HashSlotMap}.
15+
* The value must be 3/4 of a power of two, because the embedded slot map's slot size must be a
16+
* power of two, and it is resized when it is 3/4 full.
17+
*/
18+
static final int LARGE_HASH_SIZE = (1 << 10) + (1 << 9);
1419

1520
static final SlotMap EMPTY_SLOT_MAP = new EmptySlotMap();
1621

rhino/src/main/java/org/mozilla/javascript/ThreadSafeEmbeddedSlotMap.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ public int size() {
3434

3535
@Override
3636
public int dirtySize() {
37-
assert lock.isReadLocked();
37+
assert lock.isReadLocked() || lock.isWriteLocked();
3838
return current.sizeWithLock();
3939
}
4040

rhino/src/main/java/org/mozilla/javascript/ThreadSafeHashSlotMap.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ public ThreadSafeHashSlotMap(StampedLock lock, SlotMap oldMap) {
2626
}
2727

2828
public ThreadSafeHashSlotMap(StampedLock lock, SlotMap oldMap, Slot newSlot) {
29-
super(oldMap.size() + 1);
29+
super(oldMap.dirtySize() + 1);
3030
this.lock = lock;
3131
for (Slot n : oldMap) {
3232
addWithLock(null, n.copySlot());
@@ -52,7 +52,7 @@ public int size() {
5252

5353
@Override
5454
public int dirtySize() {
55-
assert lock.isReadLocked();
55+
assert lock.isReadLocked() || lock.isWriteLocked();
5656
return super.size();
5757
}
5858

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
package org.mozilla.javascript;
2+
3+
import static org.junit.jupiter.api.Assertions.assertEquals;
4+
5+
import java.util.function.Supplier;
6+
import org.junit.jupiter.api.Test;
7+
8+
/** Ensures that slot map promotion */
9+
class SlotMapPromotionTest {
10+
@Test
11+
public void promotionFromEmptyToSingleSlot() {
12+
assertPromotes(() -> SlotMapOwner.EMPTY_SLOT_MAP, SlotMapOwner.SingleEntrySlotMap.class);
13+
}
14+
15+
@Test
16+
public void promotionFromSingleToEmbedded() {
17+
assertPromotes(
18+
() -> new SlotMapOwner.SingleEntrySlotMap(new Slot(new Object(), 0, 0)),
19+
EmbeddedSlotMap.class);
20+
}
21+
22+
@Test
23+
public void promotionFromEmbeddedToHash() {
24+
assertPromotes(
25+
() -> {
26+
var map = new EmbeddedSlotMap(SlotMapOwner.LARGE_HASH_SIZE);
27+
fillToCapacity(SlotMapOwner.LARGE_HASH_SIZE, map);
28+
return map;
29+
},
30+
HashSlotMap.class);
31+
}
32+
33+
@Test
34+
public void promotionFromThreadSafeEmptyToSingleSlot() {
35+
assertPromotes(
36+
() -> SlotMapOwner.THREAD_SAFE_EMPTY_SLOT_MAP,
37+
SlotMapOwner.ThreadSafeSingleEntrySlotMap.class);
38+
}
39+
40+
@Test
41+
public void promotionFromThreadSafeSingleToEmbedded() {
42+
assertPromotes(
43+
() -> new SlotMapOwner.ThreadSafeSingleEntrySlotMap(new Slot(new Object(), 0, 0)),
44+
ThreadSafeEmbeddedSlotMap.class);
45+
}
46+
47+
@Test
48+
public void promotionFromThreadSafeEmbeddedToHash() {
49+
assertPromotes(
50+
() -> {
51+
var map = new ThreadSafeEmbeddedSlotMap();
52+
fillToCapacity(SlotMapOwner.LARGE_HASH_SIZE, map);
53+
return map;
54+
},
55+
ThreadSafeHashSlotMap.class);
56+
}
57+
58+
private static void fillToCapacity(int size, EmbeddedSlotMap map) {
59+
for (int i = 0; i < size; ++i) {
60+
map.add(null, new Slot(Integer.toString(i), i, 0));
61+
}
62+
}
63+
64+
private void assertPromotes(
65+
Supplier<SlotMap> slotMapSupplier, Class<? extends SlotMap> expectedClass) {
66+
ScriptableObject obj = new TestScriptableObject();
67+
obj.setMap(slotMapSupplier.get());
68+
69+
// Add one more slot to ensure the promotion
70+
obj.put("xxx", obj, "one more property");
71+
72+
assertEquals(expectedClass, obj.getMap().getClass());
73+
}
74+
75+
private static class TestScriptableObject extends ScriptableObject {
76+
public String getClassName() {
77+
return "foo";
78+
}
79+
}
80+
}

0 commit comments

Comments
 (0)