1009349 - Deprecate optional second argument to WeakMap.prototype.get

Status: RESOLVED DUPLICATE of bug 1127827

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

TC39, in its wisdom, decided against `weakmap.get(key, fallbackValue)`.

So we're really not in compliance as long as we support it, and we should drop it.

Comment 1

[Arpad Borsos [:Swatinem]]

Is there an easy way to figure out what code already uses this parameter?

I’m talking about WeakMap users in firefox chrome code itself.

Comment 2

[Till Schneidereit [:till]]

I see two different options:
1. create a build that doesn't only remove the second parameter, but causes an error to be thrown if it is passed. Do a full try server run and see what turns up
3. manually inspect all 132 usages of WeakMap currently in mozilla-central[1]

Unfortunately, both of these won't catch usages in addons. There is a mxr for addons, but it is almost always nearly entirely useless for stuff like this, because it only returns up to 1000 hits, and those are spent almost entirely on the same hits in the addons SDK over and over again. I don't know of a way to filter those out.


[1]: http://mxr.mozilla.org/mozilla-central/search?string=new+WeakMap%28%29&case=on&tree=mozilla-central

Comment 3

[Arpad Borsos [:Swatinem]]

1. sounds reasonable. Hopefully all firefox js should go through some kind of unit tests, so that should catch it.

Otherwise, since this is undocumented, and an experimental unfinished API, extensions should kind of expect it to break, no?

Comment 4

[Till Schneidereit [:till]]

(In reply to Arpad Borsos (Swatinem) from comment #3)
> Otherwise, since this is undocumented, and an experimental unfinished API,
> extensions should kind of expect it to break, no?

Yes, they should. Whether they do is a different matter altogether. The fact that it's undocumented makes me pretty hopeful, though, and also expect that a search through mozilla-central will turn up empty.

Comment 5

[Arpad Borsos [:Swatinem]]

Attachment: weakmapget.patch

Im trying to push a version with an assert to try, but try is really slow right now.
So lets see tomorrow if some of the tests hits that assertion.

Comment 6

[Arpad Borsos [:Swatinem]]

JS_ASSERT(args.length() == 1);

https://tbpl.mozilla.org/?tree=Try&rev=525bc83e932c

Lets see if any of the tests hit this assertion.

Comment 7

[Till Schneidereit [:till]]

(In reply to Arpad Borsos (Swatinem) from comment #6)
> Lets see if any of the tests hit this assertion.

I think we can count that as a "yes". ;)

I also just realized that an assert isn't too useful: it stops the entire test suite, and it doesn't even give us the info we need to fix the call site (because it doesn't give us the call site). At least not without in-depth analysis of the test case.

Instead, you could test for `args.length() > 1` and dump the JS stack in that case. Grepping through the logs would then give us the information we need.

Comment 8

[Arpad Borsos [:Swatinem]]

https://tbpl.mozilla.org/?tree=Try&rev=a8236264ca13
Ok, that should be clearer. Still need to grep through all the logs however.

Comment 9

[Arpad Borsos [:Swatinem]]

https://tbpl.mozilla.org/?tree=Try&rev=c490b8b58561
With the one case that was all over the logs fixed. I also suspect that I based my last try push on a broken inbound rev, so hopefully this will break less, haha.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8421821 [details] [diff] [review]
weakmapget.patch

Review of attachment 8421821 [details] [diff] [review]:
-----------------------------------------------------------------

That is a correct patch, if our plan is just to rip it out without warning about it. It might be better to get some data on who's using it in addon-land. If a bunch of our stuff broke... :-\

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 8421821 [details] [diff] [review]
weakmapget.patch

Review of attachment 8421821 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, warily

Comment 12

[Arpad Borsos [:Swatinem]]

There are still some uses in firefox itself, haven’t come around to filing bugs+patches for those yet.

Comment 13

[Arpad Borsos [:Swatinem]]

Some more callsites that are in the logs:
I’m currently on something else, so if someone wants to take this on:

modules/commonjs/sdk/page-worker.js:43
modules/commonjs/sdk/ui/state.js:88
modules/commonjs/sdk/io/buffer.js:143
modules/commonjs/sdk/io/buffer.js:147
browser/content/devtools/webaudioeditor-view.js:135
modules/accessibility/ContentControl.jsm:327

Comment 14

[André Bargull [:anba]]

WeakMap implementation was updated to conform to the latest spec draft in bug 1127827. Resolving as duplicate.

Comment 15

[Florian Scholz (Open Web Docs)]

Keywords and docs in bug 1127827 now, too.