From a69d7afa15c08f361189aee67d7783db2f43a10f Mon Sep 17 00:00:00 2001 From: "jvoung@chromium.org" Date: Wed, 6 Feb 2013 07:12:34 +0000 Subject: [PATCH] Remember to delete the not-yet-committed-to-cache file if it's only partial. Also fix up a few printf formatting warnings that are detected when you attempt to turn PLUGIN_PRINTF() off. BUG=none Review URL: https://chromiumcodereview.appspot.com/12218012 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@180920 0039d316-1c4b-4281-b951-d872f2087c98 --- ppapi/native_client/src/trusted/plugin/plugin.cc | 2 +- .../src/trusted/plugin/pnacl_coordinator.cc | 59 ++++++++++++++-------- .../src/trusted/plugin/pnacl_coordinator.h | 5 ++ .../src/trusted/plugin/pnacl_translate_thread.cc | 6 +-- 4 files changed, 47 insertions(+), 25 deletions(-) diff --git a/ppapi/native_client/src/trusted/plugin/plugin.cc b/ppapi/native_client/src/trusted/plugin/plugin.cc index 31f233e4e2bc..e90f7e967791 100644 --- a/ppapi/native_client/src/trusted/plugin/plugin.cc +++ b/ppapi/native_client/src/trusted/plugin/plugin.cc @@ -908,7 +908,7 @@ void Plugin::CopyCrashLogToJsConsole() { size_t ix_start = 0; size_t ix_end; - PLUGIN_PRINTF(("Plugin::CopyCrashLogToJsConsole: got %d bytes\n", + PLUGIN_PRINTF(("Plugin::CopyCrashLogToJsConsole: got %"NACL_PRIuS" bytes\n", fatal_msg.size())); while (nacl::string::npos != (ix_end = fatal_msg.find('\n', ix_start))) { LogLineToConsole(this, fatal_msg.substr(ix_start, ix_end - ix_start)); diff --git a/ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc b/ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc index f5a9127efec4..1fb30a3a00db 100644 --- a/ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc +++ b/ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc @@ -284,7 +284,7 @@ PnaclCoordinator* PnaclCoordinator::BitcodeToNative( coordinator->off_the_record_ = plugin->nacl_interface()->IsOffTheRecord(); PLUGIN_PRINTF(("PnaclCoordinator::BitcodeToNative (manifest=%p, " - "off_the_record=%b)\n", + "off_the_record=%d)\n", reinterpret_cast(coordinator->manifest_.get()), coordinator->off_the_record_)); @@ -597,23 +597,12 @@ void PnaclCoordinator::DidCopyNexeToCachePartial(int32_t pp_error, void PnaclCoordinator::NexeWasCopiedToCache(int32_t pp_error) { if (pp_error != PP_OK) { - // TODO(jvoung): This should try to delete the partially written - // cache file before returning... - if (pp_error == PP_ERROR_NOQUOTA) { - ReportPpapiError(ERROR_PNACL_CACHE_FINALIZE_COPY_NOQUOTA, - pp_error, - "Failed to copy translated nexe to cache (no quota)."); - return; - } - if (pp_error == PP_ERROR_NOSPACE) { - ReportPpapiError(ERROR_PNACL_CACHE_FINALIZE_COPY_NOSPACE, - pp_error, - "Failed to copy translated nexe to cache (no space)."); - return; - } - ReportPpapiError(ERROR_PNACL_CACHE_FINALIZE_COPY_OTHER, - pp_error, - "Failed to copy translated nexe to cache."); + // Try to delete the partially written not-yet-committed cache file before + // returning. We pass the current pp_error along so that it can be reported + // before returning. + pp::CompletionCallback cb = callback_factory_.NewCallback( + &PnaclCoordinator::CorruptCacheFileWasDeleted, pp_error); + cached_nexe_file_->Delete(cb); return; } // Rename the cached_nexe_file_ file to the cache id, to finalize. @@ -622,6 +611,36 @@ void PnaclCoordinator::NexeWasCopiedToCache(int32_t pp_error) { cached_nexe_file_->Rename(cache_identity_, cb); } +void PnaclCoordinator::CorruptCacheFileWasDeleted(int32_t delete_pp_error, + int32_t orig_pp_error) { + if (delete_pp_error != PP_OK) { + // The cache file was certainly already opened by the time we tried + // to write to it, so it should certainly be deletable. + PLUGIN_PRINTF(("PnaclCoordinator::CorruptCacheFileWasDeleted " + "delete failed with pp_error=%"NACL_PRId32"\n", + delete_pp_error)); + // fall through and report the original error. + } + // Report the original error that caused us to consider the + // cache file corrupted. + if (orig_pp_error == PP_ERROR_NOQUOTA) { + ReportPpapiError(ERROR_PNACL_CACHE_FINALIZE_COPY_NOQUOTA, + orig_pp_error, + "Failed to copy translated nexe to cache (no quota)."); + return; + } + if (orig_pp_error == PP_ERROR_NOSPACE) { + ReportPpapiError(ERROR_PNACL_CACHE_FINALIZE_COPY_NOSPACE, + orig_pp_error, + "Failed to copy translated nexe to cache (no space)."); + return; + } + ReportPpapiError(ERROR_PNACL_CACHE_FINALIZE_COPY_OTHER, + orig_pp_error, + "Failed to copy translated nexe to cache."); + return; +} + void PnaclCoordinator::NexeFileWasRenamed(int32_t pp_error) { PLUGIN_PRINTF(("PnaclCoordinator::NexeFileWasRenamed (pp_error=%" NACL_PRId32")\n", pp_error)); @@ -640,9 +659,7 @@ void PnaclCoordinator::NexeFileWasRenamed(int32_t pp_error) { // NOTE: if the file already existed, it looks like the rename will // happily succeed. However, we should add a test for this. // Could be a hash collision, or it could also be two tabs racing to - // translate the same pexe. The file could also be a corrupt left-over, - // but that case can be removed by doing the TODO for cleanup. - // We may want UMA stats to know if this happens. + // translate the same pexe. We may want UMA stats to know if this happens. // For now, assume that it is a race and try to continue. // If there is truly a corrupted file, then sel_ldr should prevent the // file from loading due to the file size not matching the ELF header. diff --git a/ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h b/ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h index a36b99ead98c..0113d0166a81 100644 --- a/ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h +++ b/ppapi/native_client/src/trusted/plugin/pnacl_coordinator.h @@ -187,6 +187,11 @@ class PnaclCoordinator: public CallbackSource { void DidCopyNexeToCachePartial(int32_t pp_error, int32_t num_read_prev, int64_t cur_offset); void NexeWasCopiedToCache(int32_t pp_error); + // If the copy of the nexe to the not-yet-committed-to-cache file + // failed after partial writes, we attempt to delete the partially written + // file. This callback is invoked when the delete is completed. + void CorruptCacheFileWasDeleted(int32_t delete_pp_error, + int32_t orig_pp_error); // Invoked when the nexe_file_ temporary has been renamed to the nexe name. void NexeFileWasRenamed(int32_t pp_error); // Invoked when the read descriptor for nexe_file_ is created. diff --git a/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc b/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc index e51a0ceb5c6d..2be99684f176 100644 --- a/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc +++ b/ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc @@ -74,8 +74,8 @@ void PnaclTranslateThread::RunTranslate( // Called from main thread to send bytes to the translator. void PnaclTranslateThread::PutBytes(std::vector* bytes, int count) { - PLUGIN_PRINTF(("PutBytes, this %p bytes %p, size %d, count %d\n", this, bytes, - bytes ? bytes->size(): 0, count)); + PLUGIN_PRINTF(("PutBytes (this=%p, bytes=%p, size=%"NACL_PRIuS", count=%d)\n", + this, bytes, bytes ? bytes->size() : 0, count)); size_t buffer_size = 0; // If we are done (error or not), Signal the translation thread to stop. if (count <= PP_OK) { @@ -179,7 +179,7 @@ void PnaclTranslateThread::DoTranslate() { while(!done_ && data_buffers_.size() == 0) { NaClXCondVarWait(&buffer_cond_, &cond_mu_); } - PLUGIN_PRINTF(("PnaclTranslateThread awake, done %d, size %d\n", + PLUGIN_PRINTF(("PnaclTranslateThread awake (done=%d, size=%"NACL_PRIuS")\n", done_, data_buffers_.size())); if (data_buffers_.size() > 0) { std::vector data; -- 2.11.4.GIT