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.

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

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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 37 additions & 9 deletions src/java.desktop/macosx/classes/sun/lwawt/macosx/CPrinterJob.java
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<Throwable> printErrorRef = new AtomicReference<>();

private boolean noDefaultPrinter = false;

Expand Down Expand Up @@ -322,6 +324,7 @@ public void print(PrintRequestAttributeSet attributes) throws PrinterException {
performingPrinting = true;
userCancelled = false;
}
printErrorRef.set(null);

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

Throwable printError = printErrorRef.getAndSet(null);
if (printError != null) {
if (printError instanceof PrinterException) {
throw (PrinterException) printError;
} else if (printError instanceof RuntimeException) {
throw (RuntimeException) printError;
}
throw (PrinterException)
new PrinterException().initCause(printError);
}
}

// Normalize the collated, # copies, numPages, first/last pages. Need to
Expand Down Expand Up @@ -785,22 +799,36 @@ 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 (Throwable t) {
printErrorRef.compareAndSet(null, t);
}
}
} 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 (Throwable t) {
printErrorRef.compareAndSet(null, t);
}
} else {
r.run();
}

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

// upcall from native
Expand Down
@@ -0,0 +1,129 @@
/*
* 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 expected exception,
if "Printable.print" throws an exception.
@run main/manual ExceptionFromPrintableIsIgnoredTest MAIN PE
@run main/manual ExceptionFromPrintableIsIgnoredTest MAIN RE
@run main/manual ExceptionFromPrintableIsIgnoredTest EDT PE
@run main/manual ExceptionFromPrintableIsIgnoredTest EDT RE
Copy link
Contributor

Choose a reason for hiding this comment

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

wjy is this still manual ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The test is manual, because the test initiates printing and there is a chance that on a test host a default printer will be some virtual printer which can show the native dialog asking to specify the location of PDF file in which the printed document should be saved, in this case the test will be blocked and will be killed by "jtreg" by a timeout, what is unacceptable for the automatic test which should not slow down the speed of execution of all other automatic tests. For example such a virtual printer on Windows OS can be "Microsoft Print to PDF". By the way, I already explained this reason in one of my replies to your questions in the previous iteration of the code review.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought I also explained that the printer keyword means it would only be run if the system has a real printer by virtue of it being set in the jtreg run. You could also do that on a virtual printer if you didn't mind entering the file name

*/

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}
private enum TestExceptionType {PE, RE}

public static void main(String[] args) {
if (args.length < 2) {
throw new RuntimeException("Two arguments are expected:"
+ " test thread type and test exception type.");
}

new ExceptionFromPrintableIsIgnoredTest(
TestThreadType.valueOf(args[0]),
TestExceptionType.valueOf(args[1]));
}

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

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

private void runTest(final TestExceptionType exceptionType) {
PrinterJob job = PrinterJob.getPrinterJob();
if (job.getPrintService() == null) {
throw new RuntimeException("No printers are available.");
Copy link
Contributor

Choose a reason for hiding this comment

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

where does this exception go if it happens and you are on EDT ?

Copy link
Member Author

Choose a reason for hiding this comment

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

If this exception happens on EDT, this exception as any other exception thrown from the method "runTest" will be perfectly caught and handled in "catch" block of the constructor of the test "ExceptionFromPrintableIsIgnoredTest" as "InvocationTargetException" instance, which will have this "RuntimeException" as its cause. The whole test relies on this approach of throwing and catching the exception and it works.

}

job.setPrintable(new Printable() {
@Override
public int print(Graphics graphics, PageFormat pageFormat,
int pageIndex) throws PrinterException {
if (pageIndex > 1) {
return NO_SUCH_PAGE;
}
if (exceptionType == TestExceptionType.PE) {
throw new PrinterException(
"Exception from 'Printable.print'.");
} else if (exceptionType == TestExceptionType.RE) {
throw new RuntimeException(
"Exception from 'Printable.print'.");
}
return PAGE_EXISTS;
}
});

Throwable printEx = null;
try {
job.print();
} catch (Throwable t) {
printEx = t;
}

if (printEx != null) {
System.out.println("'PrinterJob.print' threw the exception:");
printEx.printStackTrace(System.out);

if (((printEx instanceof PrinterException) &&
(exceptionType == TestExceptionType.PE)) ||
((printEx instanceof RuntimeException) &&
(exceptionType == TestExceptionType.RE))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I am fairly sure that I tested this case on Windows and nothing caught it so does this test work on Windows ?

  2. I raised the question / suggestion that should we not wrap these in a PrinterException ?
    It at least needs to be thought through.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. The test does not fail on Windows in all 4 tested scenarios. You can run this regression test on Windows and you will see in the test report in the test output that the test successfully throws the required "PrinterException" or "RuntimeException" from "java.awt.print.Printable.print​(Graphics, PageFormat, int)" and that the test successfully catches this "PrinterException" or "RuntimeException" and prints out it to "System.out" "PrintStream" in the test method "ExceptionFromPrintableIsIgnoredTest.runTest(final TestExceptionType)".

Line 115 printEx.printStackTrace(System.out);

This line in the test is done exactly as the evidence that the exception was really propagated from the method "PrinterJob.print()" and caught by the test. I verified that it is "true" on macOS, Windows, Linux.

I reported this in the code review and in JBS bug record itself that in JDK 17 and in JDK 8u291 and even in JDK 8-b132 on Windows and on Linux "PrinterJob.print()", which is called on EDT, propagates "PrinterException" and "RuntimeException" which are thrown from "Printable.print​(Graphics, PageFormat, int)" method. If you still do not trust this, then I suggest you just to recheck it yourself by any trustworthy mean in addition to this regression test and the purely manual test case "ExceptionFromPrintableIsIgnored.java" attached to the bug record.

  1. By current moment in JDK versions on Windows and Linux the "RuntimeException" thrown from "Printable.print​(Graphics, PageFormat, int)" has been propagated through "PrinterJob.print()" as is without wrapping it into "PrinterException", as my experiments show even in JDK 8-b132 and in currently latest GA release of JDK 8 and in JDK 17.

From my viewpoint not wrapping "RuntimeException" into "PrinterException" and propagating "RuntimeException" through "PrinterJob.print()" will just bring behavior of JDK on macOS into correspondence with existing behavior of JDK on Windows and Linux. If we wrap "RuntimeException" into "PrinterException" on macOS, then at some point in time somebody will need to change JDK code for Windows and Linux to also do the same wrapping of the exception which is currently not done. I do not insist on any variant.

return;// Test passed.
}
throw new RuntimeException("Unexpected exception was thrown.");
} 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(
"'PrinterJob.print' did not throw any exception.");
}
}
}