Closed Bug 1101817 Opened 10 years ago Closed 8 years ago

Remove Weak{Map,Set}.prototype.clear

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: jorendorff, Assigned: till)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])

Attachments

(6 files, 2 obsolete files)

I guess TC39 decided to remove it? We'll see when the meeting notes appear.
Yep: rationale includes:

- it's a security problem for WeakMaps as private side tables
- it'd be a performance problem for implementations that want to implement WeakMaps as internal slots on objects stored as keys

I'll add a comment with the meeting minutes when they show up.

Dave
Also: WeakSet.prototype.clear
Keywords: dev-doc-needed
OS: Linux → All
Hardware: x86_64 → All
Summary: Remove WeakMap.prototype.clear → Remove Weak{Map,Set}.prototype.clear
Whiteboard: [DocArea=JS]
For anyone interested (and perhaps for the doc), a while back, we discussed on es-discuss with Jason and came up to the conclusion that weakmaps with and without .clear can be implemented on top of one another fairly easily.
https://mail.mozilla.org/pipermail/es-discuss/2013-January/028404.html
(more readable form) https://esdiscuss.org/topic/questioning-weakmap-prototype-clear

(non-standard, but gives the idea) implementation of weakmap-with-clear on top of weakmap-without-clear at :
https://mail.mozilla.org/pipermail/es-discuss/2013-January/028370.html + https://mail.mozilla.org/pipermail/es-discuss/2013-January/028371.html
I've updated the doc on MDN.
What it says on the tin.
Attachment #8663609 - Flags: review?(evilpies)
Assignee: nobody → till
Status: NEW → ASSIGNED
Attachment #8663609 - Flags: review?(evilpies) → review+
Till, can we get this landed? Noticed kangax es6 table tests this now.
Flags: needinfo?(till)
Weak{Map,Set}#clear were removed from the standard before ES6 was finalized, so we're removing it from our implementation, too.

Of course it's used in quite a few places in the platform. This patch removes it from all of those.

Review questions as follows:
yzen for EventManager.jsm
mak for browser-fullZoom.js, DrapPositionManager.jsm, and FrameTree.jsm
yoric for SessionStore.jsm
gijs for CustomizeMode.jsm and UITour.jsm
jlongster for the devtools stuff
Attachment #8663609 - Attachment is obsolete: true
Flags: needinfo?(till)
Attachment #8705991 - Flags: review?(yzenevich)
Attachment #8705991 - Flags: review?(mak77)
Attachment #8705991 - Flags: review?(jlong)
Attachment #8705991 - Flags: review?(gijskruitbosch+bugs)
Attachment #8705991 - Flags: review?(dteller)
I'm rubberstamping this myself.
Attachment #8705992 - Flags: review+
Does this need to be done in the github repo and then merged to m-c? I have no idea.
Attachment #8705993 - Flags: review?(dtownsend)
Ms2ger reviewed this on IRC half an eternity ago.
Attachment #8705994 - Flags: review+
See part 1 for clearAvailableTargetsCache
Attachment #8705995 - Flags: review?(mdeboer)
This didn't really change since the version evilpie reviewed.
Attachment #8705997 - Flags: review+
This is all about as green on try as can be expected:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f789c908ada1
Comment on attachment 8705995 [details] [diff] [review]
Part 5: Remove WeakMap.prototype.clear from Loop addon

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

Looking at part 1, this looks correct.
Attachment #8705995 - Flags: review?(mdeboer) → review+
Attachment #8705991 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8705991 [details] [diff] [review]
Part 1: Remove usage from various platform parts

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

/accessible part looks good
Attachment #8705991 - Flags: review?(yzenevich) → review+
Comment on attachment 8705991 [details] [diff] [review]
Part 1: Remove usage from various platform parts

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

lgtm
I concentrated on FrameTree.jsm and SessionStore.jsm, and got a cursory glance through the rest.
Attachment #8705991 - Flags: review?(dteller) → review+
Attachment #8705991 - Flags: review?(mak77) → review+
Comment on attachment 8705991 [details] [diff] [review]
Part 1: Remove usage from various platform parts

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

Looks good to me.
Attachment #8705991 - Flags: review?(jlong) → review+
Comment on attachment 8705993 [details] [diff] [review]
Part 3: Remove WeakMap.prototype.clear usages from Addons SDK

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

Looks like this is just a hack around WeakSet not being available earlier. Just land it in m-c.
Attachment #8705993 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/076c569c0d514a70765621a62d6a29a0389cc942
Bug 1101817 - Part 1: Remove usages of WeakMap.prototype.clear from Gecko. r=yzen,mak,yoric,gijs,jlongster

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfef72cb131be2dbfb65994298094a47292bb593
Bug 1101817 - Part 2: Remove WeakMap.prototype.clear usages from Shumway. r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e9998f9df0263f8607bf1fcdb2d375c47b1ba9e
Bug 1101817 - Part 3: Remove WeakMap.prototype.clear usages from Addons SDK. r=mossop

https://hg.mozilla.org/integration/mozilla-inbound/rev/98f924e16102ea4a8336a61e0cb6eb77f1c95823
Bug 1101817 - Part 4: Remove WeakMap.prototype.clear from web-platform test. r=Ms2ger

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4df90ac99641ebad07dddc00fa6452fb5552914
Bug 1101817 - Part 5: Remove WeakMap.prototype.clear from Loop addon. r=mikedeboer

https://hg.mozilla.org/integration/mozilla-inbound/rev/29fb9dd8cf23bff3b80e19d28dc0c5d9dd5ef52d
Bug 1101817 - Part 6: Remove Weak{Map,Set}.prototype.clear. r=evilpie
Thanks for all the quick reviews!
Blocks: 1239780
Attachment #8708002 - Attachment is obsolete: true
Depends on: 1243561
You need to log in before you can comment on or make changes to this bug.