Closed Bug 804279 Opened 13 years ago Closed 10 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
evilpies
: review+
Details | Diff | Splinter Review
3.23 KB, patch
evilpies
: review+
Details | Diff | Splinter Review
3.27 KB, patch
evilpies
: review+
Details | Diff | Splinter Review
3.97 KB, patch
evilpies
: review+
Details | Diff | Splinter Review
16.91 KB, patch
evilpies
: 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.

Attachment

General

Created:
Updated:
Size: