[HtmlUnit] [htmlunit:bugs] #1956 java.lang.StackOverflowError when div.style.length = 100

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 422-6466

see https://github.com/HtmlUnit/htmlunit/issues/787


---

**[bugs:#1956] java.lang.StackOverflowError when div.style.length = 100**

**Status:** accepted
**Group:** Latest SVN
**Created:** Mon Mar 12, 2018 04:04 AM UTC by Atsushi Nakagawa
**Last Updated:** Sat Jun 30, 2018 12:51 PM UTC
**Owner:** RBRi


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 `length` field of `CSSStyleDeclaration` in *non-strict mode* causes `java.lang.StackOverflowError`.
* Setting the `size` field of a `Map` should not be allowed but is possible in HtmlUnit in *non-strict mode*.
* Setting the `length` field of `javascript.host.dom.AbstractList` should 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.

```html
<!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:

```text
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:

```text
t1: setting getter-only property "length"
t3: setting getter-only property "size"
t5: setting getter-only property "length"
```

#### Edge:

Odd lines differ as follows:

```text
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

```text
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](https://github.com/HtmlUnit/htmlunit-rhino-fork/blob/f41c211235de141394bff2ca1bf9ee02dfc3e58f/src/org/mozilla/javascript/ScriptableObject.java#L271).

As to *why* it's doing that, I think it's partially caused by a regression in [this commit](https://github.com/HtmlUnit/htmlunit-rhino-fork/commit/d01bf85dd0149d73e6c057d1c64c0fa2b780daae).  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](https://github.com/HtmlUnit/htmlunit-rhino-fork/blob/f41c211235de141394bff2ca1bf9ee02dfc3e58f/src/org/mozilla/javascript/ScriptableObject.java#L2736), 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)`](https://sourceforge.net/p/htmlunit/code/15163/tree/trunk/htmlunit/src/main/java/com/gargoylesoftware/htmlunit/javascript/host/dom/AbstractList.java#l468) 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__`.

```text
> 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()`](https://github.com/HtmlUnit/htmlunit-rhino-fork/blob/f41c211235de141394bff2ca1bf9ee02dfc3e58f/src/org/mozilla/javascript/ScriptableObject.java#L260) puts it more in line with [`ScriptableObject$Slot.setValue()`]( https://github.com/HtmlUnit/htmlunit-rhino-fork/blob/f41c211235de141394bff2ca1bf9ee02dfc3e58f/src/org/mozilla/javascript/ScriptableObject.java#L180).

```diff
@@ -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()`](https://github.com/HtmlUnit/htmlunit-rhino-fork/blob/f41c211235de141394bff2ca1bf9ee02dfc3e58f/src/org/mozilla/javascript/ScriptableObject.java#L3288), [`SimpleScriptable.isReadOnlySettable()`](https://sourceforge.net/p/htmlunit/code/15163/tree/trunk/htmlunit/src/main/java/com/gargoylesoftware/htmlunit/javascript/SimpleScriptable.java#l392) and the annotation [`CanSetReadOnly`](https://sourceforge.net/p/htmlunit/code/15163/tree/trunk/htmlunit/src/main/java/com/gargoylesoftware/htmlunit/javascript/configuration/CanSetReadOnly.java) 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)`](https://sourceforge.net/p/htmlunit/code/15163/tree/trunk/htmlunit/src/main/java/com/gargoylesoftware/htmlunit/javascript/host/dom/AbstractList.java#l455) is needed for t5 and t6.

```diff
@@ -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)) {
```


---

Sent from sourceforge.net because htm...@li... is subscribed to https://sourceforge.net/p/htmlunit/bugs/

To unsubscribe from further messages, a project admin can change settings at https://sourceforge.net/p/htmlunit/admin/bugs/options.  Or, if this is a mailing list, you can unsubscribe from the mailing list.

[HtmlUnit] [htmlunit:bugs] #1956 java.lang.StackOverflowError when div.style.length = 100

Java GUI-Less browser, supporting JavaScript, to run against web pages

[HtmlUnit] [htmlunit:bugs] #1956 java.lang.StackOverflowError when div.style.length = 100