-
Notifications
You must be signed in to change notification settings - Fork 187
Edge:evaluate deadlock fix #1973
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
Edge:evaluate deadlock fix #1973
Conversation
Test Results 539 files - 6 539 suites - 6 29m 55s ⏱️ -8s For more details on these failures, see this check. Results for commit bda4169. ± Comparison against base commit 601136c. This pull request removes 37 tests.♻️ This comment has been updated with latest results. |
afa88d8 to
c6af804
Compare
c6af804 to
6cd6756
Compare
6cd6756 to
e67bde7
Compare
fedejeanne
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR does 2 things, right?
- It changes the strategy about how to execute callbacks by introducing the method
executeAsynchronously(...) - It executes some new blocks of code asynchronously by using the new method
Would it be possible to split it in 2 different PRs? That way it would be possible to see for example which part of these changes (1. or 2.) fixes the tests and which part makes it easier to work with the code.
bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java
Outdated
Show resolved
Hide resolved
bd7cd75 to
87864c5
Compare
fedejeanne
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made an initial review, just a few minor comments regarding coding practices. I will test it later today, after these comments are addressed.
In general, it looks good. Thank you @amartya4256 !
bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java
Outdated
Show resolved
Hide resolved
87864c5 to
ce62574
Compare
bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java
Outdated
Show resolved
Hide resolved
|
Test failure is unrelated #1843 |
ce62574 to
f9ee6fd
Compare
fedejeanne
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amartya4256 please restore all my changes from ce62574 and improve the text of the comment. No need for a new method.
bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT Browser/win32/org/eclipse/swt/browser/Edge.java
Outdated
Show resolved
Hide resolved
f9ee6fd to
bda4169
Compare
This commit cotributes to replicating the behaviour of Edge:evaluate as it is in WebKit - it must not wait for the execution of script to obtain the result, in case evaluate is called inside a WebView callback. This commit also makes sure that OpenWindowListeners are execute synchronously or asynchronously depending on if the handleNewWindowRequested is called from the evaluate script. Moreover, this enables all the tests which were failing because of Edge:evaluate limitations. contributes to eclipse-platform#1771 and eclipse-platform#1919
bda4169 to
3f39fdb
Compare
|
This PR produced a regression regarding: While the root cause of that described issue led to a timeout before this PR, it now leads to a complete UI block from which the application does not recover. Please have a look @amartya4256. This is the according stack trace where it deadlocks: |
This PR cotributes to replicating the behaviour of Edge:evaluate as
it is in WebKit - it must not wait for the execution of script to obtain
the result, in case evaluate is called inside a WebView callback. This
PR also makes sure that OpenWindowListeners are execute
synchronously or asynchronously depending on if the
handleNewWindowRequested is called from the evaluate script. Moreover,
this enables all the tests which were failing because of Edge:evaluate
limitations.
contributes to
#1771 and
#1919