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.

Sign up for GitHub

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

Jump to bottom

Fix a possible double-borrow during event handling #1512

Merged
merged 4 commits into from
Apr 11, 2020
Merged

Fix a possible double-borrow during event handling #1512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
efd8450
Fix a possible double-borrow during event handling
ryanisaacg Mar 16, 2020
a9e59dc
Fmt pass
ryanisaacg Mar 16, 2020
d4afa1f
Changelog
ryanisaacg Mar 16, 2020
a190417
Merge remote-tracking branch 'upstream/master' into fix-double-borrow
ryanisaacg Apr 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Unreleased

- On X11, fix `ResumeTimeReached` being fired too early.
- On Web, fix a possible panic during event handling

# 0.22.0 (2020-03-09)

Expand Down
5 changes: 3 additions & 2 deletions src/platform_impl/web/event_loop/runner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,6 @@ impl<T: 'static> Shared<T> {
// It should only ever be called from send_event
fn handle_event(&self, event: Event<'static, T>, control: &mut root::ControlFlow) {
let is_closed = self.is_closed();

match *self.0.runner.borrow_mut() {
Some(ref mut runner) => {
// An event is being processed, so the runner should be marked busy
Expand All @@ -227,7 +226,9 @@ impl<T: 'static> Shared<T> {
// If the runner doesn't exist and this method recurses, it will recurse infinitely
if !is_closed && self.0.runner.borrow().is_some() {
// Take an event out of the queue and handle it
if let Some(event) = self.0.events.borrow_mut().pop_front() {
// Make sure not to let the borrow_mut live during the next handle_event
let event = { self.0.events.borrow_mut().pop_front() };
if let Some(event) = event {
Copy link
Contributor

@filnet filnet Mar 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I am new to Rust, but this fix looks like it shouldn't be necessary.
Is the borrow_mut alive for the whole scope of the if let?

PS: I am sure the fix is adequate. I am having a hard time understanding the issue and fix Rust wise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I assumed also, which is why I ended up writing the bug in the first place 😛

I'd guess it has to do with the scope the value is alive for? Even if a value isn't used, I believe the drop is performed at the end of the scope it lives in. Therefore the old borrow lasted until the end of the if let (or maybe even the outer block) even though it could have been dropped earlier.

Copy link
Contributor

@filnet filnet Mar 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The if let scope: https://users.rust-lang.org/t/curious-scope-rules-when-using-if-let/1858/2

self.handle_event(event, control);
}
}
Expand Down