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

PermissionStatus's memory management is unclear #170

Closed
smaug---- opened this issue Feb 13, 2018 · 3 comments · Fixed by #256
Closed

PermissionStatus's memory management is unclear #170

smaug---- opened this issue Feb 13, 2018 · 3 comments · Fixed by #256

Comments

@smaug----
Copy link

Since permission may change at any point, PermissionStatus should stay alive (at least in its current form) as long as it has any change event listeners.
That would follow the model WebSocket for example has.
https://html.spec.whatwg.org/#garbage-collection-3
This all to avoid exposing gc behavior to the web.

@marcoscaceres
Copy link
Member

@smaug----, this seems pretty straightforward to add (based on WebSockets in HTML).

I'm wondering, what are some of the interop implications we should watch for here? Anything observable and/or testable that would result from adding the memory management?... I'm assuming no, but what to make sure.

(maybe related to #162)

@smaug----
Copy link
Author

Can wpt test changing permissions? If it can, this should be testable: create PermissionStatus, add event listener and drop any explicit references to the object. And then try to force a GC (allocate lots of memory or something) and then change permission.

@marcoscaceres
Copy link
Member

Ah, got it! Yes, changing permissions should be testable through Web Driver (at least in Chrome, for now). I'd be reluctant to make any assurances about about CG actually occurring, but I'll throw in a bunch of iframes in the hope that creates enough garbage to cause CG.

In any case, just doing the setup in a block scope should be enough for to make an instance a CG target.

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

Successfully merging a pull request may close this issue.

2 participants