Re: RFR: 8271603: Unnecessary Vector usage in java.desktop [v2]
On Tue, 24 Aug 2021 21:13:57 GMT, Andrey Turbanov wrote: >> Usage of thread-safe collection `Vector` is unnecessary. It's recommended to >> use `ArrayList` if a thread-safe implementation is not needed. In >> post-BiasedLocking times, this is gets worse, as every access is >> synchronized. >> I checked only places where `Vector` was used as local variable. > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8271603: Unnecessary Vector usage in java.desktop > revert back to Enumeration > bring back default values Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4680
Re: RFR: 8272481: [macos] javax/swing/JFrame/NSTexturedJFrame/NSTexturedJFrame.java fails [v3]
On Tue, 24 Aug 2021 15:09:51 GMT, Alexey Ushakov wrote:
>> Update opacity only if the component is visible
>
> Alexey Ushakov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8272481: [macos] javax/swing/JFrame/NSTexturedJFrame/NSTexturedJFrame.java
> fails
>
> Moved peer.setTextured() to the place where windowLayer is initialized
src/java.desktop/macosx/classes/sun/lwawt/macosx/CPlatformWindow.java line 339:
> 337: if (peer != null) { // Not applicable to CWarningWindow
> 338: peer.setTextured(IS(TEXTURED, styleBits));
> 339: }
This probably works fine, but maybe we can move it after "setPtr(ref.get());"
below? or that ptr is not used in this new call chain?
-
PR: https://git.openjdk.java.net/jdk/pull/5172
Re: RFR: 8271603: Unnecessary Vector usage in java.desktop [v2]
On Tue, 24 Aug 2021 21:13:57 GMT, Andrey Turbanov wrote: >> Usage of thread-safe collection `Vector` is unnecessary. It's recommended to >> use `ArrayList` if a thread-safe implementation is not needed. In >> post-BiasedLocking times, this is gets worse, as every access is >> synchronized. >> I checked only places where `Vector` was used as local variable. > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8271603: Unnecessary Vector usage in java.desktop > revert back to Enumeration > bring back default values src/java.desktop/share/classes/javax/sound/sampled/AudioSystem.java line 244: > 242: } > 243: > 244: return list.toArray(new Line.Info[0]); I thought we already covered such changed under JDK-8269130, is there are any other missed cases? If yes then probably it will be good to extract such changes. - PR: https://git.openjdk.java.net/jdk/pull/4680
Re: RFR: 8271603: Unnecessary Vector usage in java.desktop [v2]
On Mon, 2 Aug 2021 05:19:32 GMT, Sergey Bylokhov wrote:
>> Andrey Turbanov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8271603: Unnecessary Vector usage in java.desktop
>> revert back to Enumeration
>> bring back default values
>
> src/java.desktop/share/classes/java/awt/MenuBar.java line 348:
>
>> 346: Iterator e = getMenu(i).shortcuts();
>> 347: while (e.hasNext()) {
>> 348: shortcuts.addElement(e.next());
>
> I think it is fine to replace the Enumeration with the Iterator in most of
> the places, but here we will get a kind of mix of both, since we cannot
> remove the usage of Enumeration in the return time.
reverted back to Vector here
> src/java.desktop/share/classes/javax/print/MimeType.java line 576:
>
>> 574: ArrayList thePieces = new ArrayList<>();
>> 575: boolean mediaTypeIsText;
>> 576: boolean parameterNameIsCharset;
>
> Default values might be removed as a separate cleanup.
reverted
-
PR: https://git.openjdk.java.net/jdk/pull/4680
Re: RFR: 8271603: Unnecessary Vector usage in java.desktop [v2]
> Usage of thread-safe collection `Vector` is unnecessary. It's recommended to > use `ArrayList` if a thread-safe implementation is not needed. In > post-BiasedLocking times, this is gets worse, as every access is synchronized. > I checked only places where `Vector` was used as local variable. Andrey Turbanov has updated the pull request incrementally with one additional commit since the last revision: 8271603: Unnecessary Vector usage in java.desktop revert back to Enumeration bring back default values - Changes: - all: https://git.openjdk.java.net/jdk/pull/4680/files - new: https://git.openjdk.java.net/jdk/pull/4680/files/12e39834..7a2393eb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4680=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4680=00-01 Stats: 15 lines in 4 files changed: 1 ins; 2 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/4680.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4680/head:pull/4680 PR: https://git.openjdk.java.net/jdk/pull/4680
Re: RFR: 8271603: Unnecessary Vector usage in java.desktop [v2]
On Mon, 9 Aug 2021 23:46:09 GMT, Phil Race wrote: >> Even non-public method can be called via reflection, so I'd be cautios about >> changing return type > > Apps should not be doing that and the desktop module along with most of the > rest of the JDK is strongly encapsulated and illegal access will be denied. > But given the relationship with the sole (?) caller I think this is not worth > the churn. Not exactly an MT hotspot. I'd revert the changes here and in > MenuBar. reverted - PR: https://git.openjdk.java.net/jdk/pull/4680
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules
On Tue, 24 Aug 2021 11:48:46 GMT, Alexander Zvegintsev wrote: > Is there any reason to not touch them along with this fix? Update them too. - PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules [v2]
> Collections.sort is just a wrapper, so it is better to use an instance method > directly. Andrey Turbanov has updated the pull request incrementally with one additional commit since the last revision: 8272863: Replace usages of Collections.sort with List.sort call in public java modules replace Collections.sort without comparator too - Changes: - all: https://git.openjdk.java.net/jdk/pull/5229/files - new: https://git.openjdk.java.net/jdk/pull/5229/files/e31936a5..d6dfc8bf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5229=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5229=00-01 Stats: 21 lines in 10 files changed: 0 ins; 4 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/5229.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5229/head:pull/5229 PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: 8181571: printing to CUPS fails on mac sandbox app [v3]
On Thu, 12 Aug 2021 19:34:44 GMT, Phil Race wrote:
>> Alexander Scherbatiy has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Return null if printers are not found in sandboxed app on MacOS
>
> src/java.desktop/unix/classes/sun/print/CUPSPrinter.java line 96:
>
>> 94: libFound = initIDs();
>> 95: if (libFound) {
>> 96:cupsServer = getCupsServer();
>
> I think we should wrap all the new lines in isMac()
> Also can you explain the reasons for the logic that implies we may have a
> server starting with "/"
> in which case your always change the cupServer to localhost even if it is not
> sandboxed ?
>
> I ask because it seems to contradict what you pasted
> "by the cupsServer() function and not changing the interface string to
> "localhost""
The logic which replaces a server starting with "/" to localhost is the
original logic which is implemented in native level in the getCupsServer()
function:
https://github.com/openjdk/jdk/blob/f608e81ad8309a001b8a92563a93b8adee1ce2b8/src/java.desktop/unix/native/common/awt/CUPSfuncs.c#L176
The fix only moves this logic to the java level to store domainSocketPathname
in case of sandboxed app on MacOS.
> src/java.desktop/unix/classes/sun/print/CUPSPrinter.java line 399:
>
>> 397: return printerURIs;
>> 398: }
>> 399: }
>
> So if getCupsDefaultPrinters() doesn't find anything you always continue to
> the original code even though
> it would seem that you know we are in a sandboxed app and it won't find
> anything ?
I updated the code to return null in case there are no printer names from
j2d_cupsGetDests() function in a sandboxed on MacOS.
> src/java.desktop/unix/classes/sun/print/CUPSPrinter.java line 489:
>
>> 487: return domainSocketPathname;
>> 488: }
>> 489:
>
> You will need to suppress deprecation warnings here.
Should I add `@SuppressWarnings("deprecation")` to the
getDomainSocketPathname() method?
I see that CUPSPrinter class has `@SuppressWarnings("removal")` but its private
method do not have any SuppressWarnings annotations.
> src/java.desktop/unix/classes/sun/print/CUPSPrinter.java line 506:
>
>> 504: IPPPrintService.debug_println(debugPrefix+"libFound "+libFound);
>> 505: if (libFound) {
>> 506: String server = getDomainSocketPathname() != null ?
>> getDomainSocketPathname() : getServer();
>
> Split this long line
Updated.
> src/java.desktop/unix/native/common/awt/CUPSfuncs.c line 244:
>
>> 242: DPRINTF("CUPSfuncs::bad alloc new array\n", "")
>> 243: (*env)->ExceptionClear(env);
>> 244: JNU_ThrowOutOfMemoryError(env, "OutOfMemoryError");
>
> I find this weird. What is the ExceptionClear for ? The only possible
> exception here is for
> an OOME which might be thrown by NewObjectArray. So you clear that and then
> re-create it ?
> And who do will catch this ? What's the point ? Maybe just clear and return
> null.
The array creation error handling is implemented in the similar way as it is
done in the getMedia() function.
The ExceptionClear() has been added to the getMedia() by `8031001: [Parfait]
warnings from b121 for jdk/src/solaris/native/sun/awt: JNI-related warnings`
I would prefer to have unified error handling in these methods. If
ExceptionClear is not suitable in this place it would be better to recheck
JDK-8031001 and update all places accordingly in a separate fix.
> src/java.desktop/unix/native/common/awt/CUPSfuncs.c line 253:
>
>> 251: j2d_cupsFreeDests(num_dests, dests);
>> 252: DPRINTF("CUPSfuncs::bad alloc new string ->name\n", "")
>> 253: JNU_ThrowOutOfMemoryError(env, "OutOfMemoryError");
>
> similar comments to above plus I am fairly sure you want to delete nameArray
> since it isn't returned.
> For that matter if the NewString fails on the 4th printer you want to free
> the 3 previous ones too before returning.
The code is updated to remove previously created strings and clear the
nameArray in case of an error during new string creation.
-
PR: https://git.openjdk.java.net/jdk/pull/4861
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules
On Mon, 23 Aug 2021 21:01:48 GMT, Andrey Turbanov wrote: > Collections.sort is just a wrapper, so it is better to use an instance method > directly. java.time changes look good. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: 8181571: printing to CUPS fails on mac sandbox app [v3]
> The issue is reproduced on macOS Big Sur 11.0.1 with jdk 16.0.1+9.
>
> Create a native macOS app from the Hello.java file, sign and run it in
> sandbox:
>
> import javax.print.*;
> import javax.swing.*;
>
> public class Hello {
>
> public static void main(String[] args) throws Exception {
> SwingUtilities.invokeAndWait(() -> {
> boolean isSandboxed = System.getenv("APP_SANDBOX_CONTAINER_ID")
> != null;
> PrintService defaultPrinter =
> PrintServiceLookup.lookupDefaultPrintService();
> PrintService[] services =
> PrintServiceLookup.lookupPrintServices(null, null);
>
> StringBuilder builder = new StringBuilder();
> builder.append("is sandboxed: ").append(isSandboxed).append("\n");
> builder.append("default printer:
> ").append(defaultPrinter).append("\n");
> int size = services.length;
> for (int i = 0; i < size; i++) {
>
> builder.append("printer[").append(i).append("]=").append(services[i]).append("\n");
> }
> JOptionPane.showMessageDialog(null, builder.toString());
> });
> }
> }
>
> The signed app in sandbox shows null default printer and
> PrintServiceLookup.lookupPrintServices(null, null) returns "Unix Printer: lp".
> 
>
> The problem has been discussed on 2d-dev mail list:
> https://mail.openjdk.java.net/pipermail/2d-dev/2017-June/008375.html
> https://mail.openjdk.java.net/pipermail/2d-dev/2017-July/008418.html
>
> According to the discussion:
>
>> I've submitted a DTS incident to Apple and a friend there has followed-up.
>> Their unofficial position is that java should be connecting to the cups
>> interface returned
>> by the cupsServer() function and not changing the interface string to
>> "localhost".
>> Security changes in 10.12.4 reject the TCP connection which they say confuses
>> network-client access with print access. They don't seem interested in
>> loosening that change.
>
>
> The proposed solution is to use the domain socket pathname in
> httpConnect(...) cups function and cupsGetDests(...) to get list of printers
> from cups when the app is signed and is run in sandbox on MacOs.
Alexander Scherbatiy has updated the pull request incrementally with one
additional commit since the last revision:
Return null if printers are not found in sandboxed app on MacOS
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4861/files
- new: https://git.openjdk.java.net/jdk/pull/4861/files/104e792b..067d0025
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4861=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4861=01-02
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/4861.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4861/head:pull/4861
PR: https://git.openjdk.java.net/jdk/pull/4861
Re: RFR: 8181571: printing to CUPS fails on mac sandbox app [v2]
> The issue is reproduced on macOS Big Sur 11.0.1 with jdk 16.0.1+9.
>
> Create a native macOS app from the Hello.java file, sign and run it in
> sandbox:
>
> import javax.print.*;
> import javax.swing.*;
>
> public class Hello {
>
> public static void main(String[] args) throws Exception {
> SwingUtilities.invokeAndWait(() -> {
> boolean isSandboxed = System.getenv("APP_SANDBOX_CONTAINER_ID")
> != null;
> PrintService defaultPrinter =
> PrintServiceLookup.lookupDefaultPrintService();
> PrintService[] services =
> PrintServiceLookup.lookupPrintServices(null, null);
>
> StringBuilder builder = new StringBuilder();
> builder.append("is sandboxed: ").append(isSandboxed).append("\n");
> builder.append("default printer:
> ").append(defaultPrinter).append("\n");
> int size = services.length;
> for (int i = 0; i < size; i++) {
>
> builder.append("printer[").append(i).append("]=").append(services[i]).append("\n");
> }
> JOptionPane.showMessageDialog(null, builder.toString());
> });
> }
> }
>
> The signed app in sandbox shows null default printer and
> PrintServiceLookup.lookupPrintServices(null, null) returns "Unix Printer: lp".
> 
>
> The problem has been discussed on 2d-dev mail list:
> https://mail.openjdk.java.net/pipermail/2d-dev/2017-June/008375.html
> https://mail.openjdk.java.net/pipermail/2d-dev/2017-July/008418.html
>
> According to the discussion:
>
>> I've submitted a DTS incident to Apple and a friend there has followed-up.
>> Their unofficial position is that java should be connecting to the cups
>> interface returned
>> by the cupsServer() function and not changing the interface string to
>> "localhost".
>> Security changes in 10.12.4 reject the TCP connection which they say confuses
>> network-client access with print access. They don't seem interested in
>> loosening that change.
>
>
> The proposed solution is to use the domain socket pathname in
> httpConnect(...) cups function and cupsGetDests(...) to get list of printers
> from cups when the app is signed and is run in sandbox on MacOs.
Alexander Scherbatiy has updated the pull request incrementally with two
additional commits since the last revision:
- Clean utf_str and nameArray references
- Split long line in CUPSPrinter.isCupsRunning() method
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4861/files
- new: https://git.openjdk.java.net/jdk/pull/4861/files/072cc498..104e792b
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4861=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4861=00-01
Stats: 11 lines in 2 files changed: 9 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/4861.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4861/head:pull/4861
PR: https://git.openjdk.java.net/jdk/pull/4861
Integrated: 8272806: [macOS] "Apple AWT Internal Exception" when input method is changed
On Sun, 22 Aug 2021 19:46:49 GMT, Phil Race wrote: > There's a long eval in the bug report : > https://bugs.openjdk.java.net/browse/JDK-8272806 but here's the summary > > When focus is lost by the app a message is sent down to native setting a > native reference to the input method to null > which is a sort of signal not to notify upwards anything to the input method > (which internally will NPE if there's > no focused component as is the case after focus is lost). > But we aren't actually setting it to null because of a check for null which > previously checked the wrapper for > the JNI ref was not null but is now obsolete. So just remove the check for > null. > > The steps to reproduce this bug are very manual and involve installing > additional input methods and toggling them > at the system level. So I didn't see a way to write a useful regression test > but if you follow the bug report steps you > should be able to verify the fix. > > I've run all automated tests just for good measure and they all pass. This pull request has now been integrated. Changeset: f681d654 Author:Phil Race URL: https://git.openjdk.java.net/jdk/commit/f681d6544ac2506cb72e45c1f9ed8dfbbde099f2 Stats: 4 lines in 2 files changed: 1 ins; 2 del; 1 mod 8272806: [macOS] "Apple AWT Internal Exception" when input method is changed Reviewed-by: serb, dmarkov, azvegint - PR: https://git.openjdk.java.net/jdk/pull/5211
Re: RFR: 8272481: [macos] javax/swing/JFrame/NSTexturedJFrame/NSTexturedJFrame.java fails [v3]
> Update opacity only if the component is visible Alexey Ushakov has updated the pull request incrementally with one additional commit since the last revision: 8272481: [macos] javax/swing/JFrame/NSTexturedJFrame/NSTexturedJFrame.java fails Moved peer.setTextured() to the place where windowLayer is initialized - Changes: - all: https://git.openjdk.java.net/jdk/pull/5172/files - new: https://git.openjdk.java.net/jdk/pull/5172/files/9948d356..3f59951b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5172=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5172=01-02 Stats: 11 lines in 3 files changed: 2 ins; 7 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5172.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5172/head:pull/5172 PR: https://git.openjdk.java.net/jdk/pull/5172
Re: RFR: 8272806: [macOS] "Apple AWT Internal Exception" when input method is changed
On Sun, 22 Aug 2021 19:46:49 GMT, Phil Race wrote: > There's a long eval in the bug report : > https://bugs.openjdk.java.net/browse/JDK-8272806 but here's the summary > > When focus is lost by the app a message is sent down to native setting a > native reference to the input method to null > which is a sort of signal not to notify upwards anything to the input method > (which internally will NPE if there's > no focused component as is the case after focus is lost). > But we aren't actually setting it to null because of a check for null which > previously checked the wrapper for > the JNI ref was not null but is now obsolete. So just remove the check for > null. > > The steps to reproduce this bug are very manual and involve installing > additional input methods and toggling them > at the system level. So I didn't see a way to write a useful regression test > but if you follow the bug report steps you > should be able to verify the fix. > > I've run all automated tests just for good measure and they all pass. Marked as reviewed by azvegint (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5211
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules
On Mon, 23 Aug 2021 21:01:48 GMT, Andrey Turbanov wrote: > Collections.sort is just a wrapper, so it is better to use an instance method > directly. There are a bunch of calls to `Collections.sort()` without a comparator specified (at least in java.desktop): https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/java2d/Spans.java#L144 https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/awt/DebugSettings.java#L278 https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/swing/text/TextComponentPrintable.java#L787 https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/DefaultRowSorter.java#L1070 https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/DefaultRowSorter.java#L1204 https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/GroupLayout.java#L2137 It is also a wrapper to `list.sort(null)`. Is there any reason to not touch them along with this fix? - PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules
On Mon, 23 Aug 2021 21:01:48 GMT, Andrey Turbanov wrote: > Collections.sort is just a wrapper, so it is better to use an instance method > directly. java/net and sun/net changes LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5229
Re: RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules
On Mon, 23 Aug 2021 21:01:48 GMT, Andrey Turbanov wrote: > Collections.sort is just a wrapper, so it is better to use an instance method > directly. The changes in the src/java.desktop/ looks fine. Filed: https://bugs.openjdk.java.net/browse/JDK-8272863 - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5229
RFR: 8272863: Replace usages of Collections.sort with List.sort call in public java modules
Collections.sort is just a wrapper, so it is better to use an instance method directly. - Commit messages: - [PATCH] Replace usages of Collections.sort with List.sort call in public java modules Changes: https://git.openjdk.java.net/jdk/pull/5229/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5229=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272863 Stats: 27 lines in 10 files changed: 0 ins; 8 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/5229.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5229/head:pull/5229 PR: https://git.openjdk.java.net/jdk/pull/5229
