Skip to content

Commit

Permalink
[Android] Reorder file closing sequence
Browse files Browse the repository at this point in the history
There is a race condition in test `testNormalPrintingFlow`,
which will cause failures in `FileInputStream`. Generally,
when `FileInputStream` created, the same file descriptor used
previously will be reused, but in the instrumentation thread,
after `onWriteFinished`, it is trying to close the same file
descriptor, so sometimes it will cause crashes.

This CL:

1) Removes unnecessary fileDescriptor.close() in
the override `onWriteFinished`.

2) Place `closeFileDescriptor(mFileDescriptor)` before calls
`onWrite*` callbacks to make the testing code wait the whole
procedure finished, this removes the race condition.

Bug: 732698
Change-Id: Ifdc1fa9f00cb77f64f46f6d09df5890b1bb12061
Reviewed-on: https://chromium-review.googlesource.com/549227
Reviewed-by: Selim Gurun <sgurun@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Nicolas Dossou-Gbété <dgn@chromium.org>
Commit-Queue: Shimi Zhang <ctzsm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483023}
  • Loading branch information
Shimi Zhang authored and Commit Bot committed Jun 28, 2017
1 parent 7272fa2 commit 825cfe8
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@

import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.util.concurrent.Callable;
import java.util.concurrent.FutureTask;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -290,13 +289,7 @@ private void callWriteOnUiThread(
new WriteResultCallbackWrapperMock() {
@Override
public void onWriteFinished(PageRange[] pages) {
try {
descriptor.close();
// Result is ready, signal to continue.
result.run();
} catch (IOException ex) {
Assert.fail("Failed file operation: " + ex.toString());
}
result.run();
}
}
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,15 +207,15 @@ public void startPrint(final Printable printable, PrintManagerDelegate printMana
public void pdfWritingDone(boolean success) {
if (mPrintingState == PRINTING_STATE_FINISHED) return;
mPrintingState = PRINTING_STATE_READY;
closeFileDescriptor(mFileDescriptor);
mFileDescriptor = -1;
if (success) {
PageRange[] pageRanges = convertIntegerArrayToPageRanges(mPages);
mOnWriteCallback.onWriteFinished(pageRanges);
} else {
mOnWriteCallback.onWriteFailed(mErrorMessage);
resetCallbacks();
}
closeFileDescriptor(mFileDescriptor);
mFileDescriptor = -1;
}

@Override
Expand Down Expand Up @@ -291,9 +291,7 @@ public void onFinish() {
// 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. Also, if we get an extra
// AskUserForSettings call, it's handled inside {@link
// PrintingContext#pageCountEstimationDone}.
// inside pdfWritingDone, thus not entering here.
mPrintingContext.askUserForSettingsReply(false);
}
mPrintingContext.updatePrintingContextMap(mFileDescriptor, true);
Expand Down

0 comments on commit 825cfe8

Please sign in to comment.