[Printing] Remove extra |askUserForSettingsReply()| call

After we migrated native side callback to |OnceCallback|, it exposed
that we sometimes call |askUserForSettingsReply()| more than once. The
|DCHECK(callback_)| in |PrintingContextAndroid::AskUserForSettingsReply|
was violated.

Since |Printingcontext.askUserForSettings()| will call
|askUserForSettingsReply()| for sure, we don't need to call it in
other places.

This CL removes a call site of |askUserForSettingsReply| in
|PrintingControllerImpl|.

Bug: 863297
Change-Id: If1c34a4a89acaca461074231ab1bef8edef9baae
Reviewed-on: https://chromium-review.googlesource.com/1161604
Commit-Queue: Shimi Zhang <ctzsm@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582301}

chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java

@@ -45,6 +45,8 @@ import org.chromium.printing.PrintingControllerImpl;
 
 import java.io.File;
 import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 
@@ -105,6 +107,53 @@ public class PrintingControllerTest {
         }
     }
 
+    private static class TemporaryFileHandler implements AutoCloseable {
+        private File mTempFile;
+        private ParcelFileDescriptor mFileDescriptor;
+
+        public TemporaryFileHandler() throws IOException {
+            mTempFile = File.createTempFile(TEMP_FILE_NAME, TEMP_FILE_EXTENSION);
+            try {
+                mFileDescriptor =
+                        ParcelFileDescriptor.open(mTempFile, ParcelFileDescriptor.MODE_READ_WRITE);
+            } catch (FileNotFoundException e) {
+                // Exception happened, can't continue, cleanup the file.
+                TestFileUtil.deleteFile(mTempFile.getAbsolutePath());
+                throw new FileNotFoundException();
+            }
+        }
+
+        ParcelFileDescriptor getFileDescriptor() {
+            return mFileDescriptor;
+        }
+
+        @Override
+        public void close() throws IOException {
+            try {
+                mFileDescriptor.close();
+            } finally {
+                TestFileUtil.deleteFile(mTempFile.getAbsolutePath());
+            }
+        }
+    }
+
+    private static class PrintingControllerImplPdfWritingDone extends PrintingControllerImpl {
+        private WaitForOnWriteHelper mWaitForOnWrite;
+
+        public PrintingControllerImplPdfWritingDone(
+                PrintDocumentAdapterWrapper printDocumentAdapterWrapper, String errorText,
+                WaitForOnWriteHelper waitForOnWrite) {
+            super(printDocumentAdapterWrapper, errorText);
+            mWaitForOnWrite = waitForOnWrite;
+            sInstance = this;
+        }
+
+        @Override
+        public void pdfWritingDone(int pageCount) {
+            mWaitForOnWrite.notifyCalled();
+        }
+    }
+
     /**
      * Test a basic printing flow by emulating the corresponding system calls to the printing
      * controller: onStart, onLayout, onWrite, onFinish.  Each one is called once, and in this
@@ -268,6 +317,67 @@ public class PrintingControllerTest {
         }
     }
 
+    /**
+     * Test for http://crbug.com/863297
+     * This bug shows Android printing framework could call |PrintDocumentAdapter.onFinish()|
+     * before one of |WriteResultCallback.onWrite{Cancelled, Failed, Finished}()| get called.
+     * Crash test, pass if there is no crash.
+     */
+    @Test
+    @TargetApi(Build.VERSION_CODES.KITKAT)
+    @MediumTest
+    @Feature({"Printing"})
+    public void testCancelPrintBeforeWriteResultCallbacks() throws Throwable {
+        if (!ApiCompatibilityUtils.isPrintingSupported()) return;
+
+        mActivityTestRule.startMainActivityWithURL(URL);
+
+        final WaitForOnWriteHelper onWriteHelper = new WaitForOnWriteHelper();
+        final Tab currentTab = mActivityTestRule.getActivity().getActivityTab();
+        final PrintingControllerImpl printingController =
+                ThreadUtils.runOnUiThreadBlockingNoException(() -> {
+                    return new PrintingControllerImplPdfWritingDone(
+                            new PrintDocumentAdapterWrapper(), PRINT_JOB_NAME, onWriteHelper);
+                });
+
+        startControllerOnUiThread(printingController, currentTab);
+        callStartOnUiThread(printingController);
+
+        final WriteResultCallbackWrapper writeResultCallback =
+                new WriteResultCallbackWrapperMock() {
+                    @Override
+                    public void onWriteFinished(PageRange[] pages) {
+                        Assert.fail("onWriteFinished shouldn't be called");
+                    }
+
+                    @Override
+                    public void onWriteFailed(CharSequence error) {
+                        Assert.fail("onWriteFailed shouldn't be called");
+                    }
+
+                    @Override
+                    public void onWriteCancelled() {
+                        Assert.fail("onWriteCancelled shouldn't be called");
+                    }
+                };
+
+        try (TemporaryFileHandler handler = new TemporaryFileHandler()) {
+            final LayoutResultCallbackWrapper layoutResultCallback =
+                    new LayoutResultCallbackWrapperMock() {
+                        @Override
+                        public void onLayoutFinished(PrintDocumentInfo info, boolean changed) {
+                            printingController.onWrite(new PageRange[] {PageRange.ALL_PAGES},
+                                    handler.getFileDescriptor(), new CancellationSignal(),
+                                    writeResultCallback);
+                        }
+                    };
+            callLayoutOnUiThread(
+                    printingController, null, createDummyPrintAttributes(), layoutResultCallback);
+            onWriteHelper.waitForCallback("pdfWritingDone never called");
+            callFinishOnUiThread(printingController);
+        }
+    }
+
     private PrintingControllerImpl createControllerOnUiThread() {
         return ThreadUtils.runOnUiThreadBlockingNoException(() -> {
             return (PrintingControllerImpl) PrintingControllerImpl.create(

printing/android/java/src/org/chromium/printing/PrintingContext.java

@@ -57,16 +57,6 @@ public class PrintingContext implements PrintingContextInterface {
         }
     }
 
-    /**
-     * Notifies the native side that the user just chose a new set of printing settings.
-     * @param success True if the user has chosen printing settings necessary for the
-     *                generation of PDF, false if there has been a problem.
-     */
-    @Override
-    public void askUserForSettingsReply(boolean success) {
-        nativeAskUserForSettingsReply(mNativeObject, success);
-    }
-
     @CalledByNative
     public static PrintingContext create(long nativeObjectPointer) {
         ThreadUtils.assertOnUiThread();
@@ -135,8 +125,7 @@ public class PrintingContext implements PrintingContextInterface {
         // If the printing dialog has already finished, tell Chromium that operation is cancelled.
         if (mController.hasPrintingFinished()) {
             // NOTE: We don't call nativeAskUserForSettingsReply (hence Chromium callback in
-            // AskUserForSettings callback) twice.  See {@link PrintingControllerImpl#onFinish}
-            // for more explanation.
+            // AskUserForSettings callback) twice.
             askUserForSettingsReply(false);
         } else {
             mController.setPrintingContext(this);
@@ -145,14 +134,18 @@ public class PrintingContext implements PrintingContextInterface {
         }
     }
 
+    private void askUserForSettingsReply(boolean success) {
+        assert mNativeObject != 0;
+        nativeAskUserForSettingsReply(mNativeObject, success);
+    }
+
     @Override
     public void showSystemDialogDone() {
         nativeShowSystemDialogDone(mNativeObject);
     }
 
     private native void nativeAskUserForSettingsReply(
-            long nativePrintingContextAndroid,
-            boolean success);
+            long nativePrintingContextAndroid, boolean success);
 
     private native void nativeShowSystemDialogDone(long nativePrintingContextAndroid);
 }

printing/android/java/src/org/chromium/printing/PrintingContextInterface.java

@@ -16,12 +16,6 @@ public interface PrintingContextInterface {
      */
     void updatePrintingContextMap(int fileDescriptor, boolean delete);
 
-    /**
-     * Notifies the native side if the printing settings are successfully prepared.
-     * @param success True if the settings are successfully prepared to be used by the native side.
-     */
-    void askUserForSettingsReply(boolean success);
-
     /**
      * Notifies the native side that the printing process is completed. This method should be
      * called when the process was initiated by the native side (window.print())

printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java

@@ -15,6 +15,7 @@ import android.print.PrintDocumentInfo;
 
 import org.chromium.base.Log;
 import org.chromium.base.ThreadUtils;
+import org.chromium.base.VisibleForTesting;
 import org.chromium.printing.PrintDocumentAdapterWrapper.PdfGenerator;
 
 import java.io.IOException;
@@ -43,7 +44,8 @@ public class PrintingControllerImpl implements PrintingController, PdfGenerator
     private static final int PRINTING_STATE_FINISHED = 2;
 
     /** The singleton instance for this class. */
-    private static PrintingController sInstance;
+    @VisibleForTesting
+    protected static PrintingController sInstance;
 
     private final String mErrorMessage;
 
@@ -92,7 +94,8 @@ public class PrintingControllerImpl implements PrintingController, PdfGenerator
 
     private PrintManagerDelegate mPrintManager;
 
-    private PrintingControllerImpl(
+    @VisibleForTesting
+    protected PrintingControllerImpl(
             PrintDocumentAdapterWrapper printDocumentAdapterWrapper, String errorText) {
         mErrorMessage = errorText;
         mPrintDocumentAdapterWrapper = printDocumentAdapterWrapper;
@@ -306,15 +309,6 @@ public class PrintingControllerImpl implements PrintingController, PdfGenerator
     public void onFinish() {
         mPages = null;
         if (mPrintingContext != null) {
-            if (mPrintingState != PRINTING_STATE_READY) {
-                // Note that we are never making an extraneous askUserForSettingsReply call.
-                // If we are in the middle of a PDF generation from onLayout or onWrite, it means
-                // the state isn't PRINTING_STATE_READY, so we enter here and make this call (no
-                // extra). If we complete the PDF generation successfully from onLayout or onWrite,
-                // we already make the state PRINTING_STATE_READY and call askUserForSettingsReply
-                // inside pdfWritingDone, thus not entering here.
-                mPrintingContext.askUserForSettingsReply(false);
-            }
             mPrintingContext.updatePrintingContextMap(mFileDescriptor, true);
             mPrintingContext = null;
         }