Skip to content

[Win32] Make Edge only process relevant OS messages #1789 #1797

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

Merged
merged 1 commit into from
Feb 5, 2025
Merged

[Win32] Make Edge only process relevant OS messages #1789 #1797

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ private static void processOSMessagesUntil(Supplier<Boolean> condition, Consumer
// The timer call also wakes up the display to avoid being stuck in display.sleep()
display.timerExec((int) MAXIMUM_OPERATION_TIME.toMillis(), () -> timeoutOccurred.set(true));
while (!display.isDisposed() && !condition.get() && !timeoutOccurred.get()) {
if (OS.PeekMessage(msg, 0, 0, 0, OS.PM_NOREMOVE)) {
if (OS.PeekMessage(msg, 0, 0, 0, OS.PM_NOREMOVE | OS.PM_QS_POSTMESSAGE)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering if we can make it more specific to the edge instance by providing a hwnd? But another concern is, does the filtering mean that the possibility of this falling into display.sleep() is higher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be possible, but we need to be aware that this will not mean that this message is processed by display.readAndDispatch() then, as there messages are processed by priority. Posted messages have high priority and will be processed before, e.g., input events, so if there is a posted message, it will always be processed before an eventual input event also being in the queue.
This change is only avoiding that we run into display.readAndDispatch() without a message being in the queue as that has already been processed via this PeekMessage call.

The possibility of falling into display.sleep() is of course higher, but that's also by intent. We limit processing to specific messages, so instead of processing other messages we will sleep and wait for the next message instead. But since we will wake from display.sleep() whenever a new message arrives, this is not an issue. Or do you have any further concerns here?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it keeps waking up with new messages, it should be fine. The only concern was about, in an imaginary scenario, no messages are received for a long time, it means the display thread will be sleeping for that period - which will freeze the UI, right? But of course we need to test it out practically. For me locally it worked just fine so I would be in the favour of merging this and waiting for the feedback from the users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, if we don't receive messages (which also means that Edge initialization fails), we run into our timeout. That's what we have the display.timerExec() for, setting the timeout value and breaking the loop execution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, then its safe from the infiite sleeping problem here.

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way I always wondered why there is no Display.sleep(timeout) is it not supported on the OS level? How does a "real" native app handle this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. If you are simply spinning the queue, waiting for messages and processing them, such a timeout should actually not be necessary, as without any messages arriving from the SO there is something completely broken anyway. In the case we have here, if we simply spinned the whole event loop instead of limiting to processing specific messages, we would not run into that situation either (see #1773).

display.readAndDispatch();
} else {
display.sleep();
Expand Down
Loading