[Win32] Fix faulty HRESULT_FROM_WIN32 comparison in Edge #1784
Conversation
|
Issue became obivous in this build, which shows that the comparison should have yielded "true" but did not: https://github.com/eclipse-platform/eclipse.platform.swt/actions/runs/13056049227/job/36427241188?pr=1706 |
| " Edge instance with same data folder but different environment options already exists"); | ||
| } | ||
| switch ((int) result) { | ||
| switch (result) { |
There was a problem hiding this comment.
if it is a long, maybe just use a switch here and instead if/else chain would be more future proof?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| }; | ||
| return newCallback((result, pv) -> { | ||
| return newCallback((resultAsLong, pv) -> { | ||
| int result = (int) resultAsLong; |
There was a problem hiding this comment.
| 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).
There was a problem hiding this comment.
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.
HeikoKlare
force-pushed
the
edge_hresult_int
branch
from
945941e to
f6d540f
Compare
Edge compares the result of WebView initialization with an HRESULT created from a Windows error code via the HRESULT_FROM_WIN32 macro. The actual result in that initialization callback is a long while the HRESULT returned from the macro is represented as integer in SWT. The current comparison of long and integer does not take into account that the values are signed. In this specific case, the actual HRESULT to compare against is 2147947423 while the HRESULT_FROM_WIN32 macro returns the signed value -2147019873. The actual result needs to be converted into a signed integer as well, as otherwise it will be considered as 2147947423, thus never matching the expected HRESULT value. This change converts the processed result into an integer to correct the comparison.
HeikoKlare
force-pushed
the
edge_hresult_int
branch
from
f6d540f to
e677954
Compare
|
Summarizing: In order to keep the conversion of long result values to int result values consistent with other places in the SWT Win32 implementation and thus avoid confusion due to inconsistent processing of the same kind of data, I keep the implementation as is. and focus on the bugfix here. I leave it up for future, holistic refactoring to clean up all places doing this kind of conversion. The validation failures are unrelated and known. |
2 participants
Edge compares the result of WebView initialization with an HRESULT created from a Windows error code via the HRESULT_FROM_WIN32 macro. The actual result in that initialization callback is a long while the HRESULT returned from the macro is represented as integer in SWT. The current comparison of long and integer does not take into account that the values are signed. In this specific case, the actual HRESULT to compare against is 2147947423 while the HRESULT_FROM_WIN32 macro returns the signed value -2147019873. The actual result needs to be converted into a signed integer as well, as otherwise it will be considered as 2147947423, thus never matching the expected HRESULT value.
This change converts the processed result into an integer to correct the comparison.