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

Handle garbage collection #322

Merged
merged 2 commits into from
Aug 26, 2021
Merged

Handle garbage collection #322

merged 2 commits into from
Aug 26, 2021

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Aug 3, 2021

The following tasks have been completed:

  • Modified Web platform tests (link to pull request)

Implementation commitment:

Handles this case:

async function run() {
  const sentinel = await navigator.wakeLock.request("screen");
  sentinel.addEvenListener("release", function() {
       // do stuff
       console.log("Sentinel released!");
  }, { once: true });
  return; 
  // `sentinel` shouldn't get CG here, because we have an event registered
  // "once" removes the listener, allowing the object to be GC'ed.
}

run(); 

I don't know if we have the means to test the above via WPT... can folks think of a way of doing it without a manual test?

Presumedly, Chrome already implements the right thing here.


Preview | Diff

@marcoscaceres marcoscaceres requested a review from rakuco August 3, 2021 04:31
@marcoscaceres
Copy link
Member Author

Checked Chrome manually. It's implemented. I checked it by switching tabs and forcing the release event to fire, without manually releasing it.

Copy link
Member

@reillyeon reillyeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with one comment. I'm also curious if there's a general guide to adding these kinds of considerations to specifications. It's come up occasionally in other APIs and I've never been sure if it is considered implementation-specific behavior.

index.html Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Member Author

for other folks that are interested, I filed a bug on the DOM spec about garbage collection + constructed objects on which events can be registered:
whatwg/dom#1003

@kenchris, maybe something the TAG might take up and add to the "the guide".

@marcoscaceres marcoscaceres merged commit 14f601c into gh-pages Aug 26, 2021
@marcoscaceres marcoscaceres deleted the memory_management branch August 26, 2021 01:49
@rakuco
Copy link
Member

rakuco commented Aug 26, 2021

(for completeness, the current Chromium code was added in https://chromium-review.googlesource.com/c/chromium/src/+/1868875 with some unit tests that basically check this behavior. I couldn't find a way to write a web test for it back then, even if I made it Chromium-specific and used self.gc())

@marcoscaceres
Copy link
Member Author

For a little bit of a giggle, this what I did for permissions:

web-platform-tests/wpt#30190

We could do the same here… it’s not 100% assured to work, but might be good enough.

The main thing that we want to check is that the event actually fires.

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 this pull request may close these issues.

3 participants