Closed Bug 804279 Opened 8 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: jorendorff, Assigned: arai)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 9 obsolete files)

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
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);
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
Assignee: general → nobody
>  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)
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)
Thanks :)
Summary: Make Map, Set, and WeakMap constructors generic → Support monkey-patched/overridden adder in Map, Set, WeakMap, and WeakSet constructors.
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)
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)
Attachment #8531078 - Attachment is obsolete: true
Attachment #8531078 - Flags: review?(evilpies)
Attachment #8531277 - Flags: review?(evilpies)
Attachment #8531079 - Attachment is obsolete: true
Attachment #8531079 - Flags: review?(evilpies)
Attachment #8531278 - Flags: review?(evilpies)
Attachment #8531080 - Attachment is obsolete: true
Attachment #8531279 - Flags: review?(evilpies)
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+
Thanks! fixed remaining patches.
Attachment #8531277 - Attachment is obsolete: true
Attachment #8531277 - Flags: review?(evilpies)
Attachment #8532001 - Flags: review?(evilpies)
Attachment #8531278 - Attachment is obsolete: true
Attachment #8531278 - Flags: review?(evilpies)
Attachment #8532002 - Flags: review?(evilpies)
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.
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+
(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.
You need to log in before you can comment on or make changes to this bug.