Fix findMatchingCatch iterating over argument array prototype chain

[luxaritas]

A bug was introduced in #19760 where when iterating over the arguments passed to findMatchingCatch, it iterates not just over the elements in the array, but all properties in the Array prototype chain (due to the semantics of for..in vs for..of). This can cause exception handling to segfault in ___cxa_can_catch, as it is not being passed a valid pointer in the case of iterating over something like a utility function added to the array prototype without marking it as enumerable.

I discovered this when debugging onnxruntime and discovering that their custom errors (which inherit from std::exception) were causing segfaults during exception handling (whereas changing to std::logic_error would work fine), and have confirmed this fix resolves that issue. However, I could use guidance on adding a test for this change.

[kripken]

I don't have a good idea for a specific test here - seems like that would require messing around with the prototype chain? But this is shorter and obviously correct, and covered by existing tests for regressions at least, lgtm.

[sbc100]

I think this warrants a little more investigation. findMatchCatching as far as I can tell should only ever be called with a JS array of pointers (numbers).

The only callers of this function are the native __cxa_find_matching_catch_X functions which all take a sequence of pointers as arguments and pass them to findMatchingCatch as [x, y, z]. I don't see how this could ever be anything other than an array numbers.

[sbc100]

Of course the change itself looks fine.. I just don't understand how you ran into needing this

[luxaritas]

I have a suspicion - give me a few minutes to investigate

[luxaritas]

Yeah I figured it out. In an old inherited portion of our codebase, custom properties are added to Array.prototype. Due to the way they're added (eg, Array.prototype.clone = ...), they're marked as enumerable.

This is a problem we should be able to solve ourselves by marking them non-enumerable. However, this change would make the code more robust to poor userland behavior ( 🥲 ) - and is arguably more readable/conventional and saves a few bytes. Not sure if there is a reason why the previous implementation decided to iterate over keys instead of values when only values are used.

[sbc100]

Yeah I figured it out. In an old inherited portion of our codebase, custom properties are added to Array.prototype. Due to the way they're added (eg, Array.prototype.clone = ...), they're marked as enumerable.

Ah yes, that makes sense. It requires some external monkey patching to mess with all array objects. Thanks for clarifying.

This is a problem we should be able to solve ourselves by marking them non-enumerable. However, this change would make the code more robust to poor userland behavior ( 🥲 ) - and is arguably more readable/conventional and saves a few bytes. Not sure if there is a reason why the previous implementation decided to iterate over keys instead of values when only values are used.

The reason for this is that until recently emscripten did not allow for .. of at all since we were still required to output valid ES5. We not use a transpiler when we need to so we allow ES6 constructs like this by default noe.