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)
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);
Comment 1•12 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•12 years ago
|
Keywords: dev-doc-needed
Updated•12 years ago
|
Blocks: harmony:collections
Updated•11 years ago
|
Assignee: general → nobody
Assignee | ||
Comment 2•10 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•10 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•10 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•10 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•10 years ago
|
||
Attachment #8531078 -
Flags: review?(evilpies)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8531079 -
Flags: review?(evilpies)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8531080 -
Flags: review?(evilpies)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8531081 -
Flags: review?(evilpies)
Comment 10•10 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•10 years ago
|
Assignee: nobody → arai_a
Comment 11•10 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•10 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•10 years ago
|
||
Attachment #8531078 -
Attachment is obsolete: true
Attachment #8531078 -
Flags: review?(evilpies)
Attachment #8531277 -
Flags: review?(evilpies)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8531079 -
Attachment is obsolete: true
Attachment #8531079 -
Flags: review?(evilpies)
Attachment #8531278 -
Flags: review?(evilpies)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8531080 -
Attachment is obsolete: true
Attachment #8531279 -
Flags: review?(evilpies)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8531081 -
Attachment is obsolete: true
Attachment #8531081 -
Flags: review?(evilpies)
Attachment #8531280 -
Flags: review?(evilpies)
Comment 17•10 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•10 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•10 years ago
|
||
Attachment #8531278 -
Attachment is obsolete: true
Attachment #8531278 -
Flags: review?(evilpies)
Attachment #8532002 -
Flags: review?(evilpies)
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8531279 -
Attachment is obsolete: true
Attachment #8531279 -
Flags: review?(evilpies)
Attachment #8532003 -
Flags: review?(evilpies)
Updated•10 years ago
|
Attachment #8532001 -
Flags: review?(evilpies) → review+
Updated•10 years ago
|
Attachment #8532002 -
Flags: review?(evilpies) → review+
Updated•10 years ago
|
Attachment #8532003 -
Flags: review?(evilpies) → review+
Comment 21•10 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•10 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•10 years ago
|
Attachment #8532054 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 23•10 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•10 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•10 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•10 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•10 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•10 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: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Assignee | ||
Comment 29•10 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•10 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Comment 30•10 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
•