<AWT Dev> RFR: 8262731: [macOS] Exception from "Printable.print" is swallowed during "PrinterJob.print"

[Phil Race]

On Fri, 14 May 2021 18:37:46 GMT, Anton Litvinov <alitvinov at openjdk.org> wrote:

> Hello,
> 
> Could you please review the following fix for the bug specific to macOS. The bug consists in the fact that if the method "java.awt.print.Printable.print​(Graphics, PageFormat, int)" throws "java.awt.print.PrinterException" or "java.lang.RuntimeException" during the call "java.awt.print.PrinterJob.print()", then the exception is caught and ignored by JDK and a user cannot learn that printing failed and what caused failure of printing, because "PrinterJob.print()" method does not throw "PrinterException" or the occurred exception is not reported by JDK through the error stream.
> 
> ROOT CAUSE OF THE BUG:
> The root cause of the bug is the fact that in the method "sun.lwawt.macosx.CPrinterJob.printAndGetPageFormatArea(final Printable, final Graphics, final PageFormat, final int)" from the file "src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java" the exception thrown during execution of the expression
> 
> "int pageResult = printable.print(graphics, pageFormat, pageIndex);"
> 
> is caught but is not returned to a developer by any mean or is not printed out to the error stream.
> 
> THE FIX:
> The fix implements propagation of the occurred and caught exception to the level of the user's code executing "PrinterJob.print()" method. Propagation of the exception by storing it in the instance variable of "CPrinterJob" object is implemented, because the engaged code always is executed:
> - on 2 threads (non-EDT thread, EDT thread) in case when "PrinterJob.print()" is called by the user on a non-EDT thread;
> - on 3 threads (2 EDT threads, a temporary thread started by JDK to execute "CPrinterJob._safePrintLoop(long, long );") when "PrinterJob.print()" is called on EDT thread.
> 
> The regression test which is part of the fix was also successfully executed on MS Windows OS and Linux OS.
> 
> Thank you,
> Anton

I'm a bit surprised the test passes on windows.
Perhaps because you aren't trying RuntimeException ?

src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java line 264:

> 262:         Exception oldEx = lastPrintExRef.getAndSet(newEx);
> 263:         if (printOldEx && (oldEx != null)) {
> 264:             oldEx.printStackTrace();

Why not Throwable ? 
I suggest to not do this. ie don't print and don't replace Instead swallow the new exception 
and let the original problem that started it get propagated. That seems more likely to be useful.

src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java line 400:

> 398:                     throw (PrinterException) lastPrintEx;
> 399:                 } else if (lastPrintEx instanceof RuntimeException) {
> 400:                     throw (RuntimeException) lastPrintEx;

I don't see you testing this case 
And do I understand correctly that this only throws the exception at the end of the printloop after trying all pages ?

src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java line 823:

> 821:                         }
> 822:                     } catch (Exception e) {
> 823:                         // Original code bailed on any exception

Throwable ?
And what was wrong with bailing ? That comment has been there since the code was brought into OPenJDK so I can't tell what the motivation was.
But I'm getting a bit lost what is going on here. 

The spec https://docs.oracle.com/en/java/javase/11/docs/api/java.desktop/java/awt/print/Printable.html#print(java.awt.Graphics,java.awt.print.PageFormat,int)

says 
Throws:
PrinterException - thrown when the print job is terminated.

I take that to mean we should not carry on printing .. but it looks like we just note the exception and carry on.
I'm not sure this code (pre-existing behaviour) is right.

We do want to grab the exception so it doesn't mess up interverning code but don't we want this to reach the caller of PrinterJob.print - in the form of a PrinterExcepton ?

test/jdk/java/awt/print/PrinterJob/ExceptionFromPrintableIsIgnoredTest.java line 30:

> 28:             "Printable.print" throws "PrinterException".
> 29:    @run main/manual ExceptionFromPrintableIsIgnoredTest MAIN
> 30:    @run main/manual ExceptionFromPrintableIsIgnoredTest EDT

See comment below

test/jdk/java/awt/print/PrinterJob/ExceptionFromPrintableIsIgnoredTest.java line 83:

> 81:                     return NO_SUCH_PAGE;
> 82:                 }
> 83:                 throw new PrinterException("Exception from Printable.print");

Don't you want to also test this with some kind of RuntimeException ?

test/jdk/java/awt/print/PrinterJob/ExceptionFromPrintableIsIgnoredTest.java line 86:

> 84:             }
> 85:         });
> 86:         if (job.printDialog()) {

why do we need a dialog ? If you remove this it can be an automated test.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4036