804279 - Support monkey-patched/overridden adder in Map, Set, WeakMap, and WeakSet constructors.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

The ES6 draft for the Map constructor is generic: it can be applied to a this-value that isn't a Map. It actually does a [[Get]] operation for the method this.set, and calls that method repeatedly.

This is observable in code like:

  // Map constructor works on non-Map this-values
  var count = 0, key, val;
  var obj = {set: function (k, v) { count++; key = k; val = v; };
  Map.call(obj, [["a", 7]]);
  assertEq(count, 1);
  assertEq(key, "a");
  assertEq(val, 7);

Or:

  // Map constructor calls monkey-patched Map.prototype.set
  var count = 0, key, val;
  Map.prototype.set = function (k, v) { count++; key = k; val = v; };
  var m = new Map([["a", 7]]);
  assertEq(count, 1);
  assertEq(key, "a");
  assertEq(val, 7);
  assertEq(m.has("a"), false);

Comment 1

[Brandon Benvie [:benvie]]

This is also true for WeakMaps, Sets, and WeakSets (using "add" for [Weak]Sets).

Comment 2

[Tooru Fujisawa [:arai]]

>  var count = 0, key, val;
>  var obj = {set: function (k, v) { count++; key = k; val = v; };
>  Map.call(obj, [["a", 7]]);
The spec for Map constructor is changed in Rev 13,
and now this call will throw TypeError at step 3 in 23.1.1.1:
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-map-iterable
> 3. If map does not have a [[MapData]] internal slot, then throw a TypeError exception.

Also, Map.call/Map.apply is not applicable to Map instance, it will throw TypeError at step 4:
> 4. If map’s [[MapData]] internal slot is not undefined, then throw a TypeError exception.

So, Map constructor is no more generic.

Second example is still valid and not yet implemented.

Is this correct?

Comment 3

[Jason Orendorff [:jorendorff]]

Yes.

This will also be possible via subclassing, I think:

    class SnoopMap extends Map {
        set(k, v) { count++; key = k; val = v; }
    }
    var m = new SnoopMap([["a", 7]]);

but we're not there yet.

Comment 4

[Tooru Fujisawa [:arai]]

Thanks :)

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part1: Support monkey-patched/overridden adder in Map constructor.

Part 1-4 fixes constructors, and Part 5 adds tests for them.

They are almost same as the removed code from the first patch for bug 1092537.

Then-clauses for `if (isOriginalAdder)` are kept for performance reason.
If I remove them and call Invoke in all cases, it still works but it takes 1.2-5 times longer for each call.

WeakSet constructors has different code for checking monkey-patching,
because WeakSet.prototype.add is self-hosted JavaScript function.
I tried implementing WeakSet.prototype.add with native function,
but I don't see any difference in terms of performance.

Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bea2e7622a10

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part2: Support monkey-patched/overridden adder in Set constructor.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part3: Support monkey-patched/overridden adder in WeakMap constructor.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part4: Support monkey-patched/overridden adder in WeakSet constructor.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part5: Add tests for monkey-patched adder in collection constructors.

Comment 10

[Tom S. (please needinfo tschuster)]

Comment on attachment 8531077 [details] [diff] [review]
Part1: Support monkey-patched/overridden adder in Map constructor.

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

Would you mind fixing those in all patches first?

::: js/src/builtin/MapObject.cpp
@@ +1241,5 @@
> +        if (!JSObject::getProperty(cx, obj, obj, cx->names().set, &adderVal))
> +            return false;
> +
> +        if (!IsCallable(adderVal)) {
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_NOT_FUNCTION, "this.set");

I see what you mean, but "this.set" kind of the wrong way of saying this. Maybe Map.prototype.set, but that would probably not fit well with subclassing? Is there a similar case, where we throw in a case like this?

@@ +1246,5 @@
> +            return false;
> +        }
> +
> +        bool isOriginalAdder = false;
> +        RootedValue mapVal(cx);

Just always initialize this with ObjectValue(obj)

@@ +1247,5 @@
> +        }
> +
> +        bool isOriginalAdder = false;
> +        RootedValue mapVal(cx);
> +        JSFunction *adder = &adderVal.toObject().as<JSFunction>();

You should be using IsNativeFunction here and in all other patches. This would crash when adderVal is not a JSFunction.

@@ +1294,5 @@
> +                    js_ReportOutOfMemory(cx);
> +                    return false;
> +                }
> +            } else {
> +                InvokeArgs args(cx);

If you want you could look into using FastInvokeGuard.

Comment 11

[Tom S. (please needinfo tschuster)]

Comment on attachment 8531080 [details] [diff] [review]
Part4: Support monkey-patched/overridden adder in WeakSet constructor.

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

::: js/src/builtin/WeakSetObject.cpp
@@ +115,5 @@
> +            return false;
> +        RootedAtom WeakSet_addAtom(cx, cx->names().WeakSet_add);
> +        if (!WeakSet_addAtom)
> +            return false;
> +        if (!cx->global()->getSelfHostedFunction(cx, WeakSet_addAtom, addAtom, 1, &originalAdder))

You should use IsSelfHostedFunctionWithName instead of this.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part1: Support monkey-patched/overridden adder in Map constructor.

Thank you for reviewing!

(In reply to Tom Schuster [:evilpie] from comment #10)
> Comment on attachment 8531077 [details] [diff] [review]
> Part1: Support monkey-patched/overridden adder in Map constructor.
> 
> Review of attachment 8531077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/builtin/MapObject.cpp
> @@ +1241,5 @@
> > +        if (!JSObject::getProperty(cx, obj, obj, cx->names().set, &adderVal))
> > +            return false;
> > +
> > +        if (!IsCallable(adderVal)) {
> > +            JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_NOT_FUNCTION, "this.set");
> 
> I see what you mean, but "this.set" kind of the wrong way of saying this.
> Maybe Map.prototype.set, but that would probably not fit well with
> subclassing? Is there a similar case, where we throw in a case like this?

Found similar case in Date.prototype.toJSON which calls Date.prototype.toISOString.
  http://dxr.mozilla.org/mozilla-central/source/js/src/jsdate.cpp#2469
  http://dxr.mozilla.org/mozilla-central/source/js/src/js.msg#116
It reports "toISOString property is not callable" when the `toISOString` property of `this` value is not callable.

So I changed the message to "set property is not callable" for Map/WeakMap, and
"add property is not callable" for Set/WeakSet.

> @@ +1247,5 @@
> > +        }
> > +
> > +        bool isOriginalAdder = false;
> > +        RootedValue mapVal(cx);
> > +        JSFunction *adder = &adderVal.toObject().as<JSFunction>();
> 
> You should be using IsNativeFunction here and in all other patches. This
> would crash when adderVal is not a JSFunction.

Thanks!
I added tests for Proxy function in Part 5.

Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=862649f14379

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Part2: Support monkey-patched/overridden adder in Set constructor.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part3: Support monkey-patched/overridden adder in WeakMap constructor.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part4: Support monkey-patched/overridden adder in WeakSet constructor.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part5: Add tests for monkey-patched adder in collection constructors.

Comment 17

[Tom S. (please needinfo tschuster)]

Comment on attachment 8531276 [details] [diff] [review]
Part1: Support monkey-patched/overridden adder in Map constructor.

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

::: js/src/builtin/MapObject.cpp
@@ +1242,5 @@
> +            return false;
> +
> +        if (!IsCallable(adderVal)) {
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr,
> +                                 JSMSG_NOT_CALLABLE, "set property");

Okay let's just do ReportIsNotFunction.

@@ +1306,2 @@
>              }
>              WriteBarrierPost(cx->runtime(), map, key);

This probably should move inside the isOriginalAdder block.

::: js/src/js.msg
@@ +49,5 @@
>  MSG_DEF(JSMSG_BAD_SORT_ARG,            0, JSEXN_TYPEERR, "invalid Array.prototype.sort argument")
>  MSG_DEF(JSMSG_CANT_WATCH,              1, JSEXN_TYPEERR, "can't watch non-native objects of class {0}")
>  MSG_DEF(JSMSG_READ_ONLY,               1, JSEXN_TYPEERR, "{0} is read-only")
>  MSG_DEF(JSMSG_CANT_DELETE,             1, JSEXN_TYPEERR, "property {0} is non-configurable and can't be deleted")
> +MSG_DEF(JSMSG_NOT_CALLABLE,            1, JSEXN_TYPEERR, "{0} is not callable")

Let's not do this.

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Part2: Support monkey-patched/overridden adder in Set constructor.

Thanks! fixed remaining patches.

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Part3: Support monkey-patched/overridden adder in WeakMap constructor.

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Part4: Support monkey-patched/overridden adder in WeakSet constructor.

Comment 21

[Tom S. (please needinfo tschuster)]

Comment on attachment 8531280 [details] [diff] [review]
Part5: Add tests for monkey-patched adder in collection constructors.

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

I think these tests are really good, however it would be a lot easier if you would just copy past it four times and split it up in one test for Map,Set,WeakMap,WeakSetf. You wouldn't need all the Constructor stuff anymore.

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Part5: Add tests for monkey-patched adder in collection constructors.

Thanks again :)
splitted into 4 files.

Comment 23

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00fc0c2dc111
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ff17c6cdec
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dcbacf3d659
https://hg.mozilla.org/integration/mozilla-inbound/rev/d74f36957303
https://hg.mozilla.org/integration/mozilla-inbound/rev/6717479ac35a

Comment 24

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/44d7ea20541f for the build bustage in https://treeherder.mozilla.org/logviewer.html#?job_id=4382998&repo=mozilla-inbound

Comment 25

[Tooru Fujisawa [:arai]]

Sorry, the bustage was caused by missing #include "vm/Interpreter-inl.h" in MapObject.cpp and WeakSetObject.cpp.

I added linux32 to try target.
Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=48c4df754884

and landed again
https://hg.mozilla.org/integration/mozilla-inbound/rev/b24bc4c1435d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e5e8e13b705
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb681c8ddba6
https://hg.mozilla.org/integration/mozilla-inbound/rev/9662d7f8d568
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f6965ca9de2

Comment 26

[Jason Orendorff [:jorendorff]]

(In reply to Tom Schuster [:evilpie] from comment #21)
> I think these tests are really good, however it would be a lot easier if you
> would just copy past it four times and split it up in one test for
> Map,Set,WeakMap,WeakSetf. You wouldn't need all the Constructor stuff
> anymore.

This seems like terrible advice to me. It looks like it's about three times as much code this way.

Please see: http://number-none.com/blow/john_carmack_on_inlined_code.html

In particular:

> On the subject of code duplication, I tracked all the bugs (problems
> that surfaced after I thought everything was working correctly) I
> fixed for a while, and I was quite surprised at how often
> copy-paste-modify operations resulted in subtle bugs that weren't
> immediately obvious. For small vector operations on three or four
> things, I would often just past and modify a few characters like this:
>
>     v[0] = HF_MANTISSA(*(halfFloat_t *)((byte *)data + i*bytePitch+j*8+0));
>     v[1] = HF_MANTISSA(*(halfFloat_t *)((byte *)data + i*bytePitch+j*8+1));
>     v[2] = HF_MANTISSA(*(halfFloat_t *)((byte *)data + i*bytePitch+j*8+2));
>     v[3] = HF_MANTISSA(*(halfFloat_t *)((byte *)data + i*bytePitch+j*8+3));
>
> I now strongly encourage explicit loops for everything, and hope the
> compiler unrolls it properly. A significant number of my bugs related
> to things like this, and I am now rethinking even two dimensional
> cases where I commonly use discrete _X, _Y or _WIDTH, _HEIGHT
> variables. I find that much easier to read than a two element array,
> but it is difficult to argue with my data on how often it made me
> screw up.

(I understand the irony in choosing to block-quote that instead of just linking to it.)

Comment 27

[Tom S. (please needinfo tschuster)]

This is not really comparable to the example you give. There is a real difference between set/add, and everybody knows loop-unrolling with conditionals is a lot harder! I would probably agree that testing WeakMap/Map and WeakSet/Set together would have been the right choice.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b24bc4c1435d
https://hg.mozilla.org/mozilla-central/rev/1e5e8e13b705
https://hg.mozilla.org/mozilla-central/rev/cb681c8ddba6
https://hg.mozilla.org/mozilla-central/rev/9662d7f8d568
https://hg.mozilla.org/mozilla-central/rev/8f6965ca9de2

Comment 29

[Tooru Fujisawa [:arai]]

Updated following documents:
  https://developer.mozilla.org/en-US/Firefox/Releases/37
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakSet
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla

Comment 30

[Kohei]

Added to the compat doc: https://developer.mozilla.org/en-US/Firefox/Releases/37/Site_Compatibility