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

RESOLVED FIXED in mozilla37

Status

()

RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: jorendorff, Assigned: arai)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Other Branch
mozilla37
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 9 obsolete attachments)

4.90 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
3.23 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
3.27 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
3.97 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
16.91 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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);

Updated

6 years ago
Depends on: 838540
This is also true for WeakMaps, Sets, and WeakSets (using "add" for [Weak]Sets).
Summary: Make Map constructor generic → Make Map, Set, and WeakMap constructors generic
Keywords: dev-doc-needed

Updated

6 years ago
Blocks: 918349
Assignee: general → nobody
(Assignee)

Comment 2

4 years ago
>  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?
Flags: needinfo?(jorendorff)
(Reporter)

Comment 3

4 years ago
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.
Flags: needinfo?(jorendorff)
(Assignee)

Comment 4

4 years ago
Thanks :)
Summary: Make Map, Set, and WeakMap constructors generic → Support monkey-patched/overridden adder in Map, Set, WeakMap, and WeakSet constructors.
(Assignee)

Comment 5

4 years ago
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
Attachment #8531077 - Flags: review?(evilpies)
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.
Attachment #8531077 - Flags: review?(evilpies)
Assignee: nobody → arai_a
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.
Attachment #8531080 - Flags: review?(evilpies)
(Assignee)

Comment 12

4 years ago
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
Attachment #8531077 - Attachment is obsolete: true
Attachment #8531276 - Flags: review?(evilpies)
(Assignee)

Comment 13

4 years ago
Attachment #8531078 - Attachment is obsolete: true
Attachment #8531078 - Flags: review?(evilpies)
Attachment #8531277 - Flags: review?(evilpies)
(Assignee)

Comment 14

4 years ago
Attachment #8531079 - Attachment is obsolete: true
Attachment #8531079 - Flags: review?(evilpies)
Attachment #8531278 - Flags: review?(evilpies)
(Assignee)

Comment 15

4 years ago
Attachment #8531080 - Attachment is obsolete: true
Attachment #8531279 - Flags: review?(evilpies)
(Assignee)

Comment 16

4 years ago
Attachment #8531081 - Attachment is obsolete: true
Attachment #8531081 - Flags: review?(evilpies)
Attachment #8531280 - Flags: review?(evilpies)
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.
Attachment #8531276 - Flags: review?(evilpies) → review+
(Assignee)

Comment 18

4 years ago
Thanks! fixed remaining patches.
Attachment #8531277 - Attachment is obsolete: true
Attachment #8531277 - Flags: review?(evilpies)
Attachment #8532001 - Flags: review?(evilpies)
(Assignee)

Comment 19

4 years ago
Attachment #8531278 - Attachment is obsolete: true
Attachment #8531278 - Flags: review?(evilpies)
Attachment #8532002 - Flags: review?(evilpies)
(Assignee)

Comment 20

4 years ago
Attachment #8531279 - Attachment is obsolete: true
Attachment #8531279 - Flags: review?(evilpies)
Attachment #8532003 - Flags: review?(evilpies)
Attachment #8532001 - Flags: review?(evilpies) → review+
Attachment #8532002 - Flags: review?(evilpies) → review+
Attachment #8532003 - Flags: review?(evilpies) → review+
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.
(Assignee)

Comment 22

4 years ago
Thanks again :)
splitted into 4 files.
Attachment #8531280 - Attachment is obsolete: true
Attachment #8531280 - Flags: review?(evilpies)
Attachment #8532054 - Flags: review?(evilpies)
Attachment #8532054 - Flags: review?(evilpies) → review+
(Reporter)

Comment 26

4 years ago
(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.)
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.