Closed
Bug 804279
Opened 11 years ago
Closed 9 years ago
Support monkey-patched/overridden adder in Map, Set, WeakMap, and WeakSet constructors.
Categories
(Core :: JavaScript Engine, defect)
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);
Comment 1•10 years ago
|
||
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
Updated•10 years ago
|
Keywords: dev-doc-needed
Updated•10 years ago
|
Blocks: harmony:collections
Updated•9 years ago
|
Assignee: general → nobody
Assignee | ||
Comment 2•9 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•9 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•9 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•9 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)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8531078 -
Flags: review?(evilpies)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8531079 -
Flags: review?(evilpies)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8531080 -
Flags: review?(evilpies)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8531081 -
Flags: review?(evilpies)
Comment 10•9 years ago
|
||
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)
Updated•9 years ago
|
Assignee: nobody → arai_a
Comment 11•9 years ago
|
||
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•9 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•9 years ago
|
||
Attachment #8531078 -
Attachment is obsolete: true
Attachment #8531078 -
Flags: review?(evilpies)
Attachment #8531277 -
Flags: review?(evilpies)
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8531079 -
Attachment is obsolete: true
Attachment #8531079 -
Flags: review?(evilpies)
Attachment #8531278 -
Flags: review?(evilpies)
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8531080 -
Attachment is obsolete: true
Attachment #8531279 -
Flags: review?(evilpies)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8531081 -
Attachment is obsolete: true
Attachment #8531081 -
Flags: review?(evilpies)
Attachment #8531280 -
Flags: review?(evilpies)
Comment 17•9 years ago
|
||
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•9 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•9 years ago
|
||
Attachment #8531278 -
Attachment is obsolete: true
Attachment #8531278 -
Flags: review?(evilpies)
Attachment #8532002 -
Flags: review?(evilpies)
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8531279 -
Attachment is obsolete: true
Attachment #8531279 -
Flags: review?(evilpies)
Attachment #8532003 -
Flags: review?(evilpies)
Updated•9 years ago
|
Attachment #8532001 -
Flags: review?(evilpies) → review+
Updated•9 years ago
|
Attachment #8532002 -
Flags: review?(evilpies) → review+
Updated•9 years ago
|
Attachment #8532003 -
Flags: review?(evilpies) → review+
Comment 21•9 years ago
|
||
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•9 years ago
|
||
Thanks again :) splitted into 4 files.
Attachment #8531280 -
Attachment is obsolete: true
Attachment #8531280 -
Flags: review?(evilpies)
Attachment #8532054 -
Flags: review?(evilpies)
Updated•9 years ago
|
Attachment #8532054 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 23•9 years ago
|
||
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•9 years ago
|
||
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
Assignee | ||
Comment 25•9 years ago
|
||
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
Reporter | ||
Comment 26•9 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.)
Comment 27•9 years ago
|
||
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•9 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Assignee | ||
Comment 29•9 years ago
|
||
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
Updated•9 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Comment 30•9 years ago
|
||
Added to the compat doc: https://developer.mozilla.org/en-US/Firefox/Releases/37/Site_Compatibility
You need to log in
before you can comment on or make changes to this bug.
Description
•