deno icon indicating copy to clipboard operation
deno copied to clipboard

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

Open aduh95 opened this issue 3 years ago • 6 comments

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;

aduh95 avatar Oct 25 '22 00:10 aduh95

Can we measure the performance of this PR vs main?

aduh95 avatar Oct 25 '22 02:10 aduh95

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

bartlomieju avatar Dec 18 '22 02:12 bartlomieju

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.

littledivy avatar Jan 15 '23 13:01 littledivy

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

petamoriken avatar Feb 04 '23 11:02 petamoriken

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.

aduh95 avatar Feb 04 '23 15:02 aduh95

Seems to be a conflict caused by #17648

petamoriken avatar Feb 09 '23 09:02 petamoriken

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 avatar Feb 11 '23 20:02 aapoalas

fileConstructorOptionsValidation failed on MacOS. Probably related.

aapoalas avatar Feb 11 '23 21:02 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)

aapoalas avatar Oct 28 '23 13:10 aapoalas