Closed
Bug 1108930
Opened 9 years ago
Closed 9 years ago
Fix in-tree consumers that call Map/Set/WeakMap constructors without "new"
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
Details
(Keywords: dev-doc-complete)
Attachments
(7 files)
38.79 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
38.26 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
5.56 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
1.29 KB,
patch
|
Felipe
:
review+
|
Details | Diff | Splinter Review |
9.31 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
5.33 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
4.87 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
(similar to bug 980962) Before fixing bug 1062075 and bug 1083752, need to add "new" to all of those calls in tree.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 1•9 years ago
|
||
running try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=08a0908f8706
Assignee | ||
Comment 2•9 years ago
|
||
filed bug 1108965 for gaia.
Assignee | ||
Comment 3•9 years ago
|
||
filed 1109453 for addon-sdk.
Assignee | ||
Comment 4•9 years ago
|
||
will work on this after bug 1114752 (addon sdk uplift) is fixed.
Assignee | ||
Comment 5•9 years ago
|
||
Prepared a patch (Part 7) to show warning when Map/Set/WeakMap are called without `new`, like other ES6 build-ins, and before it, I'd like to fix in-tree consumers (Part 1-6). Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9197229f6060
Attachment #8558566 -
Flags: review?(evilpies)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8558567 -
Flags: review?(evilpies)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8558568 -
Flags: review?(evilpies)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8558570 -
Flags: review?(felipc)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8558572 -
Flags: review?(nfitzgerald)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8558573 -
Flags: review?(mrbkap)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8558574 -
Flags: review?(evilpies)
Updated•9 years ago
|
Attachment #8558570 -
Flags: review?(felipc) → review+
Updated•9 years ago
|
Attachment #8558572 -
Flags: review?(nfitzgerald) → review+
Comment 12•9 years ago
|
||
Comment on attachment 8558566 [details] [diff] [review] Part 1: Call Map with new. Review of attachment 8558566 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/collections/Map-constructor-generator-3.js @@ +1,3 @@ > // The argument to Map may be a generator-iterator that produces no values. > > +assertEq(new Map(x for (x of [])).size, 0); Can you write var m = new Map() assert(..size, 0) I find combining |new| with other expression confusing. @@ +4,5 @@ > > function none() { > if (0) yield 0; > } > +assertEq(new Map(none()).size, 0); And here. ::: js/src/jit-test/tests/collections/Set-iterator-proxies-2.js @@ +12,5 @@ > assertIteratorNext(iterator_fn.call(setw), "x"); > > var next_fn = Set()[Symbol.iterator]().next; > assertThrowsInstanceOf(function () { next_fn.call({}); }, TypeError); > +assertThrowsInstanceOf(function () { next_fn.call(new Map()[Symbol.iterator]()); }, TypeError); (new Map())[Symbol.. ::: js/src/jit-test/tests/collections/iterator-proto-2.js @@ +2,5 @@ > > load(libdir + "iteration.js"); > > var aproto = Object.getPrototypeOf(Array()[Symbol.iterator]()); > +var mproto = Object.getPrototypeOf(new Map()[Symbol.iterator]()); (new Map())[Symbol...
Attachment #8558566 -
Flags: review?(evilpies) → review+
Comment 13•9 years ago
|
||
Comment on attachment 8558567 [details] [diff] [review] Part 2: Call Set with new. Review of attachment 8558567 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/collections/Map-iterator-proxies-2.js @@ +13,5 @@ > assertEqArray(iterator_fn.call(mapw).next().value, ["x", 1]); > > var next_fn = new Map()[Symbol.iterator]().next; > assertThrowsInstanceOf(function () { next_fn.call({}); }, TypeError); > +assertThrowsInstanceOf(function () { next_fn.call(new Set()[Symbol.iterator]()); }, TypeError); (new Set) I just gave up on this point as most of the code already did something like new Map().size. Apparently just I don't like this style.
Attachment #8558567 -
Flags: review?(evilpies) → review+
Updated•9 years ago
|
Attachment #8558568 -
Flags: review?(evilpies) → review+
Comment 14•9 years ago
|
||
Comment on attachment 8558574 [details] [diff] [review] Part 7: Warn when Map/Set/WeakMap are called without new. Review of attachment 8558574 [details] [diff] [review]: ----------------------------------------------------------------- You could actually write a test for the warnings by using options("werror") ::: js/src/jsweakmap.cpp @@ +526,5 @@ > RootedObject obj(cx, NewBuiltinClassInstance(cx, &WeakMapObject::class_)); > if (!obj) > return false; > > + // ES6 23.3.1.1 steps 2-4. Isn't this Step 1.? (in rev31) If NewTarget is null, throw a TypeError exception.
Attachment #8558574 -
Flags: review?(evilpies) → review+
Updated•9 years ago
|
Attachment #8558573 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 15•9 years ago
|
||
Thank you all for reviewing :) (In reply to Tom Schuster [:evilpie] from comment #14) > Comment on attachment 8558574 [details] [diff] [review] > Part 7: Warn when Map/Set/WeakMap are called without new. > > Review of attachment 8558574 [details] [diff] [review]: > ----------------------------------------------------------------- > > You could actually write a test for the warnings by using options("werror") JSMSG_BUILTIN_CTOR_NO_NEW is defined as JSEXN_NONE, and it cannot be caught. Is it okay to make it JSEXN_TYPEERR? (it's also used by ArrayBuffer and TypedArray) > ::: js/src/jsweakmap.cpp > @@ +526,5 @@ > > RootedObject obj(cx, NewBuiltinClassInstance(cx, &WeakMapObject::class_)); > > if (!obj) > > return false; > > > > + // ES6 23.3.1.1 steps 2-4. > > Isn't this Step 1.? (in rev31) > If NewTarget is null, throw a TypeError exception. Yeah, I was reading old one, thank you for letting me know the change :D
Flags: needinfo?(evilpies)
Comment 16•9 years ago
|
||
Oh right, now I remember \o/. I wrote a patch for something else, and they way I did it was: options("werrror"); assertEq(evaluate("WeakMap()", {catchTermination: true}), "terminated");
Flags: needinfo?(evilpies)
Assignee | ||
Comment 17•9 years ago
|
||
Thanks again :D Added werror tests to Map/Set/WeakMap. https://hg.mozilla.org/integration/mozilla-inbound/rev/a24ebc65b965 https://hg.mozilla.org/integration/mozilla-inbound/rev/754f851ff0c8 https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d0cca6c17e https://hg.mozilla.org/integration/mozilla-inbound/rev/65c74fc6e769 https://hg.mozilla.org/integration/mozilla-inbound/rev/94276cdcc5ff https://hg.mozilla.org/integration/mozilla-inbound/rev/798dc45a5610 https://hg.mozilla.org/integration/mozilla-inbound/rev/2c896a4f15ae
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a24ebc65b965 https://hg.mozilla.org/mozilla-central/rev/754f851ff0c8 https://hg.mozilla.org/mozilla-central/rev/e0d0cca6c17e https://hg.mozilla.org/mozilla-central/rev/65c74fc6e769 https://hg.mozilla.org/mozilla-central/rev/94276cdcc5ff https://hg.mozilla.org/mozilla-central/rev/798dc45a5610 https://hg.mozilla.org/mozilla-central/rev/2c896a4f15ae
Assignee: nobody → arai_a
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox38:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 19•9 years ago
|
||
Added to the compat doc: https://developer.mozilla.org/en-US/Firefox/Releases/38/Site_Compatibility
Keywords: dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•