Skip to content

Conversation

HeikoKlare
Copy link
Contributor

@HeikoKlare HeikoKlare commented May 5, 2023

The Edge browser creation uses Display.readAndDispath() to process the OS events for browser instantiation. When there are other asynchronous executions scheduled while the browser is being initialized, they may be processed in between the processing of browser-instantiating OS events if too much time elapses between them. In case such an asynchronous execution changes a state that makes the browser instantiation fail, such as disposing the composite parent of the browser, an exception occurs.

In order to avoid the processing of asynchronous executions during browser instantiation, the change ensures that readAndDisplay() is only called when an OS event is present to be processed.

A drawback of this approach is that in case the browser instantiation takes a long time, for whatever reasons, the UI will not be responsive as UI events are not processed. I do not think that this situation is likely to happen but want to mention it as a potential drawback of the solution.

Supposed to fix #339.

@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2023

Test Results

     299 files  ±0       299 suites  ±0   8m 15s ⏱️ + 2m 25s
  4 086 tests +1    4 053 ✔️  - 22    8 💤 ±0  25 +23 
12 176 runs  +3  12 081 ✔️  - 20  70 💤 ±0  25 +23 

For more details on these failures, see this check.

Results for commit ed8f821. ± Comparison against base commit 86616f0.

♻️ This comment has been updated with latest results.

@HeikoKlare HeikoKlare force-pushed the issue-339 branch 2 times, most recently from 3a7dce1 to 7975689 Compare May 5, 2023 15:46
@HeikoKlare HeikoKlare marked this pull request as ready for review May 5, 2023 15:46
@HeikoKlare HeikoKlare requested a review from niraj-modi as a code owner May 5, 2023 15:46
@HeikoKlare HeikoKlare linked an issue May 5, 2023 that may be closed by this pull request
@vogella
Copy link
Contributor

vogella commented May 10, 2023

Thanks @HeikoKlare this affected on of my clients in the past. I suggest to merge.

@Destrolaric
Copy link

We also had to patch the issue behavior. It will be good to finally make it work properly inside the SWT. I would be glad if it would be merged.

@HeikoKlare
Copy link
Contributor Author

@Destrolaric can you already say whether this change makes your patch obsolete and fixes the behavior properly for you?

From my side, we can merge this PR. I have validated proper instantiation of the Edge browser on my system in an isolated SWT application as well as in our RCP product. Should someone else validate before merging?

@lshanmug
Copy link
Member

@deepika-u Can you please verify the patch?

…platform#339

The Edge browser creation uses Display.readAndDispath() to process the
OS events for browser instantiation. When there are other asynchronous
executions scheduled while the browser is being initialized, they may be
processed in between the processing of browser-instantiating OS events
if too much time elapses between them. In case such an asynchronous
execution changes a state that makes the browser instantiation fail,
such as disposing the composite parent of the browser, an exception
occurs.

In order to avoid the processing of asynchronous executions during
browser instantiation, the change ensures that readAndDisplay() is only
called when an OS event is present to be processed.
@deepika-u
Copy link
Contributor

@HeikoKlare

I have tried to recreate the problem of "No more handles" with below Snippet.

package org.eclipse.swt.bugs;

import org.eclipse.swt.;
import org.eclipse.swt.browser.
;
import org.eclipse.swt.layout.;
import org.eclipse.swt.widgets.
;

public class NoMoreHandles {

public static void main(String[] args) {
	// TODO Auto-generated method stub
	Display display = new Display();
	Shell shell = new Shell(display);
	shell.setLayout(new GridLayout(1, false));

	createComposite(shell);

	shell.pack();
	shell.open();
	while (!shell.isDisposed()) {
		if (!display.readAndDispatch())
			display.sleep();
	}
	display.dispose();
}

public static void createComposite(Composite parent) {
	parent.setLayout(new GridLayout(1, false));

	for (int i = 0; i <= 10; i++) {
		System.out.println("Iteration " + i);
		Composite c = new Composite(parent, SWT.NONE);
		Display.getCurrent().asyncExec(new Runnable() {

			@Override
			public void run() {
				c.dispose();
			}
		});
		Browser browser = null;
		if (!c.isDisposed()) {
			browser = new Browser(c, SWT.EDGE);
		}
		if (browser != null) {
			browser.dispose();
		}
	}
}

}

When run seeing the below console output
Iteration 0
Exception in thread "main" org.eclipse.swt.SWTError: No more handles [0x80070578]
at org.eclipse.swt.SWT.error(SWT.java:4944)
at org.eclipse.swt.browser.Edge.error(Edge.java:190)
at org.eclipse.swt.browser.Edge.create(Edge.java:343)
at org.eclipse.swt.browser.Browser.(Browser.java:99)
at org.eclipse.swt.bugs.NoMoreHandles.createComposite(NoMoreHandles.java:42)
at org.eclipse.swt.bugs.NoMoreHandles.main(NoMoreHandles.java:16)

Then when i apply the patch, i am able to run it fine and not seeing any "No more handles".
So the patch looks fine for me. Please let me know if that verifies the fix and you want me to check anything else.

I have tried on below environment
Eclipse SDK
Version: 2023-06 (4.28)
Build id: I20230507-1800
OS: Windows 11, v.10.0, x86_64 / win32
Java vendor: Eclipse Adoptium
Java runtime version: 20+36
Java version: 20

@deepika-u
Copy link
Contributor

@HeikoKlare

Specifically tried for Edge browser with below snippet once again and edge browser window comes up properly after the patch is applied.

package org.eclipse.swt.bugs;

import org.eclipse.swt.;
import org.eclipse.swt.browser.
;
import org.eclipse.swt.layout.;
import org.eclipse.swt.widgets.
;

public class NoMoreHandles {

public static void main(String[] args) {
	// TODO Auto-generated method stub
	Display display = new Display();
	Shell shell = new Shell(display);
	shell.setLayout(new FillLayout());

	createComposite(shell);

	shell.pack();
	shell.open();
	while (!shell.isDisposed()) {
		if (!display.readAndDispatch())
			display.sleep();
	}
	display.dispose();
}

public static void createComposite(Composite parent) {
	//parent.setLayout(new GridLayout(1, false));

	for (int i = 0; i < 1; i++) {
		//for (int i = 0; i <= 10; i++) {
		System.out.println("Iteration " + i);
		Composite c = new Composite(parent, SWT.NONE);
		c.setLayout(new FillLayout());
		Display.getCurrent().asyncExec(new Runnable() {

			@Override
			public void run() {
				//c.dispose();
			}
		});
		Browser browser = null;
		if (!c.isDisposed()) {
			browser = new Browser(c, SWT.EDGE);
			browser.setSize(100, 100);
			browser.setUrl("whatismybrowser.com");
			//browser.setText("welcome");
		}
		if (browser != null) {
			//browser.dispose();
		}
	}
}

}

So patch looks good to me.

Copy link
Member

@niraj-modi niraj-modi left a comment

Choose a reason for hiding this comment

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

Approving, Thanks @deepika-u for verifying the patch.

@niraj-modi niraj-modi merged commit 5377bd2 into eclipse-platform:master May 12, 2023
@HeikoKlare
Copy link
Contributor Author

Great, thanks for verifying, @deepika-u!

@deepika-u
Copy link
Contributor

Verified this on below build and looks good to me.

Eclipse SDK
Version: 2023-06 (4.28)
Build id: I20230515-1800
OS: Windows 11, v.10.0, x86_64 / win32
Java vendor: Eclipse Adoptium
Java runtime version: 17.0.6+10
Java version: 17.0.6

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.

[Edge] No more handle exceptions from Edge browser
7 participants