Removed sources of panic from ports/glutin.

[asajeffrey]

Fixes #10547.

---

This change is

[samlh]

Return true? #10547 implies that this can happen when the window is closing.

[asajeffrey]

I wasn't sure about what to do here, most likely the window is already closing, and I'm not sure what'll happen if we try to close it twice? Is it idempotent?

[KiChjang]

I can't make a call on what to do about the returned value here. r? @pcwalton

[bors-servo]

☔ The latest upstream changes (presumably #10935) made this pull request unmergeable. Please resolve the merge conflicts.

[bors-servo]

☔ The latest upstream changes (presumably #11041) made this pull request unmergeable. Please resolve the merge conflicts.

[asajeffrey]

Rebased.

[asajeffrey]

Rebased and bumped log to 0.3.6.

[bors-servo]

☔ The latest upstream changes (presumably #11249) made this pull request unmergeable. Please resolve the merge conflicts.

[asajeffrey]

Rebased.

I had a think about what the return value should be from handle_next_event if the event queue returns None. In this case, we must be in shutdown already, so sending a Quit event will most likely confuse the other threads, since this is the second quit event. So I think returning false is the thing to do here.

[samlh]

Not that I have any authority, but this looks good to me :).

[highfive]

New code was committed to pull request.

[asajeffrey]

Rebased.

[aneeshusa]

@bors-servo r+

[bors-servo]

📌 Commit 37511cc has been approved by aneeshusa

[bors-servo]

⌛ Testing commit 37511cc with merge 5f34522...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

[metajack]

Hmm. I have a fix for this as well, but on the glutin side. See rust-windowing/glutin#777. The difference is that the change to glutin will prevent wait_events() from returning None.

What happens that once glutin gets a WM_DESTROY event (to close the window) it shuts down its event loops which causes recv() to panic, but the iterator discards the error and just returns None.

This patch is fine though as it doesn't coflict; it may cease to be necessary in the near future.

[asajeffrey]

We probably want to remove sources of panic from ports/glutin anway, since it's one of our single points of failure, a panic in ports/glutin brings down the whole browser. I'm quite happy about having a belt-and-braces approach here.

[metajack]

The issue here is that by returning immediately, you now have a very hot loop. This whole loop was written with the assumption that it would block here, and now that it's gone as soon as WM_DESTROY comes in, we probably spike to 100% CPU until shutdown.

[asajeffrey]

@metajack ah, the problem is that there's nothing handle_next_event can do to indicate that event processing should finish at this point? It can return a boolean, but that just controls whether a quit event is sent. We could park the thread, but that's pretty nasty.

[metajack]

Event processing isn't finished, though. We're waiting for shutdown to fully propagate. In my patch wait_events() blocks until the WAKEUP message arrives, and then we'll pull the FinishedShuttingDown message out of the other event queue. Maybe we can block on that event queue if we're in shutdown?

The main issue here is we have two event queues and Rust has nothing like select().

[asajeffrey]

Moving the conversation about hot-spinning at shutdown to #11307.