Bug 1897150 Comment 20 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

This is a bug in the new trampoline-based implementation of array sort.

When calling a comparator function during an array sort, `this` should always be `undefined` ([step 4 here](https://tc39.es/ecma262/#sec-comparearrayelements)), which will be replaced with the global in non-strict mode ([step 6 here](https://tc39.es/ecma262/#sec-ordinarycallbindthis)). In the jitcode trampoline, we preallocate space on the stack for the arguments to the comparator function, including `this`. The intention was that we could save a little bit of time by effectively preallocating part of the call frame, and reusing it across calls.

We already had issues in bug 1888746 reusing the callee token because we weren't clearing the stale HASCACHEDSAVEDFRAME_BIT. In this case, we're having issues because [InvokeFromInterpreterStub](https://searchfox.org/mozilla-central/rev/a18a7c526cf3c531f2fc24db4f0dffbc16290a7e/js/src/jit/VMFunctions.cpp#576-577), which we call if the comparator function hasn't tiered up yet, stores the return value in `argv[0]`, which is to say the `this` slot.

We never reset the value of `this` after initializing it to UndefinedValue, so we end up calling the comparator with `this` value equal to the return value of the previous call. Most comparators don't use `this`, but in this case the code is using an explicit `this` to access global variables.

The best fix is probably to store UndefinedValue on each call, the same way we currently store the callee token.
This is a bug in the new trampoline-based implementation of array sort.

When calling a comparator function during an array sort, `this` should always be `undefined` ([step 4 here](https://tc39.es/ecma262/#sec-comparearrayelements)), which will be replaced with the global in non-strict mode ([step 6 here](https://tc39.es/ecma262/#sec-ordinarycallbindthis)). In the jitcode trampoline, we preallocate space on the stack for the arguments to the comparator function, including `this`. The intention was that we could save a little bit of time by effectively preallocating part of the call frame, and reusing it across calls.

We already had issues in bug 1888746 reusing the descriptor because we weren't clearing the stale HASCACHEDSAVEDFRAME_BIT. In this case, we're having issues because [InvokeFromInterpreterStub](https://searchfox.org/mozilla-central/rev/a18a7c526cf3c531f2fc24db4f0dffbc16290a7e/js/src/jit/VMFunctions.cpp#576-577), which we call if the comparator function hasn't tiered up yet, stores the return value in `argv[0]`, which is to say the `this` slot.

We never reset the value of `this` after initializing it to UndefinedValue, so we end up calling the comparator with `this` value equal to the return value of the previous call. Most comparators don't use `this`, but in this case the code is using an explicit `this` to access global variables.

The best fix is probably to store UndefinedValue on each call, the same way we currently store the callee token.

Back to Bug 1897150 Comment 20