Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
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
8256465: [macos] Java frame and dialog presented full screen freeze application #3407
8256465: [macos] Java frame and dialog presented full screen freeze application #3407
Changes from 3 commits
20c1720
9882120
d85a002
8193b3e
4c34592
dd910f7
a09aea5
65396e8
f2a9198
7fbb729
28752a2
d710f76
c078f8d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@kevinrushforth said that since we set MIN_SDK (not sure of the exact variable name) to 10.12, that this is compiled down to a no-op .. which means it is useless and doesn't protect you from making the call on 10.11
So you might as well remove it. It won't prevent the crash that will happen on 10.11.
@mrserb also pointed out people might then copy this pattern not realising it does not work, and there's a better way ... apparently ...
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.
Right. @johanvos discovered this fun fact about
@available
when he got a crash report from a user. He filed JDK-8266743, which describes this problem.The setting of minimum version of macOS is controlled by the
-mmacosx-version-min
compile and link flag. The minimum version is defined in make/autoconf/flags.m4 and used in make/autoconf/flags-cflags.m4.One thing I don't know (and can't try, since I don't have access to a macOS system that old) is whether the JDK will fail somewhere else anyway (e.g., if they check for a minimum OS at start up). So this might be a moot point, but as it stands, I think @mrserb is right that we should avoid this pattern. I would probably just remove it, but you could decide to use something like
respondsToSelector
(which is what I think Sergey was suggesting).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.
Since OpenJFX does not have its own launcher and IIRC JDK only recently (JDK 17 b08 https://bugs.openjdk.java.net/browse/JDK-8260518 ) set the minimum to 10.12 it is possible that the submitter of the FX crash was using A JDK prior to that, in which case I am sure the Java Launcher would start up fine and you'd crash only when calling this code. So also I think very aguably library code has another reason to avoid this pattern.
And verifying what happens on 10.11 might be best done with a launcher from JDK 17 b07 or later .. also @kevinrushforth you might want to add some of these thoughts to the FX bug.
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.
Minor: add spaces after the commas.