Prevent accidental leakage of arguments between separate calls

[tomatosalat0]

While trying to fix a different problem within Jint, I found this problem.

The problem is that the arguments array passed to execute a function internally is added back to the cache after completion. However, some functions invocations live longer than their their first call (generators, async functions [when the implementation for these gets changed]).

Its a niche problem but should be solved anyway I guess.

So imaging the following script (copied from the included new tests)

function *method() {
  log(arguments[0]);
  log(arguments[1]);
}
              
function *other() {
  log(arguments[0]);
  log(arguments[1]);
}
            
var generator1 = method(42, undefined);
var generator2 = other(10, undefined);
generator1.next();
generator2.next();

The expected output for this is:

42
undefined
10
undefined

But without this fix, it is

10
undefined
10
undefined

---

Underlying reason:

The basic execution flow is:

Line var generator1 = method(42, undefined);

1. For the expression method(42, undefined), an JsValue[] is fetched from the internal JsValueArrayPool.
2. The array gets filled with [42, undefined]
3. The call to method returns with a generator object.
4. The JsValue[] is returned to the pool

Line var generator2 = other(10, undefined);

1. Then the expression other(10, undefined) gets evaluated. For that - again - an JsValue[] is fetched from the internal JsValueArrayPool. That array however is the same instance which was used to when calling method in this particular case
2. The array is filled with [10, undefined], but this also replaces the arguments used for method() because they share the same arguments array instance.
3. This is the actual problem.

Line .next()

1. Now the generators are executed using .next() to run the log statements.
2. The generator bodies are executed and the first generator uses the wrong values because the content of its arguments array was changed.

---

Small detail. First I've had a different implementation within JintCallExpression itself but after some investigation, I saw that this was probably the wrong spot for this change. A function where bind was used has a different execution path in there (that's where the second generator test comes from) and after some digging I've realized that I would have to change a lot of places.

I've kept the test inside just in case code is moved around later to catch potential regressions.

[lahma]

Should this information be part of the cached function definition (JintFunctionDefinition.BuildState)?

[tomatosalat0]

Oh thanks for the input - I didn't know about that one. In fact, this is a better place because that way the array copying can be limited to cases where arguments is actually used - because named arguments don't suffer from the issue (as far as I see - at least I've added a test case for that as well).

I've extended the check, added test cases for the check itself and moved the code to the (hopefully) right places.

[tomatosalat0]

Seems like the build did fail with a flaky test. I've created the PR #2195 which tries to fix the flaky test.

[lahma]

Thank you for this contribution, top notch with all the test coverage! 👍🏻

[tomatosalat0]

You're welcome.