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

Web backend does not handle ControlFlow::Exit nicely #1688

Closed
alvinhochun opened this issue Aug 29, 2020 · 5 comments · Fixed by #1715
Closed

Web backend does not handle ControlFlow::Exit nicely #1688

alvinhochun opened this issue Aug 29, 2020 · 5 comments · Fixed by #1715

Comments

@alvinhochun
Copy link
Contributor

  1. Bug: The code in runner::Shared::handle_event does not properly enforce that ControlFlow::Exit is sticky. It only checks for self.is_closed() and does not check the actual user-assigned control out parameter. (I'm fixing this in a PR, just including this here for completeness.)
  2. After the event loop has ended, the canvas is not removed from the DOM or blanked and the event handlers are not removed.
@ryanisaacg
Copy link
Contributor

  1. Thanks for the fix!
  2. The canvas is also not inserted into the DOM nor is it rendered to by Winit. Removing the event handlers does make sense to me, but the others can be handled by the end user (who presumably has inserted the canvas where they please and has rendered to it how they please.)

@alvinhochun
Copy link
Contributor Author

Ah, I saw code that creates a canvas element but didn't realize that the element doesn't get inserted to the DOM. I suppose it also makes sense for the user to clear the canvas at the end.

@alvinhochun
Copy link
Contributor Author

I'm giving this a try (likely only for web-sys). The changes should include:

  • Make Window deregister its event listeners when dropped and have it emit WindowEvent::Destroyed.
  • Make EventLoop deregister its event listeners and also the event handlers of all of its Windows (since they aren't dropped) on ControlFlow::Exit.
  • Fix the "closure invoked recursively or destroyed already" error that causes Event::LoopDestroyed to not be fired on beforeunload (i.e. when leaving the page).

@ryanisaacg I have some questions for you:

  • The event loop Rc reference count will probably drop to 0 after deregistering the listeners, and the event handler closure will be dropped. Should this be the expected behaviour, or should the code intentionally leak the Rc such that it never gets dropped?
  • Should any clean-up happen on beforeunload? I'm inclined to not perform any clean-up other than emitting Event::LoopDestroyed, but this would mean that the event handler closure won't get dropped.

In the current web backend, nothing in the event loop is dropped in any case.

@alvinhochun
Copy link
Contributor Author

  1. The canvas is also not inserted into the DOM nor is it rendered to by Winit. Removing the event handlers does make sense to me, but the others can be handled by the end user (who presumably has inserted the canvas where they please and has rendered to it how they please.)

I don't know how I missed this before, but I just noticed that the current implementation does actually remove the canvas in:

impl Drop for Common {
fn drop(&mut self) {
self.raw.remove();
}
}

The catch is that it only runs when a Window is explicitly dropped, which is why I don't see it happen when the event loop ends.

For the reason you pointed out, this code should be removed, which will be a breaking change.

@ryanisaacg
Copy link
Contributor

The event loop Rc reference count will probably drop to 0 after deregistering the listeners, and the event handler closure will be dropped. Should this be the expected behaviour, or should the code intentionally leak the Rc such that it never gets dropped?

Dropping the event handlers should be fine, I don't see any problems that we'll run into there.

Should any clean-up happen on beforeunload? I'm inclined to not perform any clean-up other than emitting Event::LoopDestroyed, but this would mean that the event handler closure won't get dropped.

I don't think clean-up should be necessary here, because the webpage is going to be cleaned up by the browser anyhow.

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

Successfully merging a pull request may close this issue.

2 participants