Bound functions must not leak global object into strict functions

[szegedi]

Fixes #442

[gbrail]

I meant to leave this comment on the PR but I left it on the Issue instead:

This looks like an important fix.

We have integration with test262 in the test suite now via a git submodule -- could you take a quick look and see if there are any tests in there that we could enable (by editing testsrc/test262.properties)? It'd be nice to have an independent test to verify this behavior.

[szegedi]

language/function-code/10.4.3-1-77gs.js tests exactly this. The test doesn't seem excluded to me, so I'm not sure if this is indicative of the test suite failing to detect failures? Surely if I run this from command line with:

java -jar ./buildGradle/libs/rhino-1.7.11-SNAPSHOT.jar test262/test/language/function-code/10.4.3-1-77gs.js

on master it fails with:

js: "test262/test/language/function-code/10.4.3-1-77gs.js", line 13: exception from uncaught JavaScript throw: 'this' had incorrect value!

While in the branch of this PR it silently passes.

[gbrail]

Sorry to open a can of worms but I think it's important to understand deep stuff like this...

All of the "language/function-object" tests in test262 are disabled, so I added a line to test262.properties to enable them. When I did that, we had 423 test failures, all in that suite (we run each test three times so that's 141 test cases), and with this change there are 429 (143 failing test cases).

I mucked around in the XML for a while and I might have misinterpreted some of the output, but it looks like this change fixes:

• 10.4.3-1-77-s
• 10.4.3-1-77gs

and it introduces new failures in these:

• 10.4.3-1-97gs
• 10.4.3-1-95s
• 10.4.3-1-96gs
• 10.4.3-1-97-s
• 10.4.3-1-95gs
• 10.4.3-1-96-s

Since you're in the middle of this, perhaps you'll be able to see what's going on?

[szegedi]

I completely agree. I misunderstood the test262.properties file at first, now I understand that's it an include/exclude file, so if language/function-code wasn't specified, it wasn't included at all. You're right that those new failures should be looked into; I will do so. The gist of those failures is that the substitution of null for global (for non-strict functions) should be handled by the target function invocation; it seems like for some reason in these cases it isn't. So it's possible that that early substitution in BoundFunction was masking the lack of a necessary substitution somewhere downstream; I'll investigate and add it to this PR.

[szegedi]

FWIW, we could probably cut down the test suite size by a factor of 2: for every test case, there's both a …-s.js and a …gs.js file. Their logic seems identical, so running both seems unnecessary. The -s ones use the "assert" function while the gs ones basically inline it (so they run even when "assert" is not defined). Filed this as #444.

[szegedi]

Just a heads up that I've spent some more time on this, and have a version that fixes more of the cases. It turned out to be a harder issue than I expected, so still needs some more work.

[rbri]

@szegedi maybe you can push you current version of this, so others can try to improve this further

[szegedi]

Pushed my additional pending changes on request from @rbri; with the understanding that even with this second commit it still doesn't fix all the cases.

[p-bakker]

Converted to draft as this isn't mergable in it's current state

Might be a useful basis to work on better strict-mode support in the future or be replaced by another PR autoverkeer