ArrayIndexOutOfBoundsException - EmbeddedSlotMap?

[tmallery]

I'm seeing an ArrayIndexOutOfBoundsException in ScriptableObject.getIds(boolean, boolean) because the EmbeddedSlotMap iterator is returning one more result than its count, "map.size()", is returning.
Has anyone else seen this? I haven't found the cause yet so I can't make a test case, I only see the error after a lot of processing has happened so I don't yet know the sequence of events that causes this problem. However I tried removing EmbeddedSlotMap by having SlopMapContainer just make HashSlotMap and that "fixed" this issue.

This also might be caused by a fix I'm currently doing to allow Object.getOwnPropertyDescriptor("z", 0) and "z".propertyIsEnumerable(0) to work. I'll provide more information as I find it.

[gbrail]

Can you share the stack trace? I hope we didn't miss an edge case -- certainly across lots of product usage and tests I haven't -- but anything is possible and a stack trace would help a lot!

…

On Wed, Feb 14, 2018 at 9:05 AM, tmallery ***@***.***> wrote: I'm seeing an ArrayIndexOutOfBoundsException in ScriptableObject.getIds(boolean, boolean) because the EmbeddedSlotMap iterator is returning one more result than its count, "map.size()", is returning. Has anyone else seen this? I haven't found the cause yet so I can't make a test case, I only see the error after a lot of processing has happened so I don't yet know the sequence of events that cause this problem. However I tried removing EmbeddedSlotMap by having SlopMapContainer just make HashSlotMap and that "fixed" this issue. This also might be caused by a fix I'm currently doing to allow Object.getOwnPropertyDescriptor("z", 0) and "z".propertyIsEnumerable(0) to work. I'll provide more information as I find it. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#390>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAf0a5ukFV7kIqxqncXObCo3oay6DBunks5tUxJzgaJpZM4SFp73> .

[tmallery]

ArrayIndexOutOfBoundsException: 231 (evaluateExpression_39102403#1)
org.mozilla.v17.javascript.WrappedException: Wrapped java.lang.ArrayIndexOutOfBoundsException: 231 (evaluateExpression_39102403#1)
at org.mozilla.v17.javascript.Context.throwAsScriptRuntimeEx(Context.java:1924)
at org.mozilla.v17.javascript.MemberBox.invoke(MemberBox.java:148)
at org.mozilla.v17.javascript.FunctionObject.call(FunctionObject.java:452)
at org.mozilla.v17.javascript.Interpreter.interpretLoop(Interpreter.java:1492)
at script(evaluateExpression_39102403:1)
at org.mozilla.v17.javascript.Interpreter.interpret(Interpreter.java:823)
at org.mozilla.v17.javascript.InterpretedFunction.call(InterpretedFunction.java:109)
at org.mozilla.v17.javascript.ContextFactory.doTopCall(ContextFactory.java:402)
at org.mozilla.v17.javascript.ScriptRuntime.doTopCall(ScriptRuntime.java:3514)
at org.mozilla.v17.javascript.InterpretedFunction.exec(InterpretedFunction.java:120)
at org.mozilla.v17.javascript.Context.evaluateString(Context.java:1254)
Caused by: java.lang.ArrayIndexOutOfBoundsException: 231
at org.mozilla.v17.javascript.ScriptableObject.getIds(ScriptableObject.java:2933)
at org.mozilla.v17.javascript.NativeObject.execIdCall(NativeObject.java:306)
at org.mozilla.v17.javascript.IdFunctionObject.call(IdFunctionObject.java:106)
at org.mozilla.v17.javascript.Interpreter.interpretLoop(Interpreter.java:1492)
at org.mozilla.v17.javascript.Interpreter.interpret(Interpreter.java:823)
at org.mozilla.v17.javascript.InterpretedFunction.call(InterpretedFunction.java:109)

[tmallery]

I haven't created a test case yet, but I believe I found the issue. EmbeddedSlotMap.createSlot when there is an existing slot. The code adds the new slot to the linked list as if it were a brand new slot, but its not really, it's replacing the existing slot. The code does not remove the existing node from the linked list before adding the new one. I did a quick and dirty test and it works.
// remove old entry from the linked list
ScriptableObject.Slot temp = firstAdded;
ScriptableObject.Slot orderedPrev = null;
while( temp != null ) {
if( temp == slot ) {
if( orderedPrev != null ) {
orderedPrev.orderedNext = slot.orderedNext;
}
break;
}
orderedPrev = temp;
temp = temp.orderedNext;
}

// add new slot to linked list
......

[tmallery]

I'll throw this into a test case, but here's the JS to reproduce the ArrayIndexoutofBoundsException
var obj = new Object();
Object.defineProperty(obj, 'aProp', {configurable:true, get:function() {return 1;}, set:function(arg){ }});" +
Object.defineProperty(obj, 'aProp', {value:42});
if( Object.getOwnPropertyNames(obj).length != 1 ) { result = 'success'; } result;

[gbrail]

You are exactly right -- Thanks! That is a nasty error in a complicated bit of code.

I just submitted a PR -- can you take a look?

[gbrail]

Fixed in #391.

[raimi]

Aw man. This ".0" behaviour has been bothering us forever. We have workaround-code at our end to find the integer from the floting point number again.
I'm also a little worried about the performance impact of that extra conversion-check - but the overall behaviour becomes better and less surprising.

I handle way more integers than floating point numbers in my programs...

[gbrail]

So are you suggesting that the fix for the ".0" Integer translation is something we should enable by default? I cautiously asked for the opposite but we can always change it later, or for language level. We broke backward compatibility in the recent string of releases (with a "strict mode" feature that we since put under a language version check) and I am sensitive to doing it again.

…

On Thu, Mar 15, 2018 at 12:21 AM, raimi ***@***.***> wrote: Aw man. This ".0" behaviour has been bothering us forever. We have workaround-code at our end to find the integer from the floting point number again. I'm also a little worried about the performance impact of that extra conversion-check - but the overall behaviour becomes better and less surprising. I handle way more integers than floating point numbers in my programs... — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#390 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAf0a159uuFB1GOZu16ti6VHwSdvR_OEks5tehZlgaJpZM4SFp73> .

[raimi]

No no no, do not enable it by default (yet). I also think it may break things for too many people.
But with the optional feature available, we could start to experiment with it.

[phil15world988]

the best