Menu
▾
▴
Fix for
Extra fix for
#1956 java.lang.StackOverflowError when div.style.length = 100
This bug report encompasses three problems caused by the same root cause.
I'll first outline the problems, then go over what I think is the cause, and then offer my suggested fix.
Problems
- Setting
lengthfield ofCSSStyleDeclarationin non-strict mode causesjava.lang.StackOverflowError. - Setting the
sizefield of aMapshould not be allowed but is possible in HtmlUnit in non-strict mode. - Setting the
lengthfield ofjavascript.host.dom.AbstractListshould not be allowed, but is possible in HtmlUnit, in both strict mode and non-strict mode.
Test case
This test is for demonstrating the behaviour in both conventional browsers and HtmlUnit.
<!DOCTYPE html>
<html>
<head>
<script>
function check(tn, func) {
try { console.log(tn + ':', func()) } catch (e) { console.log(tn + ':', e.message) }
}
function test() {
check('t1', function () { 'use strict'; var x = document.body.style; x.length = 100; return x.length })
check('t2', function () { var x = document.body.style; x.length = 100; return x.length })
check('t3', function () { 'use strict'; var x = new Map(); x.size = 100; return x.size })
check('t4', function () { var x = new Map(); x.size = 100; return x.size })
check('t5', function () { 'use strict'; var x = document.children; x.length = 100; return x.length })
check('t6', function () { var x = document.children; x.length = 100; return x.length })
}
</script>
</head>
<body>
<input type="button" onclick="test()" value="test"/>
</body>
</html>
Result in browsers
Chrome:
t1: Cannot assign to read only property 'length' of object '#<CSSStyleDeclaration>'
t2: 0
t3: Cannot set property size of #<Map> which has only a getter
t4: 0
t5: Cannot assign to read only property 'length' of object '#<HTMLCollection>'
t6: 1
Firefox:
Odd lines differ as follows:
t1: setting getter-only property "length"
t3: setting getter-only property "size"
t5: setting getter-only property "length"
Edge:
Odd lines differ as follows:
t1: Assignment to read-only properties is not allowed in strict mode
t3: Assignment to read-only properties is not allowed in strict mode
t5: Assignment to read-only properties is not allowed in strict mode
Result in HtmlUnit
t1: Cannot set property [CSSStyleDeclaration].length that has only a getter to 100.
t3: Cannot set property [Map].size that has only a getter to 100.
t4: 100.0
t5. 100.0
t6. 100.0
t2 doesn't complete but instead throws java.lang.StackOverflowException.
Possible cause
1) The problem appears to be caused foremost by HtmlUnit's rhino fork attempting to setValue() in a ScriptableObject$GetterSlot that has no setter.
As to why it's doing that, I think it's partially caused by a regression in this commit. This commit message says "Remove unused parameters" but it appears to be removing it in the logically opposite manner for the three parameters FEATURE_HTMLUNIT_ASK_OBJECT_TO_WRITE_READONLY, FEATURE_HTMLUNIT_FUNCTION_NULL_SETTER and FEATURE_HTMLUNIT_EVAL_LOCAL_SCOPE.
For example, the literal value of FEATURE_HTMLUNIT_ASK_OBJECT_TO_WRITE_READONLY was false. So logically, "if (FEATURE_HTMLUNIT_ASK_OBJECT_TO_WRITE_READONLY) { ... } else { ... }" blocks should leave the "else" part. The commit however leaves "if" part.
2) A follow up problem is that just reverting commit won't suffice as the previous code throws TypeError even in non-strict mode. Judging from surround code, it should just return true in non-strict mode.
3) Lastly, a problem that will still break t5 and t6 is this line in AbstractList.has(String, Scriptable) which makes HtmlUnit look for the length field in the object instance even though it should be looking at the prototype instance.
This simple test in Chrone / FF / Edge shows there is no "length" field on the object for document.children but there is on __proto__.
> Object.getOwnPropertyDescriptor(document.children, 'length')
< undefined
> Object.getOwnPropertyDescriptor(document.children.__proto__, 'length')
< {get: ƒ, set: undefined, enumerable: true, configurable: true}
Suggested fix
These are the fixes we're using locally.
Fix for GetterSlot.setValue(Object value, Scriptable owner, Scriptable start).
This change to ScriptableObject$GetterSlot.setValue() puts it more in line with ScriptableObject$Slot.setValue().
@@ -286,32 +286,14 @@ ScriptableObject getPropertyDescriptor(Context cx, Scriptable scope) {
@Override
boolean setValue(Object value, Scriptable owner, Scriptable start) {
if (setter == null) {
if (getter != null) {
Context cx = Context.getContext();
- if (cx.isStrictMode() ||
- // Based on TC39 ES3.1 Draft of 9-Feb-2009, 8.12.4, step 2,
- // we should throw a TypeError in this case.
- cx.hasFeature(Context.FEATURE_STRICT_MODE)) {
-
+ if (cx.isStrictMode()) {
throw ScriptRuntime.typeError3("msg.set.prop.no.setter", name, start.getClassName(), Context.toString(value));
}
- Scriptable scriptable = start;
-
- if (scriptable instanceof Delegator) {
- scriptable = ((Delegator) scriptable).getDelegee();
- }
-
- if (scriptable instanceof ScriptableObject) {
- boolean allowSetting = ((ScriptableObject) scriptable).isReadOnlySettable(name, value);
- if (!allowSetting) {
- return true;
- }
- }
- if (owner == start) {
- getter = null;
- }
+ return true;
}
} else {
Context cx = Context.getContext();
if (setter instanceof MemberBox) {
MemberBox nativeSetter = (MemberBox)setter;
This change obseletes ScriptableObject.isReadOnlySettable(), SimpleScriptable.isReadOnlySettable() and the annotation CanSetReadOnly as they become unused code. They might be salvagable but isReadOnlySettable()'s default return value is true which isn't easy to work with, and I don't know the use case.
Extra fix for AbstractList.has(final String name, final Scriptable start).
This is the extra fix to AbstractList.has(final String name, final Scriptable start) is needed for t5 and t6.
@@ -457,13 +457,10 @@ public boolean has(final String name, final Scriptable start) {
}
catch (final NumberFormatException e) {
// Ignore.
}
- if ("length".equals(name)) {
- return true;
- }
final BrowserVersion browserVersion = getBrowserVersion();
if (browserVersion.hasFeature(JS_NODE_LIST_ENUMERATE_FUNCTIONS)) {
final JavaScriptConfiguration jsConfig = getWindow().getWebWindow().getWebClient()
.getJavaScriptEngine().getJavaScriptConfiguration();
if (jsConfig.getClassConfiguration(getClassName()).getFunctionKeys().contains(name)) {
Amendment
Further testing has revealed there's a few properties in HtmlUnit that lack setters even though they're settable in browsers such as Chrome. Behaviour-wise, the fix above will "break" code that could previously set them. In strict-mode these properties are already "broken" in that they throw an exception when an attempt is made to set them.
For example, these fields in the
Windowclass have setters in Chrome but not in HtmlUnit:event,clientInformation,self,screen,external,parent,frames,length,innerWidth,outerWidth,innerHeight,outerHeight.In light of this, this is the fix I'm now using:
Ideally,
shadowablemight be best set like@JsxGetter(shadowable = true)but I did it through a quick and dirty hack.Last edit: Atsushi Nakagawa 2018-03-13
Many thanks for all thsi observations. Will require some time to fix this.
I think you are wrong here, please have a look at the HtmlUnit commit 14689. HtmlUnit had a differnet feature setup then core-js.
Ahh, you're right, thanks for explaining.
see https://github.com/HtmlUnit/htmlunit/issues/787