Menu
▾
▴
[HtmlUnit] [htmlunit:bugs] #1956 java.lang.StackOverflowError when div.style.length = 100
|
From: RBRi <rb...@us...> - 2024-05-14 11:46:27
|
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. |
