1338884 - Harden jstests harness against changes to built-ins

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

We're currently disabling a handful of test262 tests because the tests modify built-in properties, which breaks jstests harness functions.

Comment 1

[André Bargull [:anba]]

Attachment: bug1338884-part1-move-util-functions.patch

Moves the two helper functions |toPrinted| and |XorShiftGenerator| into the IIFE. The |escapeString| function is only used for |toPrinted| and not in any test, so we don't need to export it to the global object.

Comment 2

[André Bargull [:anba]]

Attachment: bug1338884-part2-move-harness-functions.patch

Moves the remaining helper functions into the IIFE. The only functions still outside the IIFE are the various options handling functions. We can't simply move them into the IIFE because |optionsInit| is overridden in js/src/tests/browser.js, but also called during the harness start-up.

Comment 3

[André Bargull [:anba]]

Attachment: bug1338884-part3-format-and-cleanup.patch

Only fixes some style nits in the harness functions moved in part 2.

Comment 4

[André Bargull [:anba]]

Attachment: bug1338884-part4-harden-test-harness.patch

This is the main patch to harden the utility and harness functions.

Tests may override or delete String.prototype.* or RegExp.prototype.* functions, so we need to save the original values of these functions. There are extra complications for the RegExp.prototype.* functions, because of the ES6 changed the RegExp object. We could save all original RegExp.prototype.* values and create a safe RegExp subclass for the tests, but I'm not sure it's worth [1,2] it when all we need are simple shims for RegExp.prototype[@@replace] and RegExp.prototype[@@split] (resp. String.prototype.replace and String.prototype.split). 


[1] There could be too much additional overhead at harness start-up to save all original RegExp.prototype.* values and create a safer RegExp abstraction for the tests. Therefore I went with the other alternative to create custom functions to replace RegExp.prototype[@@replace] and RegExp.prototype[@@split].

[2] The replacement for RegExp.prototype[@@replace]/String.prototype.replace is only needed for the |compareSource| function, which we may be able to remove once bug 1317400 has landed.

Comment 5

[André Bargull [:anba]]

Attachment: bug1338884-part5-enable-more-tests.patch

And now we're able to run some of the previously disabled tests.

Comment 6

[Tooru Fujisawa [:arai]]

Comment on attachment 8836663 [details] [diff] [review]
bug1338884-part4-harden-test-harness.patch

Review of attachment 8836663 [details] [diff] [review]:
-----------------------------------------------------------------

Great :D

(In reply to André Bargull from comment #4)
> [2] The replacement for RegExp.prototype[@@replace]/String.prototype.replace
> is only needed for the |compareSource| function, which we may be able to
> remove once bug 1317400 has landed.

I feel most of its consumers don't need its normalization functionality, even before bug 1317400.
anyway, it's for another bug.

::: js/src/tests/shell.js
@@ +46,5 @@
>    }
>  
>    var runningInShell = typeof window === "undefined";
>  
> +  var isReftest = typeof document === "object" && /jsreftest.html/.test(document.location.href);

I wonder if there's any case that runningInShell == isReftest ...

Comment 7

[André Bargull [:anba]]

(In reply to Tooru Fujisawa [:arai] from comment #6)
> ::: js/src/tests/shell.js
> @@ +46,5 @@
> >    }
> >  
> >    var runningInShell = typeof window === "undefined";
> >  
> > +  var isReftest = typeof document === "object" && /jsreftest.html/.test(document.location.href);
> 
> I wonder if there's any case that runningInShell == isReftest ...

isReftest is probably true iff runningInBrowser is true, but I wanted to play it safe, so I didn't change the test for document.location.href. :-)

Comment 8

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c54cadd6d601368d681a9052362517598a93eec

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d68d0fe2e446
Part 1: Move the remaining helper functions into shell.js' IIFE. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd0adb13317
Part 2: Move the remaining harness functions into shell.js' IIFE. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/e09e5519427f
Part 3: Fix style nits in moved harness functions. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/22f1e0f42f6f
Part 4: Harden jstests harness for test262 which modify built-ins. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/318e6097b095
Part 5: Enable test262 tests previously disable due to missing jstests harness hardening. r=arai

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d68d0fe2e446
https://hg.mozilla.org/mozilla-central/rev/1cd0adb13317
https://hg.mozilla.org/mozilla-central/rev/e09e5519427f
https://hg.mozilla.org/mozilla-central/rev/22f1e0f42f6f
https://hg.mozilla.org/mozilla-central/rev/318e6097b095