933467 - Shumway needs iterable weakly-keyed maps

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

AS3's dictionary is iterable, and it is optionally weakly-keyed. Currently, we just always let the keys be strong references. Since this feature is widely used, that means that we frequently leak memory (and event listeners).

There's a strawman for Weak References for ES.Next[1], which would allow us to implement this in JS. A prototype implementation is attached. This strawman apparently has general consensus in TC39, but there's serious pushback from people working on our GC and XPConnect.

So I guess it's safe to assume that this at least won't happen any time soon.

Which leaves us with a second option: exposing iterability on WeakMaps in the ShumwayWorker. (Which we need anyway for bug 930908.) This should be fairly simple to implement, as terrence tells me that, internally, such an iterator exists.


[1]: http://wiki.ecmascript.org/doku.php?id=strawman:weak_references

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

And serious pushback from people working on our Web APIs.
In DOM world any API which exposes GC behavior is broken by definition and must not be implemented.

Exposing WeakRefs or similar to Gecko only stuff is "fine", and we do that already via
xpconnect https://developer.mozilla.org/en-US/docs/Components.utils.getWeakReference.

Comment 2

[Till Schneidereit [:till]]

Benvie created this implementation based on Cu.getWeakReference: https://gist.github.com/Benvie/7123690

Comment 3

[Steve Fink [:sfink] [:s:]]

Do you need them to be both iterable and weakly-keyed at the same time?

As in, what if a dictionary held its keys weakly as long as you didn't iterate over it? As soon as you requested its @@iterator (or whatever), it would switch to holding them strongly.

It would be a strange sort of action at a distance. And you might want to allow a Debugger object to inspect it without triggering the strong reference poison pill.

Comment 4

[Andrew McCreight [:mccr8]]

This is already implemented for weakmaps (available only to browser code), as Components.utils.nondeterministicGetWeakMapKeys.  We even use it as part of the implementation of BrowserElementParent.

Comment 5

[Andrew McCreight [:mccr8]]

> Do you need them to be both iterable and weakly-keyed at the same time?
That doesn't really help.  You can still observe GC behavior.

Comment 6

[Steve Fink [:sfink] [:s:]]

Never mind, that doesn't work. You'd still be able to observe whether a key is still in the table at the time of the first iteration.

  create dictionary
  put a key in it with no other references
  ...do stuff...
  iterate over the dictionary to see if it has any keys

Comment 7

[Brandon Benvie [:benvie]]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> This is already implemented for weakmaps (available only to browser code),
> as Components.utils.nondeterministicGetWeakMapKeys.  We even use it as part
> of the implementation of BrowserElementParent.

This can be problematic for implementing an iterator because you have to hold the array of keys for the duration of iteration (or at least keep so far unvisited keys) making them strongly held for the life of iterator.

The implementation in gist above that uses Cu.getWeakReference is better since you only strongly hold a reference to the single current element at a time.

Comment 8

[Till Schneidereit [:till]]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> > Do you need them to be both iterable and weakly-keyed at the same time?
> That doesn't really help.  You can still observe GC behavior.

Besides that, we *do* need both at the same time. Weakly-keyed dictionaries are used as a way to implement weak references in Flash, so they're frequently iterated with the expectation that the keys keep being weak.

Thanks, mccr8, for the pointer to nondeterministicGetWeakMapKeys. Either that or benvie's implementation based on getWeakReference would work for our needs: I don't think all keys being strong refs during iteration is a concern for us.

We'd still need to make one of these available in our worker, but that doesn't seem too bad.

The upside to having ES.Next Weak References would be that we have to use less non-standard stuff, of course.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Is ShumwayWorker a separate thread? Like a web worker with some special features?
Then xpconnect stuff can't be used there.

Comment 10

[Till Schneidereit [:till]]

(In reply to Olli Pettay [:smaug] from comment #9)
> Is ShumwayWorker a separate thread? Like a web worker with some special
> features?
> Then xpconnect stuff can't be used there.

Yes, it is. And that's what I meant: we have to expose this stuff in some other way.

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to Brandon Benvie [:benvie] from comment #7)
> (In reply to Andrew McCreight [:mccr8] from comment #4)
> > This is already implemented for weakmaps (available only to browser code),
> > as Components.utils.nondeterministicGetWeakMapKeys.  We even use it as part
> > of the implementation of BrowserElementParent.
> 
> This can be problematic for implementing an iterator because you have to
> hold the array of keys for the duration of iteration (or at least keep so
> far unvisited keys) making them strongly held for the life of iterator.

Yes in theory, but I don't think that is actually bad in practice. Iterators are transient in practice. You use them in a single for-of loop, you drop them, they get GC'd.

Comment 12

[Masatoshi Kimura [:emk]]

Are AS weakly-keyed dictionaries sensitive to GC? If so, we have no choice. Even if it is broken, we have to implement the broken garbage as-is. If not, how AS implements it?

Comment 13

[Till Schneidereit [:till]]

(In reply to Masatoshi Kimura [:emk] from comment #12)
> Are AS weakly-keyed dictionaries sensitive to GC? If so, we have no choice.
> Even if it is broken, we have to implement the broken garbage as-is. If not,
> how AS implements it?

I'm not sure I understand what you mean by "sensitive to GC". It allows you to get information about GC timings, if that's what you mean. And, in combination with conservative rooting, it enables some more insidious stuff: https://github.com/justdionysus/gcwoah

Note, however, that the Flash Player has since been switched to exact rooting, so this particular attack doesn't apply to it, anymore. Well, and we will have exact rooting enabled long before Shumway gets enabled, too.

Comment 14

[Till Schneidereit [:till]]

This is now available in the extension. In the web viewer version, it's not possible to support until weak references become available, but that's ok for us.