Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8262731: [macOS] Exception from "Printable.print" is swallowed during "PrinterJob.print" #4036

Closed
wants to merge 4 commits into from
Closed

8262731: [macOS] Exception from "Printable.print" is swallowed during "PrinterJob.print" #4036

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
db2ce06
8262731: [macOS] Exception from "Printable.print" is swallowed during…
alitvinv May 14, 2021
4705151
Second version of the fix
alitvinv May 21, 2021
746dc0c
Merge remote-tracking branch 'origin/master' into JDK-8262731
alitvinv Jun 9, 2021
056734b
Third version of the fix
alitvinv Jun 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 48 additions & 9 deletions src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import java.net.URI;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.concurrent.atomic.AtomicReference;

import javax.print.*;
import javax.print.attribute.PrintRequestAttributeSet;
Expand Down Expand Up @@ -60,6 +61,7 @@ public final class CPrinterJob extends RasterPrinterJob {
private static String sShouldNotReachHere = "Should not reach here.";

private volatile SecondaryLoop printingLoop;
private AtomicReference<Exception> lastPrintExRef = new AtomicReference<>();

private boolean noDefaultPrinter = false;

Expand Down Expand Up @@ -256,6 +258,14 @@ private void completePrintLoop() {
}
}

private Exception setLastPrintEx(Exception newEx, boolean printOldEx) {
Exception oldEx = lastPrintExRef.getAndSet(newEx);
if (printOldEx && (oldEx != null)) {
oldEx.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this remark. I agree using "Throwable" is more solid approach. In the 2nd version of the fix I use "Throwable" in this "AtomicReference" object and I removed completely this method "setLastPrintEx".

}
return oldEx;
}

boolean isPrintToFile = false;
private void setPrintToFile(boolean printToFile) {
isPrintToFile = printToFile;
Expand Down Expand Up @@ -322,6 +332,7 @@ public void print(PrintRequestAttributeSet attributes) throws PrinterException {
performingPrinting = true;
userCancelled = false;
}
setLastPrintEx(null, false);

//Add support for PageRange
PageRanges pr = (attributes == null) ? null
Expand Down Expand Up @@ -380,6 +391,19 @@ public SecondaryLoop run() {
if (printingLoop != null) {
printingLoop.exit();
}

Exception lastPrintEx = setLastPrintEx(null, false);
if (lastPrintEx != null) {
if (lastPrintEx instanceof PrinterException) {
throw (PrinterException) lastPrintEx;
} else if (lastPrintEx instanceof RuntimeException) {
throw (RuntimeException) lastPrintEx;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the 2nd version of the fix I added testing of the scenario with "RuntimeException".

Yes, this code is executed only after the execution of the "printLoop" is fully finished for each of all page ranges. If printing of one page fails within one page range, then printing of other pages does not occur. "CPrinterJob.print(PrintRequestAttributeSet)" is blocking until printing is finished or fails, and my code is executed in the very end of the method.

} else {
PrinterException pe = new PrinterException();
pe.initCause(lastPrintEx);
throw pe;
}
}
}

// Normalize the collated, # copies, numPages, first/last pages. Need to
Expand Down Expand Up @@ -785,22 +809,37 @@ private Object[] getPageformatPrintablePeekgraphics(final int pageIndex) {
private Rectangle2D printAndGetPageFormatArea(final Printable printable, final Graphics graphics, final PageFormat pageFormat, final int pageIndex) {
final Rectangle2D[] ret = new Rectangle2D[1];

Runnable r = new Runnable() { public void run() { synchronized(ret) {
try {
int pageResult = printable.print(graphics, pageFormat, pageIndex);
if (pageResult != Printable.NO_SUCH_PAGE) {
ret[0] = getPageFormatArea(pageFormat);
Runnable r = new Runnable() {
@Override
public void run() {
synchronized (ret) {
try {
int pageResult = printable.print(
graphics, pageFormat, pageIndex);
if (pageResult != Printable.NO_SUCH_PAGE) {
ret[0] = getPageFormatArea(pageFormat);
}
} catch (Exception e) {
// Original code bailed on any exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the 2nd version of the fix I substituted "Exception" for "Throwable" in this catch block and in second "catch" block several lines below. Also in the 2nd fix version I removed this comment about bailing.

My opinion is the same, the code before the fix did not work according to the specification and was ignoring any caught exceptions. Perhaps, the reason of this is involvement of more than 1 thread in the code flow standing behind "PrinterJob.print" method execution and execution of the JDK's code through JNI in this code flow. Thus propagating the exception is impossible across threads and it is not easy to stop printing done by macOS at the moment, when the exception occurs in Java code in JDK. This pattern is used in the file "CPrinterJob.java" in other places, even there is the same comment in the method "CPrinterJob.getPageformatPrintablePeekgraphics". At the same time propagating "RuntimeException" from "PrinterJob.print" on Windows, Linux and now on macOS after this fix also is not according to the specification but still consistency in JDK behavior on these OS versions should be good.

As I understand, the method "CPrinterJob.printAndGetPageFormatArea" returns "null" as the "Rectangle2D", when the exception is caught, and this "null" value should automatically lead to returning of "NSZeroRect" from the Objective-C method "(NSRect)rectForPage:(NSInteger)pageNumber" in the file "src/java.desktop/macosx/native/libawt_lwawt/awt/PrinterView.m", from which "CPrinterJob.printAndGetPageFormatArea" Java method was invoked. And this "NSZeroRect" return value is indication of the error for macOS, which further leads to stopping of the printing process.

setLastPrintEx(e, true);
}
}
} catch (Exception e) {} // Original code bailed on any exception
}}};
}
};

if (onEventThread) {
try { EventQueue.invokeAndWait(r); } catch (Exception e) { e.printStackTrace(); }
try {
EventQueue.invokeAndWait(r);
} catch (Exception e) {
setLastPrintEx(e, true);
}
} else {
r.run();
}

synchronized(ret) { return ret[0]; }
synchronized (ret) {
return ret[0];
}
}

// upcall from native
Expand Down
105 changes: 105 additions & 0 deletions test/jdk/java/awt/print/PrinterJob/ExceptionFromPrintableIsIgnoredTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/* @test
@bug 8262731
@key headful printer
@summary Verify that "PrinterJob.print" throws the exception, if
"Printable.print" throws "PrinterException".
@run main/manual ExceptionFromPrintableIsIgnoredTest MAIN
@run main/manual ExceptionFromPrintableIsIgnoredTest EDT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment below

*/

import java.awt.Graphics;
import java.awt.print.PageFormat;
import java.awt.print.Printable;
import java.awt.print.PrinterException;
import java.awt.print.PrinterJob;
import java.lang.reflect.InvocationTargetException;
import javax.swing.SwingUtilities;

public class ExceptionFromPrintableIsIgnoredTest {
private enum TestThreadType {MAIN, EDT}

public static void main(String[] args) {
if (args.length < 1) {
throw new RuntimeException("Test thread type is not specified.");
}

TestThreadType threadType = TestThreadType.valueOf(args[0]);
new ExceptionFromPrintableIsIgnoredTest(threadType);
}

public ExceptionFromPrintableIsIgnoredTest(TestThreadType threadType) {
System.out.println(String.format(
"Test started. threadType='%s'", threadType));

if (threadType == TestThreadType.MAIN) {
runTest();
} else if (threadType == TestThreadType.EDT) {
try {
SwingUtilities.invokeAndWait(new Runnable() {
@Override
public void run() {
runTest();
}
});
} catch (InterruptedException | InvocationTargetException e) {
throw new RuntimeException(e);
}
}
System.out.println("Test passed.");
}

private void runTest() {
PrinterJob job = PrinterJob.getPrinterJob();
job.setPrintable(new Printable() {
@Override
public int print(Graphics graphics, PageFormat pageFormat,
int pageIndex) throws PrinterException {
if (pageIndex > 1) {
return NO_SUCH_PAGE;
}
throw new PrinterException("Exception from Printable.print");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the 1st version of the fix I did not test the scenario with "RuntimeException", because despite of the fact that I had known that JDK 17 will pass on Windows and Linux the scenario with "RuntimeException" I still was not sure if the behavior of older JDK release families is the same with JDK 17 on Windows and Linux, and just did not want to run the risk of failure of this test during potential porting of the fix to older JDK release families.

But yesterday I did testing with my standalone test case attached to the bug record in JBS and now I can certainly say that JDK 8u291-b10, JDK JDK 8-b132 do not fail in the test scenarios with "RuntimeException" on Windows and Linux. Therefore it is safe to include test scenario with "RuntimeException" to the regression test.

In the 2nd version of the fix now I am testing also "RuntimeException" both on EDT thread and on main thread.

}
});
if (job.printDialog()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Phil. Thank you very much for review of this fix. First I am answering your question, which is about why the test passes on Windows and which is your first and separate comment not bound to the source code. The test passes on Windows, because in JDK 17 for both Windows and Linux JDK works similarly stably and reliably, JDK 17 on Window and Linux successfully propagates "PrinterException" and "RuntimeException", if it is thrown from "Printable.print" method. I had verified this using my standalone manual test case "ExceptionFromPrintableIsIgnored.java" attached to the bug record in JBS, before sending the 1st version of the fix for review. I did not explore, why it is so, because it works on Windows and Linux and this bug is related to macOS, but I think that it works on Windows and Linux, because in JDK implementations for Windows and Linux printing is consistently done by JDK on the same thread on which "PrinterJob.print" was called and no portions of the involved code are executed asynchronously on other thread, like EDT in case of macOS.

I agree there is no need in the dialog in the regression test. In the 1st version of the fix I did it deliberately, to make it more manual, because the test initiates printing and there is no guarantee what type of printer would be set up on the test host, if some virtual printer is used on the host, that printer can show its own native dialog asking to specify location of a file, in which the printed document should be saved. That is why I think that this test cannot be fully automatic. In the 2nd version of the fix which I am submitting right now I removed this unnecessary code showing JDK print dialog.

Exception printEx = null;
try {
job.print();
} catch (PrinterException pe) {
printEx = pe;
}

if (printEx != null) {
System.out.println("'PrinterJob.print' threw the exception:");
printEx.printStackTrace(System.out);
} else {
throw new RuntimeException(
"'PrinterJob.print' did not throw any exception.");
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running on the EDT do you really want to do this ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I do really want to do this, when running on EDT, the whole test relies on this approach. When the test is run on EDT this is done via "SwingUtilities.invokeAndWait" method in the constructor of the test "ExceptionFromPrintableIsIgnoredTest" and in that constructor there is "try/catch" block. If some exception occurs on EDT and is not caught on EDT, then it will be wrapped by JDK in "InvocationTargetException" instance which will be thrown from "SwingUtilities.invokeAndWait" method and will be caught by that "try/catch" block in the constructor of the test. I checked that this code works before sending the code for review and today additionally.

throw new RuntimeException("User canceled the print dialog.");
}
}
}