Closed Bug 1127827 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

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)

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
Blocks: es6
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
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)
sure
Assignee: nobody → continuation
Mentor: continuation
Flags: needinfo?(continuation)
Anuj said he could get this done in the next week or two.
Assignee: continuation → anujagarwal464
Mentor: continuation
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.
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: nobody → continuation
Mentor: continuation
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
Attachment #8559414 - Flags: review?(jwalden+bmo)
OS: Linux → All
Hardware: x86_64 → All
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.
(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?
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)
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)
(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.
(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".
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+
(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
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
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)
You need to log in before you can comment on or make changes to this bug.