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

8234474: [macos 10.15] Crash in file dialog in sandbox mode #70

Closed

Conversation

@kevinrushforth
Copy link
Member

kevinrushforth commented Dec 19, 2019

This PR is a fix for JDK-8234474, a crash in the code that shows a file open or save dialog.

In order to provide additional support for Copy (CMD-C), Cut (CMD-X), and Paste (CMD-V), the Glass implementation for displaying a file open or save dialog subclasses NSSavePanel or NSOpenPanel to add this support. When the application is running in sandboxed mode, the dialogs are shown out-of-process by the "powerbox". In this mode, attempting to use our subclass results in a security exception. Previously, we added code to detect whether we were running in a sandbox as a fix for JDK-8092977; we now use NSSavePanel or NSOpenPanel directly when in sandboxed mode.

Starting with macOS 10.15 (Catalina) Apple always displays file dialogs out-of-process via powerbox, so our use of a subclass is ineffective. Further, we have reports of some cases where we crash even though our sandbox detection code doesn't indicate that we are running in a sandbox.

Since there is no point in trying to use our subclasses on macOS 10.15 or later, I propose to fix this bug by changing the logic so that we use NSSavePanel or NSOpenPanel directly in either of the following conditions:

  1. the app is running in sandbox mode
    OR
  2. The platform is macOS 10.15 or later

Progress

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

Issue

JDK-8234474: [macos 10.15] Crash in file dialog in sandbox mode

Approvers

  • Ambarish Rapte (arapte - Reviewer)
  • Phil Race (prr - Reviewer)
@bridgekeeper
Copy link

bridgekeeper bot commented Dec 19, 2019

👋 Welcome back kcr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@kevinrushforth kevinrushforth self-assigned this Dec 19, 2019
@kevinrushforth kevinrushforth force-pushed the kevinrushforth:8234474-macos-dialogs branch from 3528312 to dca394e Dec 19, 2019
@kevinrushforth kevinrushforth marked this pull request as ready for review Dec 19, 2019
@kevinrushforth kevinrushforth changed the title WIP: 8234474: [macos 10.15] Crash in file open or save dialog in sandbox mode on macOS Catalina 8234474: [macos 10.15] Crash in file dialog in sandbox mode Dec 19, 2019
@openjdk openjdk bot added the rfr label Dec 19, 2019
@mlbridge
Copy link

mlbridge bot commented Dec 19, 2019

Webrevs

@kevinrushforth
Copy link
Member Author

kevinrushforth commented Dec 19, 2019

This will need two reviewers.

@arapte I'd like you to be one of the reviewers.

@arapte
arapte approved these changes Jan 6, 2020
Copy link

arapte left a comment

Looks good to me, Verified that all tests pass on Mojave 10.14.6

@openjdk openjdk bot removed the rfr label Jan 6, 2020
@openjdk
Copy link

openjdk bot commented Jan 6, 2020

@kevinrushforth This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8234474: [macos 10.15] Crash in file dialog in sandbox mode

Reviewed-by: arapte, prr
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 9 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to do this manually, please merge master into your branch first.

➡️ To integrate this PR with the above commit message, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jan 6, 2020
@prrace
Copy link
Contributor

prrace commented Jan 6, 2020

Based on my reading of https://bugs.openjdk.java.net/browse/JDK-8092977, this means the support for edit operations in these dialogs is on its way out ... now supportable only in non-sandboxed apps on the older macOS releases. Is there a serious practical consequence of this, or was the editing just a convenience ? There must have been some resason we went to the trouble. However based on this being "on its way out" why try to prolong it ? Just drop the subclasses now.

@kevinrushforth
Copy link
Member Author

kevinrushforth commented Jan 6, 2020

This was just added as a convenience, so it isn't critical functionality.

Having said that, while we could just drop the subclasses now, it would cause a perceived regression in behavior for users running on macOS 10.14 Mojave. So my preference would be to not remove the subclass at this time, but to just not use them when running on 10.15 or later.

@prrace
Copy link
Contributor

prrace commented Jan 6, 2020

Well then you'll just have to come back to it. If its not critical I'd remove it now so that FX 15 (or is this for 14 ?) behaves consistently

@kevinrushforth
Copy link
Member Author

kevinrushforth commented Jan 6, 2020

This is targeted for JavaFX 14, which is another reason to go with a more targeted (safer) fix, since we are almost at RDP1. For JavaFX 15 it does seem best to just remove the functionality entirely.

@prrace
Copy link
Contributor

prrace commented Jan 6, 2020

Ok, fair enough for 14 ...

@kevinrushforth
Copy link
Member Author

kevinrushforth commented Jan 6, 2020

I'll file a follow-up bug for 15 shortly.

@prrace would you be willing to be the second reviewer on this fix?

@kevinrushforth
Copy link
Member Author

kevinrushforth commented Jan 6, 2020

I filed JDK-8236685 to remove the custom dialogs in JavaFX 15.

@prrace
prrace approved these changes Jan 6, 2020
@kevinrushforth
Copy link
Member Author

kevinrushforth commented Jan 6, 2020

/integrate

@openjdk openjdk bot closed this Jan 6, 2020
@openjdk openjdk bot added the integrated label Jan 6, 2020
@openjdk
Copy link

openjdk bot commented Jan 6, 2020

@kevinrushforth The following commits have been pushed to master since your change was applied:

  • f1108b0: 8233747: JVM crash in com.sun.webkit.dom.DocumentImpl.createAttribute
  • 3c4d68d: 8236626: Update copyright header for files modified in 2019
  • 580a2a9: 8087980: Add property to disable Monocle cursor
  • 3bbcbfb: 8225571: Port DND source to use GTK instead of GDK
  • 1952606: 8232811: Dialog's preferred size no longer accommodates multi-line strings
  • 4c6ebfb: 8236484: Compile error in monocle dispman
  • 8367e1a: 8130738: Add tabSize property to Text and TextFlow
  • 69e4ef3: 8235627: Blank stages when running JavaFX app in a macOS virtual machine
  • 5e0fb91: 8235364: Update copyright header for files modified in 2019

Your commit was automatically rebased without conflicts.

Pushed as commit 587f195.

@openjdk openjdk bot removed the ready label Jan 6, 2020
@mlbridge
Copy link

mlbridge bot commented Jan 6, 2020

Mailing list message from Kevin Rushforth on openjfx-dev:

Changeset: 587f195
Author: Kevin Rushforth <kcr at openjdk.org>
Date: 2020-01-06 21:11:20 +0000
URL: https://git.openjdk.java.net/jfx/commit/587f195c

8234474: [macos 10.15] Crash in file dialog in sandbox mode

Reviewed-by: arapte, prr

! modules/javafx.graphics/src/main/native-glass/mac/GlassDialogs.m

@kevinrushforth kevinrushforth deleted the kevinrushforth:8234474-macos-dialogs branch Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.