Skip to content

[Win32] Fix faulty HRESULT_FROM_WIN32 comparison in Edge #1784

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
Jan 31, 2025
Merged

[Win32] Fix faulty HRESULT_FROM_WIN32 comparison in Edge #1784

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
9 changes: 5 additions & 4 deletions 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 @@ -601,7 +601,8 @@ private IUnknown createControllerInitializationCallback(int previousAttempts) {
webViewProvider.abortInitialization();
initializationRollback.run();
};
return newCallback((result, pv) -> {
return newCallback((resultAsLong, pv) -> {
int result = (int) resultAsLong;
Copy link
Contributor

@laeubi laeubi Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
int result = (int) resultAsLong;
int result = (int) resultAsLong & 0xFFFFFF;

I think this would be the correct way to truncate an unsigned long to an int (in java).

Copy link
Contributor Author

@HeikoKlare HeikoKlare Jan 31, 2025

Choose a reason for hiding this comment

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

resultAsLong is a value passed via JNI that was converted from native unsigned long to jint. I have no idea how to best truncate that to Java int, but as said above: this all should be revised independently. The change in this PR just sticks to existing conversion behavior (for which we know it worked expect for this line that did a wrong comparison).
So would not be in favor of changing conversion here with no real gain and in the worst case the risk of a regression.

if (browser.isDisposed()) {
initializationAbortion.run();
return COM.S_OK;
Expand All @@ -611,14 +612,14 @@ private IUnknown createControllerInitializationCallback(int previousAttempts) {
SWT.error(SWT.ERROR_INVALID_ARGUMENT, null,
" Edge instance with same data folder but different environment options already exists");
}
switch ((int) result) {
switch (result) {
Copy link
Contributor

@laeubi laeubi Jan 31, 2025

Choose a reason for hiding this comment

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

if it is a long, maybe just use a switch here and instead if/else chain would be more future proof?

Copy link
Contributor Author

@HeikoKlare HeikoKlare Jan 31, 2025

Choose a reason for hiding this comment

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

The issue is that we use integers to represent HRESULT values in SWT all over the place. This also includes the "improper" signing of these values, as in the Windows API they have always been unsigned values (which Java is not able to represent). That's why we also have constants (like E_ABORT used in this switch) represented as signed integer values.

For that reason, I would propose to stick to integer in all the result code processing for now. As a separate topic, I would be in favor of completely replacing all tretments of HRESULT as integer in the OS APIs and constants with long values.

Copy link
Contributor

@laeubi laeubi Jan 31, 2025

Choose a reason for hiding this comment

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

This also includes the "improper" signing of these values, as in the Windows API they have always been unsigned values (which Java is not able to represent).

I don't think this is particular true. Java can represent an "unsigned" long but one has to be careful when comparing them or perform math, beside from that you can do whatever you want with them (including equals comparison).

Also I'm not sure that "because we do it everywhere" is a good argument here ... maybe this is a good reason to start with something new, e.g. make a HRESULT(long) record and use that whenever possible with a HRESULT#match(...) that makes sure one do no "illegal" operations on this here (like using it as a long).

Copy link
Contributor Author

@HeikoKlare HeikoKlare Jan 31, 2025

Choose a reason for hiding this comment

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

As said: I agree that there is some need for general improvement and I will create an issue for that. But starting with some locally introduced improvement for HRESULT processing that is incompatible with the OS APIs (that currently treat HRESULT as int) does not make sense to me. The current assumption is that HRESULT is an int, so we need to treat it like that. If we want to replace that (of which I would be in favor), we need to do that everywhere.

case COM.S_OK:
new IUnknown(pv).AddRef();
setupBrowser((int) result, pv);
setupBrowser(result, pv);
break;
case COM.E_WRONG_THREAD:
initializationAbortion.run();
error(SWT.ERROR_THREAD_INVALID_ACCESS, (int) result);
error(SWT.ERROR_THREAD_INVALID_ACCESS, result);
break;
case COM.E_ABORT:
initializationAbortion.run();
Expand Down
Loading