Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow cloning WeakMaps/WeakSets with .from #26

Open
bakkot opened this issue Oct 22, 2019 · 23 comments
Open

Allow cloning WeakMaps/WeakSets with .from #26

bakkot opened this issue Oct 22, 2019 · 23 comments

Comments

@bakkot
Copy link
Collaborator

bakkot commented Oct 22, 2019

Currently WeakMap.from(x) requires x to be an iterable, which WeakMaps are not. It might make sense to allow x to be a WeakMap, in which case WeakMap.from(x) would return a copy of x. That's not currently possible in the language, but I am pretty confident it wouldn't violate any important guarantees.

Alternatively, we might say that WeakSet.prototype.union (see tc39/proposal-set-methods#23) is the right place for that ability. I am neutral on that question.

@zloirock
Copy link
Contributor

zloirock commented Oct 22, 2019

It looks wrong for me and somehow related to removing of WeakMap.prototype.clear. IIRC one of the problems was a performance problem for implementations that want to implement WeakMaps as internal slots on objects stored as keys.

@ljharb
Copy link
Member

ljharb commented Oct 22, 2019

They could still do that - by copying the internal slots over, for example.

@zloirock
Copy link
Contributor

In this case, it's also will be a performance problem. But IIRC it's only one of many reasons for removing WeakMap.prototype.clear.

@zloirock
Copy link
Contributor

zloirock commented Oct 22, 2019

@ljharb BTW, for example, it's our primitive slot-based WeakMap:

class WeakMap {
  static #ids = Symbol();
  #id = Symbol();
  constructor(init) {
    if (init) for (const el of init) this.set(el[0], el[1]);
  }
  delete(key) {
    if (key[WeakMap.#ids]) return key[WeakMap.#ids].delete(this.#id);
  }
  get(key) {
    if (key[WeakMap.#ids]) return key[WeakMap.#ids].get(this.#id);
  }
  has(key) {
    return !!key[WeakMap.#ids] && key[WeakMap.#ids].has(this.#id);
  }
  set(key, value) {
    if (!hasOwnProperty.call(key, WeakMap.#ids)) {
      Object.defineProperty(key, WeakMap.#ids, { value: new Map() });
    }
    key[WeakMap.#ids].set(this.#id, value);
    return this;
  }
}

How do you implement cloning without performance degradation (at least on cloned weak maps)?

@ljharb
Copy link
Member

ljharb commented Oct 22, 2019

static from(weakMap) {
  const wm = new WeakMap():
  wm[ids] = weakMap[ids];
  return wm;
}

@zloirock
Copy link
Contributor

zloirock commented Oct 22, 2019

And, in this case, any changes on clone affect original, on original - clone (if I understood your idea correctly, since ids is not a weakmap field). It's not a clone, it's just a new reference to old weakmap. If each weakmap will have a set of ids - it will cause performance degradation for clones - at least 2x for the first, 1001x for 1000th.

@ljharb
Copy link
Member

ljharb commented Oct 22, 2019

Ah, that's true. In that case, there's no way to avoid enumeration for a polyfill, but performance considerations of userland implementations don't tend to hold much weight when designing a feature, historically.

@zloirock
Copy link
Contributor

One more time - it's not about userland, it's about engines implementations which works in this way, see my first comment.

@ljharb
Copy link
Member

ljharb commented Oct 22, 2019

I'd be happy to hear from engine implementors if that in fact is a concern.

@zloirock
Copy link
Contributor

I don't know how it works in popular engines, but at least it will cause problems for developers of new engines. Also interesting opinions of implementors -)

@zloirock
Copy link
Contributor

zloirock commented Oct 22, 2019

Also, it will cause some security issues. For example:

// frozen env where we can't patch WeakSet and methods
// somehow we got access to a weakset
const clone = WeakSet.from(weakset);
// sometime later the `weakset` is cleared or just `object` key removed from it
// and now we have access to the `object`
clone.has(object);
// we could know that the `object` was in the `weakset` in the moment of creating a clone

It's far-fetched, but the idea of WeakMap / WeakSet is that we can't observe keys.

@bakkot
Copy link
Collaborator Author

bakkot commented Oct 22, 2019

I agree that this is a capability which was not previous present, but I don't think it's likely to be a security issue. The important guarantee that a WeakMap provides is that having the map does not allow you to access its contents. That guarantee is preserved.

@zloirock
Copy link
Contributor

In this case, making all private fields public in the next version of an abstract language is not a security issue, just a new possibility.

I think that it should solve the committee.

@bakkot
Copy link
Collaborator Author

bakkot commented Oct 22, 2019

... what?

@zloirock
Copy link
Contributor

  • Is the cloning of weakmaps a serious complication / performance issue for implementers.
  • Is the observability of keys a security issue.

@bakkot
Copy link
Collaborator Author

bakkot commented Oct 22, 2019

I don't think this makes keys observable

@zloirock
Copy link
Contributor

It makes keys observable, an example above.

@ljharb
Copy link
Member

ljharb commented Oct 22, 2019

If you have object, you already know if it's in weakset, so knowing if it's in clone isn't new information.

@bakkot
Copy link
Collaborator Author

bakkot commented Oct 22, 2019

The example above does not demonstrate keys being observable.

@zloirock
Copy link
Contributor

zloirock commented Oct 22, 2019

@ljharb at the moment of creation clone we haven't access to the object, when we have access to the object it's missed in the weakset. It's observability of a private flag.

@zloirock
Copy link
Contributor

@bakkot one more time - lets the committee solve is this a security issue or not. In my opinion - yes, since I see some places where it can be exploited.

@ljharb
Copy link
Member

ljharb commented Oct 22, 2019

I see what you're saying, but you also can't know if someone did clone.add(object) or weakset.delete(object) in the "some time after" interim (and if you can know that, there's no new information to be had).

@zloirock
Copy link
Contributor

@ljharb since we create a clone, we have full control over it. About weakset.delete(object) or another operation - yes, but now we have a chance to find it out - that's what I wrote.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants