Questioning WeakMap.prototype.clear (was: Private symbols as WeakMap sugar)
On Mon, Jan 21, 2013 at 6:04 AM, David Bruant <bruant.d at gmail.com> wrote:
I think WeakMap.prototype.clear slipped through the crack without being specifically discussed. Based on what's publicly available, I don't see anyone noticed and discussed the fact that WeakMap.prototype.clear questions the property that was true before its adoption ("you can only modify a weakmap entry if you have the key")
I agree and disagree. I disagree because WeakMap.prototype.clear() doesn't modify the weakmap entry in the same direct way, ie. WeakMap.prototype.set, WeakMap.prototype.delete. On the other hand, I agree because it provides (if the weakmap is exposed) a way to remove an entry that might be used without any defense, ie. WeakMap.prototype.has() or might leave the program in an unstable state due to loss of some aggregate state information stored in the weakmap.
That being said, I support the inclusion of WeakMap.prototype.clear(), because reasonable security and state defense expectations can be met by:
- Not exposing a sensitive weakmap in program code
- Defending against missing entries with has()
Subjectively, #1 mitigates without nannying.
If you don't want to expose clear on a WeakMap, create a subclass that doesn't support it:
class WeakMapWithoutClear extends WeakMap { delete() {throw Error("Clearing this map is not allowed"); }
if you are really paranoid that somebody is going to do WeakMap.prototype.clear.call(myWeakMapWithoutClear); then don't expose myWeakMapWithoutClear to untrusted parties or only expose wrapper object that hides the WeakMap as private state.
clear is an operation that can not be otherwise synthesized for WeakMaps. Given the ambient impact of the mere existance of WeakMap entries on garbage collection algorithms, it seems likely that will be performance advantages in some situations to proactively clear a WeakMap rather than waiting for the GC to do so. Having clear enables this while it doesn't prevent using WeakMaps in ways that don't expose that operation to ambient use.
On Jan 21, 2013, at 3:04 AM, David Bruant wrote:
Le 21/01/2013 11:58, David Bruant a écrit :
Before the clear method, weakmaps had the following property: "you can only modify a weakmap entry if you have the key". This property isn't true anymore with .clear. This may be considered as abusive ambient authority. Let's see how this feature came to appear: Jason Orendorff talked about Map.prototype.clear [1] (Oct 22nd). Seen as a good idea. No discussion on whether it's a good idea for WeakMaps specifically. Nicholas Zakas briefly mentions it in November [2]. No one replied to it specifically. I haven't seen any discussion about it in meeting notes [3]. A brief mention of Set/Map.prototype.clear [4] as a review of the Oct 26th draft [5] (note, 4 days after Jason post, which is a very short amount of time) but nothing about WeakMap.prototype.clear. Implemented in Firefox soon after [6]... I think WeakMap.prototype.clear slipped through the crack without being specifically discussed. Based on what's publicly available, I don't see anyone noticed and discussed the fact that WeakMap.prototype.clear questions the property that was true before its adoption ("you can only modify a weakmap entry if you have the key")
As much as makes sense we want Map, WeakMap, and Set to expose the same interfaces. Any new method added to one is going to get added to all of them, unless it either simply makes no sense for one of them or if somebody can make a convincing argument for excluding it for one of them.
I think the property I mentioned is cricial to weakmap integrity and I think WeakMap.prototype.clear should be considered for removal... or at least proper discussion since none really happened from what I've found.
TC39 doesn't have a process that requires a priori discussion on es-discuss of every design detailed before it can become part of specification draft. I'm pretty sure we would never finish anything it that was the case. The spec. drafts exist so the actual details of the proposed specification an be reviewed and discussed like this.
somebody asked why clear() is needed, when if you have the reference you can simply ref = new WeakMap; instead of clearing, while clear() exposes undesired side effects. Said that, even localStorage lets us remove all keys even if it wasn't us setting them so ... well, I guess we can survive then with clear().
This is specially for Rick, since it's holidays today and I am bored home :D Where is exactly the latest state of es6 collections? I have no idea how I should update mine accordingly, please point me to the very latest, most updated, agreed link 'cause I am confused.
Thanks and have a nice day or evening
On Jan 21, 2013, at 9:38 AM, Andrea Giammarchi wrote:
somebody asked why clear() is needed, when if you have the reference you can simply ref = new WeakMap; instead of clearing, while clear() exposes undesired side effects. Said that, even localStorage lets us remove all keys even if it wasn't us setting them so ... well, I guess we can survive then with clear().
This is specially for Rick, since it's holidays today and I am bored home :D Where is exactly the latest state of es6 collections? I have no idea how I should update mine accordingly, please point me to the very latest, most updated, agreed link 'cause I am confused.
The best way to contribute to the ES6 development process is to read and comment on the actual specification drafts.
No markup version of the latest ES6 spec (pdf link)
harmony:working_draft_ecma-262_edition_6_12-21-12-nomarkup.pdf
Sorry for the echo
On Mon, Jan 21, 2013 at 3:04 AM, David Bruant <bruant.d at gmail.com> wrote:
Le 21/01/2013 11:58, David Bruant a écrit :
Before the clear method, weakmaps had the following property: "you can only modify a weakmap entry if you have the key". This property isn't true anymore with .clear. This may be considered as abusive ambient authority.
Let's see how this feature came to appear: Jason Orendorff talked about Map.prototype.clear [1] (Oct 22nd). Seen as a good idea. No discussion on whether it's a good idea for WeakMaps specifically. Nicholas Zakas briefly mentions it in November [2]. No one replied to it specifically. I haven't seen any discussion about it in meeting notes [3]. A brief mention of Set/Map.prototype.clear [4] as a review of the Oct 26th draft [5] (note, 4 days after Jason post, which is a very short amount of time) but nothing about WeakMap.prototype.clear. Implemented in Firefox soon after [6]... I think WeakMap.prototype.clear slipped through the crack without being specifically discussed. Based on what's publicly available, I don't see anyone noticed and discussed the fact that WeakMap.prototype.clear questions the property that was true before its adoption ("you can only modify a weakmap entry if you have the key")
I think the property I mentioned is cricial to weakmap integrity and I think WeakMap.prototype.clear should be considered for removal... or at least proper discussion since none really happened from what I've found.
Hi David, thanks for raising this. Not having been able to take much time lately, I had not been aware that a .clear method had been added. I think it's a terrible idea. I like your explanation of why -- because of the simple property it breaks. Indeed, this violation contradicts the main use case of WeakMaps -- for soft fields.
On Mon, Jan 21, 2013 at 9:02 AM, Allen Wirfs-Brock <allen at wirfs-brock.com> wrote:
If you don't want to expose clear on a WeakMap, create a subclass that doesn't support it:
class WeakMapWithoutClear extends WeakMap { delete() {throw Error("Clearing this map is not allowed"); }
Did you mean "clear()" rather than "delete()" ? Assuming so, how does this defend against
In any case, your argument cuts both ways. Given WeakMaps without .clear,
// note: implements the WeakMap API but does not extend WeakMap. class WeakMapWithClear { // doesn't matter whether private is implemented using WeakMap or private symbols // as long as it provides real encapsulation. private let wrapped; // not const, however we say this for a field constructor() { wrapped = new WeakMap(); } get(key) => wrapped.get(key), set(key, val) => wrapped.set(key, value), has(key) => wrapped.has(key), delete(key) => wrapped.delete(key), clear() => { wrapped = null; } }
Btw, as a separate point, notice that the above code becomes insecure if .set and .delete implicitly chain, unless we wrap curlies around the body. Not a strong point by itself because it's not hard to wrap curlies around the body, but does show that chaining creates a hazard that is easy to trip on.
if you are really paranoid that somebody is going to do WeakMap.prototype.clear.call(myWeakMapWithoutClear); then don't expose myWeakMapWithoutClear to untrusted parties or only expose wrapper object that hides the WeakMap as private state.
Please don't speak out of both sides of your mouth in this way. First you show a piece of code as a supposed solution to the problem you raise. Then in an aside, you admit your solution is broken but do not show the code needed to actually fix it, leading to an underestimation of costs. My code above shows the cost of wrapping to get the dual effect. It's pay as you go -- the built-in weakmaps do not need to pay the costs of .clear. Note that in the reversed weakmap representation we're talking about, which will be common, these costs are real.
clear is an operation that can not be otherwise synthesized for WeakMaps.
See counter example above.
Given the ambient impact of the mere existance of WeakMap entries on garbage collection algorithms, it seems likely that will be performance advantages in some situations to proactively clear a WeakMap rather than waiting for the GC to do so. Having clear enables this while it doesn't prevent using WeakMaps in ways that don't expose that operation to ambient use.
This argument is backwards. It is the presence of .clear that prevents the non-ephemeron optimization for the common rights-amplification pattern.
On Mon, Jan 21, 2013 at 9:38 AM, Andrea Giammarchi <andrea.giammarchi at gmail.com> wrote:
somebody asked why clear() is needed, when if you have the reference you can simply ref = new WeakMap;
Thanks for pointing this out. The clear method in my wrapper should have been
clear() { wrapped = new WeakMap(); }On Mon, Jan 21, 2013 at 10:40 AM, Mark S. Miller <erights at google.com> wrote:
On Mon, Jan 21, 2013 at 9:02 AM, Allen Wirfs-Brock <allen at wirfs-brock.com> wrote:
If you don't want to expose clear on a WeakMap, create a subclass that doesn't support it:
class WeakMapWithoutClear extends WeakMap { delete() {throw Error("Clearing this map is not allowed"); }
Did you mean "clear()" rather than "delete()" ? Assuming so, how does this defend against
The above fragment was a leftover from a partial edit, sorry. I had written your clear.call attack before seeing that you'd already covered that below and suggested the wrapping defense.
On Mon, Jan 21, 2013 at 10:40 AM, Mark S. Miller <erights at google.com> wrote:
if you are really paranoid that somebody is going to do WeakMap.prototype.clear.call(myWeakMapWithoutClear); then don't expose myWeakMapWithoutClear to untrusted parties or only expose wrapper object that hides the WeakMap as private state.
Please don't speak out of both sides of your mouth in this way.
My apologies to you and to the whole list for this phrasing. It is offensive and uncalled for. It was your "really paranoid" that set me off, but that is no excuse. Really, I am sorry.
That said, please let us proceed without being dismissive of imposing extra costs and hazards on programming securely. It's already hard enough without these.
On Jan 21, 2013, at 10:20 AM, Mark S. Miller wrote:
Hi David, thanks for raising this. Not having been able to take much time lately, I had not been aware that a .clear method had been added. I think it's a terrible idea. I like your explanation of why -- because of the simple property it breaks. Indeed, this violation contradicts the main use case of WeakMaps -- for soft fields.
I really have to disagree about the main use case of WeakMaps. I asset that the main use is for representing an extensible set of external relationships among objects in a manner that does cause memory leaks.
A soft field (and arguably all ES properties on non-sealed objects are themselves "soft field") is another way to represent such relationships. Alternatively, if soft fields aren't available an external relationship store can be used to represent them.
Even if soft fields are a primary use case, nothing says that all soft fields must be high integrity fields just as not all objects or object properties need to be high integrity properties.
On Jan 21, 2013, at 10:46 AM, Mark S. Miller wrote:
On Mon, Jan 21, 2013 at 10:40 AM, Mark S. Miller <erights at google.com> wrote:
On Mon, Jan 21, 2013 at 9:02 AM, Allen Wirfs-Brock <allen at wirfs-brock.com> wrote:
If you don't want to expose clear on a WeakMap, create a subclass that doesn't support it:
class WeakMapWithoutClear extends WeakMap { delete() {throw Error("Clearing this map is not allowed"); }
Did you mean "clear()" rather than "delete()" ? Assuming so, how does this defend against
The above fragment was a leftover from a partial edit, sorry. I had written your clear.call attack before seeing that you'd already covered that below and suggested the wrapping defense.
clear is an operation that can not be otherwise synthesized for WeakMaps.
See counter example above.
No, you ES level code does not have the immediately effect of freeing up GC resources that are associated with maintain the WeakMap. Also, not tht in a generation collection there may be a very long time interval before the discard WeakMap is recognized as such.
You can not synthesize the implementation level memory management optimizations that are possible using a built-in clear method.
Given the ambient impact of the mere existance of WeakMap entries on garbage collection algorithms, it seems likely that will be performance advantages in some situations to proactively clear a WeakMap rather than waiting for the GC to do so. Having clear enables this while it doesn't prevent using WeakMaps in ways that don't expose that operation to ambient use.
This argument is backwards. It is the presence of .clear that prevents the non-ephemeron optimization for the common rights-amplification pattern.
I'm actually primarily interested in uses of WeakMap where ephemeron behavior is necessarly -- representing external relationships between arbitrary objects.
Le 21/01/2013 11:58, David Bruant a écrit :
Let's see how this feature came to appear: Jason Orendorff talked about Map.prototype.clear [1] (Oct 22nd). Seen as a good idea. No discussion on whether it's a good idea for WeakMaps specifically. Nicholas Zakas briefly mentions it in November [2]. No one replied to it specifically. I haven't seen any discussion about it in meeting notes [3]. A brief mention of Set/Map.prototype.clear [4] as a review of the Oct 26th draft [5] (note, 4 days after Jason post, which is a very short amount of time) but nothing about WeakMap.prototype.clear. Implemented in Firefox soon after [6]... I think WeakMap.prototype.clear slipped through the crack without being specifically discussed. Based on what's publicly available, I don't see anyone noticed and discussed the fact that WeakMap.prototype.clear questions the property that was true before its adoption ("you can only modify a weakmap entry if you have the key")
I think the property I mentioned is cricial to weakmap integrity and I think WeakMap.prototype.clear should be considered for removal... or at least proper discussion since none really happened from what I've found.
David
[1] esdiscuss/2012-October/025962 [2] esdiscuss/2012-November/026114 [3] rwldrn/tc39-notes [4] rwldrn/tc39-notes/blob/master/es6/2012-11/nov-27.md#review-of-new-draft-spec [5] harmony:specification_drafts [6] bugzilla.mozilla.org/show_bug.cgi?id=814562
Le 21/01/2013 11:58, David Bruant a écrit : > Before the clear method, weakmaps had the following property: "you can > only modify a weakmap entry if you have the key". This property isn't > true anymore with .clear. This may be considered as abusive ambient > authority. Let's see how this feature came to appear: Jason Orendorff talked about Map.prototype.clear [1] (Oct 22nd). Seen as a good idea. No discussion on whether it's a good idea for WeakMaps specifically. Nicholas Zakas briefly mentions it in November [2]. No one replied to it specifically. I haven't seen any discussion about it in meeting notes [3]. A brief mention of Set/Map.prototype.clear [4] as a review of the Oct 26th draft [5] (note, 4 days after Jason post, which is a very short amount of time) but nothing about WeakMap.prototype.clear. Implemented in Firefox soon after [6]... I think WeakMap.prototype.clear slipped through the crack without being specifically discussed. Based on what's publicly available, I don't see anyone noticed and discussed the fact that WeakMap.prototype.clear questions the property that was true before its adoption ("you can only modify a weakmap entry if you have the key") I think the property I mentioned is cricial to weakmap integrity and I think WeakMap.prototype.clear should be considered for removal... or at least proper discussion since none really happened from what I've found. David [1] https://mail.mozilla.org/pipermail/es-discuss/2012-October/025962.html [2] https://mail.mozilla.org/pipermail/es-discuss/2012-November/026114.html [3] https://github.com/rwldrn/tc39-notes [4] https://github.com/rwldrn/tc39-notes/blob/master/es6/2012-11/nov-27.md#review-of-new-draft-spec [5] http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts [6] https://bugzilla.mozilla.org/show_bug.cgi?id=814562