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

SlotMap.compute can lead to deadlock in ThreadSafeSlotMapContainer #1746

Merged
merged 10 commits into from
Dec 11, 2024
Merged

SlotMap.compute can lead to deadlock in ThreadSafeSlotMapContainer #1746

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d69afa8
adding PropertyTest redefinePropertyWithThreadSafeSlotMap as a repro …
nabacg Dec 2, 2024
466c6e8
adding minimal Deadlock repro case
nabacg Dec 2, 2024
0ba1f30
spotlessApply
nabacg Dec 2, 2024
8681b19
Change to prevent creation of property descriptors when not required.
aardvark179 Dec 2, 2024
0f4e856
Refactor `IdScriptableObject` to avoid building property descriptors.
aardvark179 Dec 2, 2024
c14ec1c
Minor style change round if...else cases with `return`.
aardvark179 Dec 3, 2024
ad7d0a6
Change `getBuiltInDescriptor` to `getBuiltInDataDescriptor` for clarity.
aardvark179 Dec 3, 2024
e20ecb0
moving all getProperty out of slotMap.compute lambda inside Scriptabl…
nabacg Dec 4, 2024
d350e81
refactoring LambdaAccessorSlot and corresponding ScriptableObject::de…
nabacg Dec 5, 2024
474c3ba
adding redefine existing property test to LambdaAccessorSlotTests to …
nabacg Dec 9, 2024
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
48 changes: 48 additions & 0 deletions rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,34 @@ Function getGetterFunction(String name, Scriptable scope) {
return getter.asGetterFunction(name, scope);
}

@Override
boolean isSameGetterFunction(Object function) {
if (function == Scriptable.NOT_FOUND) {
return true;
}
if (getter == null) {
return ScriptRuntime.shallowEq(Undefined.instance, function);
}
return getter.isSameGetterFunction(function);
}

@Override
boolean isSameSetterFunction(Object function) {
if (function == Scriptable.NOT_FOUND) {
return true;
}
if (setter == null) {
return ScriptRuntime.shallowEq(Undefined.instance, function);
}
return setter.isSameSetterFunction(function);
}

interface Getter {
Object getValue(Scriptable start);

Function asGetterFunction(final String name, final Scriptable scope);

boolean isSameGetterFunction(Object getter);
}

/** This is a Getter that delegates to a Java function via a MemberBox. */
Expand All @@ -139,6 +163,11 @@ public Object getValue(Scriptable start) {
public Function asGetterFunction(String name, Scriptable scope) {
return member.asGetterFunction(name, scope);
}

@Override
public boolean isSameGetterFunction(Object function) {
return member.isSameGetterFunction(function);
}
}

/** This is a getter that delegates to a JavaScript function. */
Expand All @@ -164,12 +193,20 @@ public Object getValue(Scriptable start) {
public Function asGetterFunction(String name, Scriptable scope) {
return target instanceof Function ? (Function) target : null;
}

@Override
public boolean isSameGetterFunction(Object function) {
return ScriptRuntime.shallowEq(
target instanceof Function ? (Function) target : Undefined.instance, function);
}
}

interface Setter {
boolean setValue(Object value, Scriptable owner, Scriptable start);

Function asSetterFunction(final String name, final Scriptable scope);

boolean isSameSetterFunction(Object getter);
}

/** Invoke the setter on this slot via reflection using MemberBox. */
Expand Down Expand Up @@ -202,6 +239,11 @@ public boolean setValue(Object value, Scriptable owner, Scriptable start) {
public Function asSetterFunction(String name, Scriptable scope) {
return member.asSetterFunction(name, scope);
}

@Override
public boolean isSameSetterFunction(Object function) {
return member.isSameSetterFunction(function);
}
}

/**
Expand All @@ -228,5 +270,11 @@ public boolean setValue(Object value, Scriptable owner, Scriptable start) {
public Function asSetterFunction(String name, Scriptable scope) {
return target instanceof Function ? (Function) target : null;
}

@Override
public boolean isSameSetterFunction(Object function) {
return ScriptRuntime.shallowEq(
target instanceof Function ? (Function) target : Undefined.instance, function);
}
}
}
82 changes: 63 additions & 19 deletions rhino/src/main/java/org/mozilla/javascript/IdScriptableObject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -867,8 +867,8 @@ protected void defineOwnProperty(
delete(id); // it will be replaced with a slot
} else {
checkPropertyDefinition(desc);
ScriptableObject current = getOwnPropertyDescriptor(cx, key);
checkPropertyChange(name, current, desc);
var slot = queryOrFakeSlot(cx, key);
checkPropertyChangeForSlot(name, slot, desc);
int attr = (info >>> 16);
Object value = getProperty(desc, "value");
if (value != NOT_FOUND && ((attr & READONLY) == 0 || (attr & PERMANENT) == 0)) {
Expand All @@ -877,7 +877,12 @@ protected void defineOwnProperty(
setInstanceIdValue(id, value);
}
}
attr = applyDescriptorToAttributeBitset(attr, desc);
attr =
applyDescriptorToAttributeBitset(
attr,
getProperty(desc, "enumerable"),
getProperty(desc, "writable"),
getProperty(desc, "configurable"));
setAttributes(name, attr);
return;
}
Expand All @@ -889,8 +894,8 @@ protected void defineOwnProperty(
prototypeValues.delete(id); // it will be replaced with a slot
} else {
checkPropertyDefinition(desc);
ScriptableObject current = getOwnPropertyDescriptor(cx, key);
checkPropertyChange(name, current, desc);
var slot = queryOrFakeSlot(cx, key);
checkPropertyChangeForSlot(name, slot, desc);
int attr = prototypeValues.getAttributes(id);
Object value = getProperty(desc, "value");
if (value != NOT_FOUND && (attr & READONLY) == 0) {
Expand All @@ -900,7 +905,12 @@ protected void defineOwnProperty(
}
}
prototypeValues.setAttributes(
id, applyDescriptorToAttributeBitset(attr, desc));
id,
applyDescriptorToAttributeBitset(
attr,
getProperty(desc, "enumerable"),
getProperty(desc, "writable"),
getProperty(desc, "configurable")));

// Handle the regular slot that was created if this property was previously
// replaced
Expand All @@ -922,62 +932,96 @@ protected ScriptableObject getOwnPropertyDescriptor(Context cx, Object id) {
ScriptableObject desc = super.getOwnPropertyDescriptor(cx, id);
if (desc == null) {
if (id instanceof String) {
return getBuiltInDescriptor((String) id);
return getBuiltInDataDescriptor((String) id);
}

if (ScriptRuntime.isSymbol(id)) {
if (id instanceof SymbolKey) {
return getBuiltInDescriptor((SymbolKey) id);
return getBuiltInDataDescriptor((SymbolKey) id);
}

return getBuiltInDescriptor(((NativeSymbol) id).getKey());
return getBuiltInDataDescriptor(((NativeSymbol) id).getKey());
}
}
return desc;
}

private ScriptableObject getBuiltInDescriptor(String name) {
Object value = null;
int attr = EMPTY;
private Slot queryOrFakeSlot(Context cx, Object id) {
var slot = querySlot(cx, id);
if (slot == null) {
if (id instanceof String) {
return getBuiltInSlot((String) id);
}

if (ScriptRuntime.isSymbol(id)) {
if (id instanceof SymbolKey) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, looking at coverage, nothing from line 957 to 961 seems to get covered either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is a little more convoluted: as of right now, I think it's impossible for this code path to be executed, thus it will be challenging to test it. This is because it's only called from IdScriptableObject::defineOwnProperty ( here and here) and before the call, we've already established that key must be a CharSequence, so it can't a Symbol. So maybe the solutions is to remove this branch?

Having said that, this doesn't feel right and perhaps it's an issue with current implementation of IdScriptableObject::defineOwnProperty, since it's only handling String (or CharSequence) keys and essentially delegating Symbol key'ed properties to it's base class (ScriptableObject). It's very consistent with how getOwnPropertyDescriptor implementation, which does try to find built in descriptor using both Strings and Symbols (here ). I believe this currently works mostly, because we always check base class for property descriptor first, but it's a little confusing why we need that symbol lookup.

One could also imagine, someday in the future, someone creating a new built-in NativeX class extending IdScriptableObject that defines a Symbol keyed property, that's shouldn't be redefined ( i.e. configurable: false ) and current implementation of IdScriptableObject::defineOwnProperty would not check it ( only Strings are handled) and allow base class implementation to override it.

I appreciate it's a little theoretical, but wanted to share this and ask for opinions on whether we should address this? Also if addressing this deserves a separate PR or should I add it here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, thanks -- I suppose one way we could deal with this could be to assert, and in the future if someone wants to implement a subclass that needs it, we'd fix it -- although in many cases we are trying to get rid of IdScriptableObject as it doesn't yield the performance benefit it once did. If you're not comfortable with that, however, I'd also be OK leaving this as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally wouldn't mind leaving it as is for now, I don't see an obvious place to assert that wouldn't break existing functionality or be completely redundant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, then I'm satisfied, thanks!

return getBuiltInSlot((SymbolKey) id);
}

return getBuiltInSlot(((NativeSymbol) id).getKey());
}
}
return slot;
}

private ScriptableObject getBuiltInDataDescriptor(String name) {
Scriptable scope = getParentScope();
if (scope == null) {
scope = this;
}

var slot = getBuiltInSlot(name);
return slot == null ? null : buildDataDescriptor(scope, slot.value, slot.getAttributes());
}

private Slot getBuiltInSlot(String name) {
Object value = null;
int attr = EMPTY;

int info = findInstanceIdInfo(name);
if (info != 0) {
int id = (info & 0xFFFF);
value = getInstanceIdValue(id);
attr = (info >>> 16);
return buildDataDescriptor(scope, value, attr);
var slot = new Slot(name, 0, attr);
slot.value = value;
return slot;
}
if (prototypeValues != null) {
int id = prototypeValues.findId(name);
if (id != 0) {
value = prototypeValues.get(id);
attr = prototypeValues.getAttributes(id);
return buildDataDescriptor(scope, value, attr);
var slot = new Slot(name, 0, attr);
slot.value = value;
return slot;
}
}
return null;
}

private ScriptableObject getBuiltInDescriptor(Symbol key) {
Object value = null;
int attr = EMPTY;

private ScriptableObject getBuiltInDataDescriptor(Symbol key) {
Scriptable scope = getParentScope();
if (scope == null) {
scope = this;
}

var slot = getBuiltInSlot(key);
return slot == null ? null : buildDataDescriptor(scope, slot.value, slot.getAttributes());
}

private Slot getBuiltInSlot(Symbol key) {
Object value = null;
int attr = EMPTY;

if (prototypeValues != null) {
int id = prototypeValues.findId(key);
if (id != 0) {
value = prototypeValues.get(id);
attr = prototypeValues.getAttributes(id);
return buildDataDescriptor(scope, value, attr);
var slot = new Slot(key, 0, attr);
slot.value = value;
return slot;
}
}
return null;
Expand Down
20 changes: 19 additions & 1 deletion rhino/src/main/java/org/mozilla/javascript/LambdaAccessorSlot.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,17 @@ boolean isSetterSlot() {

@Override
ScriptableObject getPropertyDescriptor(Context cx, Scriptable scope) {
ScriptableObject desc = (ScriptableObject) cx.newObject(scope);
return buildPropertyDescriptor(cx);
}

/**
* The method exists avoid changing the getPropertyDescriptor signature and at the same time to
* make it explicit that we don't use Scriptable scope parameter of getPropertyDescriptor, since
* it can be problematic when called from inside ThreadSafeSlotMapContainer::compute lambda
* which can lead to deadlocks.
*/
public ScriptableObject buildPropertyDescriptor(Context cx) {
ScriptableObject desc = new NativeObject();

int attr = getAttributes();
boolean es6 = cx.getLanguageVersion() >= Context.VERSION_ES6;
Expand Down Expand Up @@ -126,4 +136,12 @@ public void setSetter(Scriptable scope, BiConsumer<Scriptable, Object> setter) {
});
}
}

public void replaceWith(LambdaAccessorSlot slot) {
this.getterFunction = slot.getterFunction;
this.getter = slot.getter;
this.setterFunction = slot.setterFunction;
this.setter = slot.setter;
setAttributes(slot.getAttributes());
}
}
10 changes: 10 additions & 0 deletions rhino/src/main/java/org/mozilla/javascript/MemberBox.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,16 @@ public String toString() {
return memberObject.toString();
}

boolean isSameGetterFunction(Object function) {
var f = asGetterFunction == null ? Undefined.instance : asGetterFunction;
return ScriptRuntime.shallowEq(function, f);
}

boolean isSameSetterFunction(Object function) {
var f = asSetterFunction == null ? Undefined.instance : asSetterFunction;
return ScriptRuntime.shallowEq(function, f);
}

/** Function returned by calls to __lookupGetter__ */
Function asGetterFunction(final String name, final Scriptable scope) {
// Note: scope is the scriptable this function is related to; therefore this function
Expand Down
Loading
Loading