[HtmlUnit] [htmlunit:bugs] #1956 java.lang.StackOverflowError when div.style.length = 100
RBRi via HtmlUnit-develop <[email protected]>
| Newsgroups | gmane.comp.java.htmlunit.devel |
|---|---|
| Message-ID | </p/htmlunit/bugs/1956/8c13034c902c1db8125df2c8415b039f76a03dfc.bugs@htmlunit.p.sourceforge.net> |
- **status**: open --> accepted
- **assigned_to**: RBRi
---
** [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 Mar 17, 2018 04:53 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 [email protected] 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.
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
HtmlUnit-develop mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/htmlunit-develop