On Fri, 14 May 2021 18:37:46 GMT, Anton Litvinov <alitvi...@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
