Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compatibility with Hardened JavaScript #4088

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

phoddie
Copy link

@phoddie phoddie commented May 18, 2024

This PR proposes changes to existing test262 tests to allow them to pass under Hardened JavaScript (see Secure ECMAScript proposal and Hardened JavaScript). Moddable uses Hardened JavaScript for JavaScript runtimes on resource constrained embedded devices, including those targeted by ECMA-419.

The changes fall into four groups:

  1. Replace use of new Date() with new Date(1970). Scripts running inside a Compartment cannot retrieve the current time, so new Date() throws but new Date(1970) succeeds. Very few tests need the current time, but instead simply need a Date instance.
  2. Use Object.defineProperty instead of setting existing built-in properties directly, such as toString and toValue. In Hardened JavaScript, prototypes of built-in objects are frozen. Consequently, setting properties of an instance that exist on the prototype throw (Hardened JavaScript is always in strict mode).
  3. Eliminate use of Math.random(). Scripts running inside a Compartment cannot generate random numbers. One test identified so far uses Math.random() in a way that can easily be replaced with a counter.
  4. Narrow the scope of exception tests. Consider the following
assert.throws(TypeError, () => {
  var s1 = new Date();
  s1.toString = Boolean.prototype.toString;
  s1.toString();
});

This test passes, but only because new Date() fails by throwing a TypeError. If the invocation of the Date constructor is resolved by (1) above, then the assignment to toString fails as per (2) above. The script should be modified as below to ensure that assert.throws only tests the intended statement, s1.toString(). The modified script tests the intended functionality and passes under Hardened JavaScript

var s1 = new Date(1970);
Object.defineProperty(s1, "toString", {
  value: Boolean.prototype.toString
});
assert.throws(TypeError, () => {
  s1.toString();
});

This is an initial PR to begin the process of adapting test262 for use with Hardened JavaScript. Further changes are expected, with the vast majority likely to fall into the four groups described above.

Thank you to @gibson042, @kriskowal, and @erights for their advice on this work.

@phoddie phoddie requested a review from a team as a code owner May 18, 2024 02:06
@anba
Copy link
Contributor

anba commented May 18, 2024

Using new Date(1970) is a bit confusing, because it can easily be misinterpreted as creating a Date object for the year 1970, even though it's actually creating a Date object 1970 milliseconds from the epoch January 1, 1970. Can this be replaced with new Date(0)?

@anba
Copy link
Contributor

anba commented May 18, 2024

var s1 = new Date(1970);
Object.defineProperty(s1, "toString", {
  value: Boolean.prototype.toString
});
assert.throws(TypeError, () => {
  s1.toString();
});

145913a also uses writable: true in all property descriptors. Is specifying writable: true needed for the tests or can be it be omitted?

@ljharb
Copy link
Member

ljharb commented May 18, 2024

I believe it’s necessary because the things it’s shadowing are accessors in a locked-down realm.

@phoddie
Copy link
Author

phoddie commented May 18, 2024

@anba, thanks for the review.

Using new Date(1970) is a bit confusing, because it can easily be misinterpreted as creating a Date object for the year 1970, even though it's actually creating a Date object 1970 milliseconds from the epoch January 1, 1970. Can this be replaced with new Date(0)?

I wanted an indication of the Date constructor changes made for Hardened JavaScript and chose 1970. Admittedly, it could be misleading. Is there another value that could be used for that purpose? If not, I can change them to 0.

145913a also uses writable: true in all property descriptors. Is specifying writable: true needed for the tests or can be it be omitted?

Setting writable: true is not necessary for the tests in that commit, since the property is not written after the call to defineProperty. I imagine some other tests might need it to be writable. Would you prefer to remove the writable: true in these?

@anba
Copy link
Contributor

anba commented May 28, 2024

I wanted an indication of the Date constructor changes made for Hardened JavaScript and chose 1970. Admittedly, it could be misleading. Is there another value that could be used for that purpose? If not, I can change them to 0.

The exact value doesn't matter to me, it's just important for me that there's no confusion about the milliseconds value which is passed to Date.

Setting writable: true is not necessary for the tests in that commit, since the property is not written after the call to defineProperty. I imagine some other tests might need it to be writable. Would you prefer to remove the writable: true in these?

I'd prefer to either:

  • Omit writable: true and only use value, e.g. Object.defineProperty(obj, prop, {value: 123}).
  • Or use a fully populated property descriptor, e.g. Object.defineProperty(obj, prop, {value: 123, writable: true, enumerable: true, configurable: true}).

When only writable: true is used, it indicates (at least to me) that the property needs to support write-access, but doesn't support deletion.

@phoddie
Copy link
Author

phoddie commented May 31, 2024

@anba – I've changed new Date(1970) to new Date(0) everywhere. That should invite the fewest questions moving forward.

For Object.defineProperty let's try your rule: either a minimal property descriptor with value alone or a fully populated property descriptor. For this PR, the minimal property descriptor is always enough. (I added four more files that use defineProperty in this update).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants