Closed Bug 933467 Opened 8 years ago Closed 6 years ago

Shumway needs iterable weakly-keyed maps

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: till, Unassigned)

References

Details

(Whiteboard: [shumway:p1])

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
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.
Benvie created this implementation based on Cu.getWeakReference: https://gist.github.com/Benvie/7123690
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.
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.
> 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.
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
(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.
(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.
Is ShumwayWorker a separate thread? Like a web worker with some special features?
Then xpconnect stuff can't be used there.
(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.
(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.
Depends on: 949992
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?
(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.
Depends on: ExactRooting
Whiteboard: [Shumway:m2] → [shumway:p1]
Assignee: general → nobody
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.