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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gilles Lepretre]

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."

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

sure

Comment 4

[Andrew McCreight [:mccr8]]

Anuj said he could get this done in the next week or two.

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Andrew McCreight [:mccr8]]

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.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: 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

Comment 8

[Jeff Walden [:Waldo]]

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.

Comment 9

[Tom S]

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.

Comment 10

[Andrew McCreight [:mccr8]]

(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?

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: 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.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: 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.

Comment 13

[Andrew McCreight [:mccr8]]

(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?

Comment 14

[Jeff Walden [:Waldo]]

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.  :-(

Comment 15

[Jeff Walden [:Waldo]]

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.

Comment 16

[Jeff Walden [:Waldo]]

(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.

Comment 17

[Jeff Walden [:Waldo]]

...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.

Comment 18

[Andrew McCreight [:mccr8]]

(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".

Comment 19

[Andrew McCreight [:mccr8]]

Attachment: 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

Comment 20

[Andrew McCreight [:mccr8]]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/53a0ff28cc17
https://hg.mozilla.org/mozilla-central/rev/e9449c138c67
https://hg.mozilla.org/mozilla-central/rev/f74ab4efb948

Comment 22

[Gilles Lepretre]

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

[Florian Scholz (Open Web Docs)]

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

[Kohei Yoshino]

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.

Comment 26

[Florian Scholz (Open Web Docs)]

Thanks for letting me know. Seems like your tracking based on the keywords is colliding with the MDN team's tracking then ;-)

Comment 27

[Kohei Yoshino]

I could request to add the new site-compat-doc-needed & site-compat-doc-complete keywords but I think that's too much.

Comment 28

[Kohei Yoshino]

Technically "dev-doc" may include an add-ons blog post on addon-compat too, though looks like :jorgev is not using that keyword for tracking purpose.

Comment 29

[Florian Scholz (Open Web Docs)]

Please follow up on a mailing list like dev-mdc, if you see the need to discuss this issue. I will keep it in mind, but bear with me / us if we forget it occasionally.