Closed
Bug 1127827
Opened 10 years ago
Closed 10 years ago
WeakMap.get, has and delete should not throw when key param is not an object
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: gilles.lepretre, Assigned: mccr8)
References
(Blocks 1 open bug)
Details
(Keywords: addon-compat, dev-doc-complete, site-compat)
Attachments
(4 files)
5.53 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
5.49 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
1.83 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
1.37 KB,
patch
|
jgraham
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2288.8 Safari/537.36
Steps to reproduce:
var wm = new WeakMap();
wm.get("baz");
Actual results:
wm.get("baz"); // Throws TypeError: "value is not a non-null object"
This error message is strange because:
wm.get({}); // Returns undefined.
wm.get(Object.create(null)); // Returns undefined.
Expected results:
wm.get("baz"); // Returns undefined
According to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap/get
and http://people.mozilla.org/~jorendorff/es6-draft.html#sec-weakmap.prototype.get
"6. If Type(key) is not Object, return undefined."
Updated•10 years ago
|
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
It looks like "has" and "delete" are also wrong. They are supposed to return false, but it looks like they will also throw. It would probably be good to just hoist the JS_ReportErrorNumber out of GetKeyArg (in js/src/jsweakmap.cpp) for the one case that actually needs it now, set.
Summary: WeakMap.get should not throw when key param is not an object → WeakMap.get, has and delete should not throw when key param is not an object
Assignee | ||
Updated•10 years ago
|
Mentor: continuation
Comment 2•10 years ago
|
||
Yeah, looks like the implementation of this stuff predates the current spec text...
Andrew, want to just fix this? If not, I will; it's an ES6 compliance issue that we shouldn't keep shipping.
Flags: needinfo?(continuation)
Assignee | ||
Comment 3•10 years ago
|
||
sure
Assignee: nobody → continuation
Mentor: continuation
Flags: needinfo?(continuation)
Assignee | ||
Comment 4•10 years ago
|
||
Anuj said he could get this done in the next week or two.
Assignee: continuation → anujagarwal464
Mentor: continuation
Assignee | ||
Comment 5•10 years ago
|
||
Basically, there are a few WeakMap functions that are returning incorrect values when they get passed in things like strings, which are not valid weak map keys. These methods are implemented as WeakMap_*_impl in js/src/jsweakmap.cpp. This error stuff is being handled in the function GetKeyArg(), so you want to look at every place that calls it and see if they match the spec linked in comment 0 in http://people.mozilla.org/~jorendorff/es6-draft.html
Right now, we throw an exception if there's a non-string value, but for some of these functions, we need to succeed instead, and return some value. You return a value in these functions by calling something like |args.rval().setBoolean(true);| (that would return the boolean true value), and I think returning true.
Assignee | ||
Comment 6•10 years ago
|
||
The high level idea here is that our error handling for these methods is not matching the official specification, so people who try to use it will get confused, and won't be able to use the same code for Firefox as in Chrome.
Updated•10 years ago
|
Assignee: anujagarwal464 → nobody
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → continuation
Mentor: continuation
Assignee | ||
Comment 7•10 years ago
|
||
I based the behavior on the es6 draft. I added tests for the return values for the broken functions, plus a few other cases of return values that were missing tests.
Hopefully this doesn't break any of our code that uses weak maps.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aee98a40903
Assignee | ||
Updated•10 years ago
|
Attachment #8559414 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•10 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Comment 8•10 years ago
|
||
Comment on attachment 8559414 [details] [diff] [review]
WeakMap.get, has and delete should not throw when the key arg is not an object.
Review of attachment 8559414 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/tests/js1_8_5/extensions/weakmap.js
@@ +91,5 @@
> gc(); gc(); gc();
>
> check(function() map.get(key) == 42);
> + check(function() map.delete(key) == true);
> + check(function() map.delete({}) == false);
Add
check(function() map.delete(key) == false);
after the first addition, too.
Attachment #8559414 -
Flags: review?(jwalden+bmo) → review+
Comment on attachment 8559414 [details] [diff] [review]
WeakMap.get, has and delete should not throw when the key arg is not an object.
Review of attachment 8559414 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsweakmap.cpp
@@ +211,5 @@
> {
> MOZ_ASSERT(IsWeakMap(args.thisv()));
>
> if (args.length() < 1) {
> JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_MORE_ARGS_NEEDED,
You have to remove those as well. Otherwise map.has() throws, but map.has(undefined) doesn't.
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> Add check(function() map.delete(key) == false); after the first addition, too.
After the set you mean? And then I guess add another set because it is deleted?
Assignee | ||
Comment 11•10 years ago
|
||
This fixes the issue evilpie pointed out. Plus I added a few more random checks for return values.
I also added one use of CallArgs::get() to clean it up a little.
Attachment #8564363 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 12•10 years ago
|
||
I noticed the spec says get should always return undefined if the key is not in the map, but we were returning the key for some reason.
Attachment #8564364 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #10)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> > Add check(function() map.delete(key) == false); after the first addition, too.
>
> After the set you mean? And then I guess add another set because it is
> deleted?
Flags: needinfo?(jwalden+bmo)
Comment 14•10 years ago
|
||
Comment on attachment 8564363 [details] [diff] [review]
part 2 - Treat missing arguments to weakmap methods as undefined.
Review of attachment 8564363 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsweakmap.cpp
@@ +388,5 @@
> return false;
> }
>
> RootedObject key(cx, &args[0].toObject());
> + RootedValue value(cx, args.get(1));
Can you simply pass |args.get(1)| down to the setting method?
::: js/src/tests/js1_8_5/extensions/weakmap.js
@@ +83,5 @@
> var map = new WeakMap();
>
> check(function() !map.has(key));
> + check(function() map.delete(key) == false);
> + check(function() map.set(key, 42) === map);
Bah, this-returning methods are dumb. :-(
Attachment #8564363 -
Flags: review?(jwalden+bmo) → review+
Comment 15•10 years ago
|
||
Comment on attachment 8564364 [details] [diff] [review]
part 3 - Weak map get should always return undefined on failure to find the key.
Review of attachment 8564364 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsweakmap.cpp
@@ -273,5 @@
> return true;
> }
> }
>
> - args.rval().set((args.length() > 1) ? args[1] : UndefinedValue());
So this was basically like Python's dict.get(k, ifNotFound) method before, but they didn't standardize it this way. Be on the lookout for anyone breaking because of this change.
Attachment #8564364 -
Flags: review?(jwalden+bmo) → review+
Comment 16•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #13)
> (In reply to Andrew McCreight [:mccr8] from comment #10)
> > (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> > > Add check(function() map.delete(key) == false); after the first addition, too.
> >
> > After the set you mean? And then I guess add another set because it is
> > deleted?
Your patch has:
check(function() map.get(key) == 42);
check(function() map.delete(key) == true);
check(function() map.delete({}) == false);
I was asking for
check(function() map.get(key) == 42);
check(function() map.delete(key) == true);
check(function() map.delete(key) == false);
check(function() map.delete({}) == false);
adding the line after your first added line, to test that deletion of a non-existent key returned false. Could well be tested elsewhere for all I know, but my habit is to add whatever tests make local sense whenever I can, when they're quick. Redundant O(1) tests are not going to hurt runtime to a measurable degree.
Flags: needinfo?(jwalden+bmo)
Comment 17•10 years ago
|
||
...or rather, to test that deletion of an existing key returns true, then a repeated deletion of it detects the new absence of it to return false. I could imagine different code coming into play for the not-existing-now-but-did-before case, and for the never-existed-ever case, in theory.
Assignee | ||
Comment 18•10 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15)
> So this was basically like Python's dict.get(k, ifNotFound) method before,
> but they didn't standardize it this way. Be on the lookout for anyone
> breaking because of this change.
Oh, right, args.get(1) is the second argument, not the first. I'm too used to argv[0] being a non-argument. That makes more sense. I did some spot-checking of weak maps in m-c and I only found a few places that passed in a second argument, and they both passed in null, so hopefully it will be okay.
Thanks for the explanation about the test. For some reason I interpreted "addition" in your initial comment as "addition to the weak map", rather than "addition to the patch".
Assignee | ||
Comment 19•10 years ago
|
||
The test for WeakMap get's return value now pass. I guess these tests are less complete than our own tests, given that only one started passing.
try run without this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3b995cd4c72
Attachment #8566122 -
Flags: review?(james)
Updated•10 years ago
|
Attachment #8566122 -
Flags: review?(james) → review+
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> Can you simply pass |args.get(1)| down to the setting method?
Fixed.
I also added the test you suggested.
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/53a0ff28cc17
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e9449c138c67
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f74ab4efb948
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/53a0ff28cc17
https://hg.mozilla.org/mozilla-central/rev/e9449c138c67
https://hg.mozilla.org/mozilla-central/rev/f74ab4efb948
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Reporter | ||
Comment 22•10 years ago
|
||
A bug has been filed against the spec
Chromium issue https://code.google.com/p/chromium/issues/detail?id=460083
EcmaScript issue https://bugs.ecmascript.org/show_bug.cgi?id=4019
Comment 24•10 years ago
|
||
Carrying over keywords from bug 1009349
Docs:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap/get#Firefox-specific_notes
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap/has#Firefox-specific_notes
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap/delete#Firefox-specific_notes
https://developer.mozilla.org/en-US/Firefox/Releases/38#JavaScript
Comment 25•10 years ago
|
||
Added to the site compat doc: https://developer.mozilla.org/en-US/Firefox/Releases/38/Site_Compatibility#WeakMap_implementation_has_been_updated
:fscholz, please do not set the dev-doc-complete keyword until the site compatibility document is ready, if the bug has the site-compat keyword as well. That makes my life much easier. Thanks.
Flags: needinfo?(fscholz)
Comment hidden (off-topic) |
Comment hidden (off-topic) |
Comment hidden (off-topic) |
Comment hidden (off-topic) |
You need to log in
before you can comment on or make changes to this bug.
Description
•