Fix assertion failure in JavascriptArray.cpp

[ShortDevelopment]

Fix #6770

[rhuanjl]

Please can you add a test case for this?

[ShortDevelopment]

Sure, but it seems like I’ve created a new assertion failure; I’ll have another look at this…

[ShortDevelopment]

Problem is, that the assertion error only occurred with extremely large arrays.
Using the poc of the mentioned issue causes the test to timeout.

[rhuanjl]

The timeouts are on a per JS file basis - does it still timeout if this test is done in a separate file with nothing else?

[ppenzin]

For the timeout, is there a better way to go about the test? The array copy of such size if going to be slow.

[rhuanjl]

Could the test complete in 3 minutes? Normally each test file gets a 1 min timeout but tests run with the Slow tag get 3 minutes instead (and are limited to only a couple of the test builds though that's not the point here).

If even 3 minutes is too short I don't know what to do - an Assert we can't test seems pointless. @ppenzin thoughts?

[ppenzin]

I agree, we should try to run it as a separate test and mark as slow (given it does finish in 3 minutes).

[ppenzin]

I took a peek at the test - I think the key is separating this test from the reset of ES6 Array API tests by putting it into a separate JS file that would be marked as Slow. I've done that and the testing succeeded, though do we actually run these slow files? Question to @rhuanjl, I think. Both of my runs (debug and release with debug info) produced "exclude: slow" in the output.

[rhuanjl]

I took a peek at the test - I think the key is separating this test from the reset of ES6 Array API tests by putting it into a separate JS file that would be marked as Slow. I've done that and the testing succeeded, though do we actually run these slow files? Question to @rhuanjl, I think. Both of my runs (debug and release with debug info) produced "exclude: slow" in the output.

You have to use a flag to run slow tests. Currently we do this on two of the windows CI builds x86 test and x64 test.

[ShortDevelopment]

I should note I never actually checked whether the assertion was just a left-over from a refactoring or a valid check for the following code.

Edit: The fast path uses uint32 as indices, so the asserts have to ensure no overflow occurs.

[ppenzin]

This looks good in my opinion, though needs a rebase. MSVC CI failure is very likely due to the patch being based on an older version which doesn't have the stringier macro change.

[ShortDevelopment]

@ppenzin
It seems like the macro change hasn't been merged yet (See #6970).
I gues their checks were failing because of a wrong copyright notice...

[ppenzin]

I guess it is time for mass update to PAL copyright headers...

[ppenzin]

@ShortDevelopment did you catch if these were the new tests failing? Azure link sometimes opens and sometimes doesn't for me

[ShortDevelopment]

@ppenzin Yes. Sadly the tests still timeout.
I'm not sure whether I did apply the slow flag correctly...

[ppenzin]

I've tried that on Windows sometime ago, but can't remember how I did it, need to revisit.