WeakMap.get, has and delete should not throw when key param is not an object

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gilles.lepretre, Assigned: mccr8)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-complete, site-compat})

Trunk
mozilla38
addon-compat, dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

4 years ago
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."
Component: Untriaged → JavaScript Engine
Product: Firefox → Core

Updated

4 years ago
Blocks: 694100
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

4 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

4 years ago
Mentor: continuation
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

4 years ago
sure
Assignee: nobody → continuation
Mentor: continuation
Flags: needinfo?(continuation)
(Assignee)

Comment 4

4 years ago
Anuj said he could get this done in the next week or two.
Assignee: continuation → anujagarwal464
Mentor: continuation
(Assignee)

Comment 5

4 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

4 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.
Assignee: anujagarwal464 → nobody
(Assignee)

Updated

4 years ago
Assignee: nobody → continuation
Mentor: continuation
(Assignee)

Comment 7

4 years ago
Created attachment 8559414 [details] [diff] [review]
WeakMap.get, has and delete should not throw when the key arg is not an object.

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

4 years ago
Attachment #8559414 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

4 years ago
OS: Linux → All
Hardware: x86_64 → All

Comment 8

4 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

4 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

4 years ago
Created attachment 8564363 [details] [diff] [review]
part 2 - Treat missing arguments to weakmap methods as undefined.

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

4 years ago
Created attachment 8564364 [details] [diff] [review]
part 3 - Weak map get should always return undefined on failure to find the key.

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

4 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 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 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+
(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)
...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

4 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

4 years ago
Created attachment 8566122 [details] [diff] [review]
part 3b - Update manifests to indicate that WeakMap.prototype.get tests pass.

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)
Attachment #8566122 - Flags: review?(james) → review+
(Assignee)

Comment 20

4 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
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
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Reporter)

Comment 22

4 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

Updated

4 years ago
Duplicate of this bug: 1009349
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.