On Thu, 12 Aug 2021 19:34:44 GMT, Phil Race <p...@openjdk.org> 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
