Closed Bug 1101817 Opened 9 years ago Closed 8 years ago

Remove Weak{Map,Set}.prototype.clear


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox46 --- fixed


(Reporter: jorendorff, Assigned: till)


(Blocks 1 open bug)


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


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

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.
(more readable form)

(non-standard, but gives the idea) implementation of weakmap-with-clear on top of weakmap-without-clear at : +
I've updated the doc on MDN.
What it says on the tin.
Attachment #8663609 - Flags: review?(evilpies)
Assignee: nobody → till
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:
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]:

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+
Bug 1101817 - Part 1: Remove usages of WeakMap.prototype.clear from Gecko. r=yzen,mak,yoric,gijs,jlongster
Bug 1101817 - Part 2: Remove WeakMap.prototype.clear usages from Shumway. r=me
Bug 1101817 - Part 3: Remove WeakMap.prototype.clear usages from Addons SDK. r=mossop
Bug 1101817 - Part 4: Remove WeakMap.prototype.clear from web-platform test. r=Ms2ger
Bug 1101817 - Part 5: Remove WeakMap.prototype.clear from Loop addon. r=mikedeboer
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.