[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-awt-dev
Subject:    Re: <AWT Dev> RFR: 8181571: printing to CUPS fails on mac sandbox app [v3]
From:       Alexander Scherbatiy <alexsch () openjdk ! java ! net>
Date:       2021-08-24 16:34:30
Message-ID: Noj7YW0DvwQOOW-6mjfJzKzVCAro-5AccwCLjQ9A4ZY=.5475f0c7-82cb-4015-9a97-b8f444c1657e () github ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]