fix(webidl): do not throw if Object.prototype has been mutated

[aduh95]

Adds a test that ensures that "weird" value for the FilePropertyBag options parameter are handled as in the browsers (I tested on Firefox, Safari, and Chromium), and fixes a bug that made Deno throw with:

Uncaught TypeError: Cannot set property lastModified of #<Object> which has only a getter
    at Array.FilePropertyBag (deno:ext/webidl/00_webidl.js:713:24)
    at new File (deno:ext/web/09_file.js:461:53)
    at <anonymous>:56:24
    at Array.forEach (<anonymous>)
    at <anonymous>:55:3

Code that I used to test on the browser

Same as in the PR, but in JS rather than TS:

const assert = {
  strictEqual(actual, expected) {
    if (actual !== expected) throw new Error(`Expected ${expected}, got ${actual}`);
  },
  throws(fn, expected) {
    try {
      fn();
      throw new Error('Missing expected exception');
    } catch (e) {
      if (e instanceof expected) {
        return;
      }

      throw new Error('Wrong type of error', { cause: e });
    }
  },
};

function mustCall(fn, nb = 1) {
  const timeout = setTimeout(() => {
    if (nb !== 0) throw new Error(`Expected ${nb} more calls`);
  }, 999);
  return function() {
    nb--;
    if (nb === 0) clearTimeout(timeout);
    else if (nb < 0) {
      throw new Error('Function has been called more times than expected');
    }
    return Reflect.apply(fn, this, arguments);
  };
}

[undefined, null, Object.create(null), { lastModified: undefined }, {
  get lastModified() {},
}].forEach((options) => {
  assert.strictEqual(
    new File([], null, options).lastModified,
    new File([], null).lastModified,
  );
});

Reflect.defineProperty(Object.prototype, 'get', {
  __proto__: null,
  configurable: true,
  get() {
    throw new Error();
  },
});
Reflect.defineProperty(Object.prototype, 'lastModified', {
  __proto__: null,
  configurable: true,
  get: mustCall(() => 3, 7),
});

[{}, [], () => {}, Number, new Number(), new String(), new Boolean()].forEach(
  (options) => {
    assert.strictEqual(new File([], null, options).lastModified, 3);
  },
);
[0, '', true, Symbol(), 0n].forEach((options) => {
  assert.throws(() => new File([], null, options), TypeError);
});

delete Object.prototype.lastModified;

[littledivy]

Thanks for the PR.

I'm concerned about the cost of ObjectDefineProperty, it's much slower than simple assignment in V8 (example #16315). This will slowdown all WebAPIs that use createDictionaryConverter.

[aduh95]

Can we measure the performance of this PR vs main?

[bartlomieju]

@littledivy please take a look, can we get this PR to green?

[littledivy]

main:

running 4 tests
test converter_object               ... bench:     118,463 ns/iter (+/- 110,683)
test converter_undefined            ... bench:      21,224 ns/iter (+/- 2,585)
test handwritten_baseline_object    ... bench:       3,489 ns/iter (+/- 148)
test handwritten_baseline_undefined ... bench:       3,558 ns/iter (+/- 114)

This patch:

running 4 tests
test converter_object               ... bench:     343,198 ns/iter (+/- 11,872)
test converter_undefined            ... bench:      21,761 ns/iter (+/- 2,348)
test handwritten_baseline_object    ... bench:       3,710 ns/iter (+/- 183)
test handwritten_baseline_undefined ... bench:       3,359 ns/iter (+/- 97)

Here, converter_object bench calls TextDecodeOptions({ stream: false }); converter which hits this code path. I don't think we can land this with these results.

[petamoriken]

Maybe it would be better to change idlDict to { __proto__: null } or ObjectCreate(null).

[aduh95]

I've rebased and made the change to using null-prototype object as suggested. From the results on the experiments with it on Node.js, it's indeed probably the fastest way around, and thankfully it looks like all the tests are passing.

[petamoriken]

Seems to be a conflict caused by #17648

[aapoalas]

Main:

test converter_object               ... bench:      44,835 ns/iter (+/- 5,683)
test converter_undefined            ... bench:      28,500 ns/iter (+/- 2,462)
test handwritten_baseline_object    ... bench:       4,400 ns/iter (+/- 612)
test handwritten_baseline_undefined ... bench:       3,893 ns/iter (+/- 187)

This branch:

test converter_object               ... bench:      74,464 ns/iter (+/- 12,951)
test converter_undefined            ... bench:      51,699 ns/iter (+/- 8,935)
test handwritten_baseline_object    ... bench:       4,396 ns/iter (+/- 330)
test handwritten_baseline_undefined ... bench:       3,842 ns/iter (+/- 193)

Still apparently slower but maybe not as much?

[aapoalas]

fileConstructorOptionsValidation failed on MacOS. Probably related.

[aapoalas]

main

running 4 tests
test converter_object               ... bench:      63,633 ns/iter (+/- 8,049)
test converter_undefined            ... bench:      37,873 ns/iter (+/- 4,950)
test handwritten_baseline_object    ... bench:       4,023 ns/iter (+/- 443)
test handwritten_baseline_undefined ... bench:       3,338 ns/iter (+/- 160)

this branch

running 4 tests
test converter_object               ... bench:      73,568 ns/iter (+/- 3,361)
test converter_undefined            ... bench:      54,910 ns/iter (+/- 2,800)
test handwritten_baseline_object    ... bench:       4,104 ns/iter (+/- 235)
test handwritten_baseline_undefined ... bench:       3,310 ns/iter (+/- 152)