Skip to content
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

8263490: [macos] Crash occurs on JPasswordField with activated InputMethod #2959

Closed
wants to merge 1 commit into from

Conversation

toshiona
Copy link
Contributor

@toshiona toshiona commented Mar 12, 2021

Hi,
Please review the fix for the issue of JPasswordField and activated InputMethod on macOS.
I don't think this condition is usual, but I'd like to avoid crash.

It needs two additional checks in "AWTView attributedSubstringForProposedRange:actualRange".

Tested test/jdk/java/awt on macOS Catalina and BigSur (both headful), and no regression was occurred.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263490: [macos] Crash occurs on JPasswordField with activated InputMethod

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2959/head:pull/2959
$ git checkout pull/2959

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 12, 2021

👋 Welcome back tnakamura! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 12, 2021

@toshiona The following label will be automatically applied to this pull request:

  • awt

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the awt label Mar 12, 2021
@toshiona toshiona marked this pull request as ready for review Mar 12, 2021
@openjdk openjdk bot added the rfr label Mar 12, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 12, 2021

Webrevs

@openjdk
Copy link

@openjdk openjdk bot commented Mar 12, 2021

@toshiona This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8263490: [macos] Crash occurs on JPasswordField with activated InputMethod

Reviewed-by: dmarkov, serb, kizune

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 26 new commits pushed to the master branch:

  • da9ead5: 8263399: CDS should archive only classes allowed by module system
  • 9c84899: 8263555: use driver-mode to run ClassFileInstaller
  • 8e562d2: 8263477: serviceability/sa/ClhsdbDumpheap.java timed out
  • a7aba2b: 8263549: 8263412 can cause jtreg testlibrary split
  • d339320: 8263136: C4530 was reported from VS 2019 at access bridge
  • a528771: 8262491: AArch64: CPU description should contain compatible board list
  • 86e4c75: 8256156: JFR: Allow 'jfr' tool to show metadata without a recording
  • 0b68ced: 8263548: runtime/cds/appcds/SharedRegionAlignmentTest.java fails to compile after JDK-8263412
  • 43524cc: 8243455: Many SA tests can fail due to trying to get the stack trace of an active method
  • e834f99: 8263412: ClassFileInstaller can't be used by classes outside of default package
  • ... and 16 more: https://git.openjdk.java.net/jdk/compare/ad1f60541940ae59da2af63015c5b5038832a676...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dmarkov20, @mrserb, @azuev-java) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Mar 12, 2021
@mrserb
Copy link
Member

@mrserb mrserb commented Mar 12, 2021

As far as I understand the JPasswordField is affected because it disables InputMethods by default? I wonder why in this case we even try to run something in the native code?

@toshiona
Copy link
Contributor Author

@toshiona toshiona commented Mar 13, 2021

I agree that disabling IM completely on JPasswordField is the fundamental fix, but the current macOS can still activate the prediction candidate feature even if the target component doesn't accept non ASCII characters. Then, the feature may call native codes.

I'd like to prevent crash this time. Also, the other IM related native codes in AWTView.m have similar checks, and only attributedSubstringForProposedRange lacks them.

mrserb
mrserb approved these changes Mar 13, 2021
@toshiona
Copy link
Contributor Author

@toshiona toshiona commented Mar 14, 2021

/integrate

@openjdk openjdk bot added the sponsor label Mar 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 14, 2021

@toshiona
Your change (at version 217703d) is now ready to be sponsored by a Committer.

@toshiona
Copy link
Contributor Author

@toshiona toshiona commented Mar 14, 2021

@dmarkov20 @mrserb @azuev-java
Thank you for the review. Could one of you sponsor this fix?

@dmarkov20
Copy link
Member

@dmarkov20 dmarkov20 commented Mar 15, 2021

/sponsor

@openjdk openjdk bot closed this Mar 15, 2021
@openjdk openjdk bot added integrated and removed sponsor ready rfr labels Mar 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 15, 2021

@dmarkov20 @toshiona Since your change was applied there have been 32 commits pushed to the master branch:

  • 8afec70: 8260931: Implement JEP 382: New macOS Rendering Pipeline
  • 0638303: 8263497: Clean up sun.security.krb5.PrincipalName::toByteArray
  • ba22e6f: 8263446: Avoid unary minus over unsigned type in ObjectSynchronizer::dec_in_use_list_ceiling
  • b371f90: 8263504: Some OutputMachOpcodes fields are uninitialized
  • f7e0a09: 8263425: AArch64: two potential bugs in C1 LIRGenerator::generate_address()
  • 554dd29: 8263564: Consolidate POSIX code for runtime exit support: os::shutdown, os::abort and os::die
  • da9ead5: 8263399: CDS should archive only classes allowed by module system
  • 9c84899: 8263555: use driver-mode to run ClassFileInstaller
  • 8e562d2: 8263477: serviceability/sa/ClhsdbDumpheap.java timed out
  • a7aba2b: 8263549: 8263412 can cause jtreg testlibrary split
  • ... and 22 more: https://git.openjdk.java.net/jdk/compare/ad1f60541940ae59da2af63015c5b5038832a676...master

Your commit was automatically rebased without conflicts.

Pushed as commit 32c7fcc.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awt integrated
4 participants