CdmFileIOImpl: Refactor Write() implementation.

media/cdm/ppapi/cdm_file_io_impl.cc

Index: media/cdm/ppapi/cdm_file_io_impl.cc
diff --git a/media/cdm/ppapi/cdm_file_io_impl.cc b/media/cdm/ppapi/cdm_file_io_impl.cc
index 3409f16fd581f9258f23f0096aa1c0ea97ec9765..fb95278c319766b5f46e6f6a8fa65d4f5d4be732 100644
--- a/media/cdm/ppapi/cdm_file_io_impl.cc
+++ b/media/cdm/ppapi/cdm_file_io_impl.cc
@@ -13,7 +13,10 @@
 
 namespace media {
 
-const int kReadSize = 4 * 1024;  // Arbitrary choice.
+// Arbitrary choice based on the following heuristic ideas:
+// - not too big to avoid unnecessarily large memory allocation;
+// - not too small to avoid breaking most reads into multiple read operations.
+const int kReadSize = 8 * 1024;
 
 // Call func_call and check the result. If the result is not
 // PP_OK_COMPLETIONPENDING, print out logs, call OnError() and return.
@@ -23,6 +26,7 @@ const int kReadSize = 4 * 1024;  // Arbitrary choice.
     PP_DCHECK(result != PP_OK);                                         \
     if (result != PP_OK_COMPLETIONPENDING) {                            \
       CDM_DLOG() << #func_call << " failed with result: " << result;    \
+      state_ = STATE_ERROR;                                             \
       OnError(error_type);                                              \
       return;                                                           \
     }                                                                   \
@@ -59,37 +63,43 @@ CdmFileIOImpl::CdmFileIOImpl(
     cdm::FileIOClient* client,
     PP_Instance pp_instance,
     const pp::CompletionCallback& first_file_read_cb)
-    : state_(FILE_UNOPENED),
+    : state_(STATE_UNOPENED),
       client_(client),
       pp_instance_handle_(pp_instance),
-      callback_factory_(this),
       io_offset_(0),
       first_file_read_reported_(false),
-      first_file_read_cb_(first_file_read_cb) {
+      first_file_read_cb_(first_file_read_cb),
+      callback_factory_(this) {
   PP_DCHECK(IsMainThread());
   PP_DCHECK(pp_instance);  // 0 indicates a "NULL handle".
 }
 
 CdmFileIOImpl::~CdmFileIOImpl() {
-  PP_DCHECK(state_ == FILE_CLOSED);
+  // The destructor is private. |this| can only be destructed through Close().
+  PP_DCHECK(state_ == STATE_CLOSED);
 }
 
-// Call sequence: Open() -> OpenFileSystem() -> OpenFile() -> FILE_OPENED.
+// Call sequence: Open() -> OpenFileSystem() -> STATE_FILE_SYSTEM_OPENED.
+// Note: This only stores file name and opens the file system. The real file
+// open is deferred to when Read() or Write() is called.
 void CdmFileIOImpl::Open(const char* file_name, uint32_t file_name_size) {
   CDM_DLOG() << __FUNCTION__;
   PP_DCHECK(IsMainThread());
 
-  if (state_ != FILE_UNOPENED) {
+  if (state_ != STATE_UNOPENED) {
     CDM_DLOG() << "Open() called in an invalid state.";
     OnError(OPEN_ERROR);
     return;
   }
 
-  // File name should not contain any path separators.
+  // File name should not (1) be empty, (2) start with '_', or (3) contain any
+  // path separators.
   std::string file_name_str(file_name, file_name_size);
-  if (file_name_str.find('/') != std::string::npos ||
+  if (file_name_str.empty() || file_name_str[0] == '_' ||

[ddorwin, 2014/09/17 23:16:38]

nit: Would be nice to have all the independent checks on separate lines.

[xhwang, 2014/09/18 05:58:07]

Done.

+      file_name_str.find('/') != std::string::npos ||
       file_name_str.find('\\') != std::string::npos) {
     CDM_DLOG() << "Invalid file name.";
+    state_ = STATE_ERROR;
     OnError(OPEN_ERROR);
     return;
   }
@@ -103,59 +113,52 @@ void CdmFileIOImpl::Open(const char* file_name, uint32_t file_name_size) {
     return;
   }
 
-  state_ = OPENING_FILE_SYSTEM;
+  state_ = STATE_OPENING_FILE_SYSTEM;
   OpenFileSystem();
 }
 
 // Call sequence:
-//                                       finished
-// Read() -> ReadFile() -> OnFileRead() ----------> Done.
-//               ^              |
-//               | not finished |
-//               |--------------|
+// Read() -> OpenFileForRead() -> ReadFile() -> Done.
 void CdmFileIOImpl::Read() {
   CDM_DLOG() << __FUNCTION__;
   PP_DCHECK(IsMainThread());
 
-  if (state_ == READING_FILE || state_ == WRITING_FILE) {
+  if (state_ == STATE_READING || state_ == STATE_WRITING) {
     CDM_DLOG() << "Read() called during pending read/write.";
     OnError(READ_WHILE_IN_USE);
     return;
   }
 
-  if (state_ != FILE_OPENED) {
+  if (state_ != STATE_FILE_SYSTEM_OPENED) {
     CDM_DLOG() << "Read() called in an invalid state.";
     OnError(READ_ERROR);
     return;
   }
 
+  PP_DCHECK(io_offset_ == 0);
   PP_DCHECK(io_buffer_.empty());
   PP_DCHECK(cumulative_read_buffer_.empty());
-
   io_buffer_.resize(kReadSize);
   io_offset_ = 0;
 
-  state_ = READING_FILE;
-  ReadFile();
+  state_ = STATE_READING;
+  OpenFileForRead();
 }
 
 // Call sequence:
-//                                            finished
-// Write() -> WriteFile() -> OnFileWritten() ----------> Done.
-//                ^                  |
-//                |                  | not finished
-//                |------------------|
+// Write() -> OpenTempFileForWrite() -> WriteTempFile() -> RenameTempFile().
+// The file name of the temporary file is /_<requested_file_name>.
 void CdmFileIOImpl::Write(const uint8_t* data, uint32_t data_size) {
   CDM_DLOG() << __FUNCTION__;
   PP_DCHECK(IsMainThread());
 
-  if (state_ == READING_FILE || state_ == WRITING_FILE) {
+  if (state_ == STATE_READING || state_ == STATE_WRITING) {
     CDM_DLOG() << "Write() called during pending read/write.";
     OnError(WRITE_WHILE_IN_USE);
     return;
   }
 
-  if (state_ != FILE_OPENED) {
+  if (state_ != STATE_FILE_SYSTEM_OPENED) {
     CDM_DLOG() << "Write() called in an invalid state.";
     OnError(WRITE_ERROR);
     return;
@@ -168,17 +171,16 @@ void CdmFileIOImpl::Write(const uint8_t* data, uint32_t data_size) {
   else
     PP_DCHECK(!data);
 
-  state_ = WRITING_FILE;
-
-  // Always SetLength() in case |data_size| is less than the file size.
-  SetLength(data_size);
+  state_ = STATE_WRITING;
+  OpenTempFileForWrite();
 }
 
 void CdmFileIOImpl::Close() {
   CDM_DLOG() << __FUNCTION__;
   PP_DCHECK(IsMainThread());
-  PP_DCHECK(state_ != FILE_CLOSED);
-  CloseFile();
+  PP_DCHECK(state_ != STATE_CLOSED);
+  Reset();
+  state_ = STATE_CLOSED;
   ReleaseFileLock();
   // All pending callbacks are canceled since |callback_factory_| is destroyed.
   delete this;
@@ -237,7 +239,7 @@ void CdmFileIOImpl::ReleaseFileLock() {
 }
 
 void CdmFileIOImpl::OpenFileSystem() {
-  PP_DCHECK(state_ == OPENING_FILE_SYSTEM);
+  PP_DCHECK(state_ == STATE_OPENING_FILE_SYSTEM);
 
   pp::CompletionCallbackWithOutput<pp::FileSystem> cb =
       callback_factory_.NewCallbackWithOutput(
@@ -251,51 +253,71 @@ void CdmFileIOImpl::OpenFileSystem() {
 void CdmFileIOImpl::OnFileSystemOpened(int32_t result,
                                        pp::FileSystem file_system) {
   PP_DCHECK(IsMainThread());
-  PP_DCHECK(state_ == OPENING_FILE_SYSTEM);
+  PP_DCHECK(state_ == STATE_OPENING_FILE_SYSTEM);
 
   if (result != PP_OK) {
     CDM_DLOG() << "File system open failed asynchronously.";
     ReleaseFileLock();
+    state_ = STATE_ERROR;
     OnError(OPEN_ERROR);
     return;
   }
 
   file_system_ = file_system;
-  state_ = OPENING_FILE;
-  OpenFile();
+
+  state_ = STATE_FILE_SYSTEM_OPENED;
+  client_->OnOpenComplete(cdm::FileIOClient::kSuccess);
 }
 
-void CdmFileIOImpl::OpenFile() {
-  PP_DCHECK(state_ == OPENING_FILE);
+void CdmFileIOImpl::OpenFileForRead() {
+  PP_DCHECK(state_ == STATE_READING);
 
+  PP_DCHECK(file_io_.is_null());
+  PP_DCHECK(file_ref_.is_null());
   file_io_ = pp::FileIO(pp_instance_handle_);
   file_ref_ = pp::FileRef(file_system_, file_name_.c_str());
-  int32_t file_open_flag = PP_FILEOPENFLAG_READ |
-                           PP_FILEOPENFLAG_WRITE |
-                           PP_FILEOPENFLAG_CREATE;
+
+  // Open file for read. If file doesn't exist, PP_ERROR_FILENOTFOUND will be
+  // returned.
+  int32_t file_open_flag = PP_FILEOPENFLAG_READ;
+
   pp::CompletionCallback cb =
-      callback_factory_.NewCallback(&CdmFileIOImpl::OnFileOpened);
+      callback_factory_.NewCallback(&CdmFileIOImpl::OnFileOpenedForRead);
   CHECK_PP_OK_COMPLETIONPENDING(file_io_.Open(file_ref_, file_open_flag, cb),
-                                OPEN_ERROR);
+                                READ_ERROR);
 }
 
-void CdmFileIOImpl::OnFileOpened(int32_t result) {
+void CdmFileIOImpl::OnFileOpenedForRead(int32_t result) {
+  CDM_DLOG() << __FUNCTION__ << ": " << result;
   PP_DCHECK(IsMainThread());
-  PP_DCHECK(state_ == OPENING_FILE);
+  PP_DCHECK(state_ == STATE_READING);
 
-  if (result != PP_OK) {
+  if (result != PP_OK && result != PP_ERROR_FILENOTFOUND) {

[ddorwin, 2014/09/17 23:16:38]

Avoid this by checking the specific error first?

[xhwang, 2014/09/18 05:58:07]

I'd like to handle "error" cases first. PP_ERROR_FILENOTFOUND is actually not an
error in our case...

     CDM_DLOG() << "File open failed.";
-    ReleaseFileLock();
+    state_ = STATE_ERROR;

[ddorwin, 2014/09/17 23:16:38]

Who is releasing the lock now? In fact, it's not clear where this is being
released except Close(). Does the caller need to call Close before opening
another file?

[xhwang, 2014/09/18 05:58:07]

The FileIO doc says that when a FileIO is opened, it's "in use":

https://code.google.com/p/chromium/codesearch#chromium/src/media/cdm/ppapi/ap...

So we only ReleaseFileLock() if Open() failed (i.e. open file system failed). If
Open() succeeded, we won't release the lock until Close() is called.

Since we moved OpenFile() from the Open() sequence to the Read() sequence,
ReleaseFileLock() is not needed here now.

     OnError(OPEN_ERROR);
     return;
   }
 
-  state_ = FILE_OPENED;
-  client_->OnOpenComplete(cdm::FileIOClient::kSuccess);
+  // File doesn't exist.
+  if (result == PP_ERROR_FILENOTFOUND) {
+    Reset();
+    state_ = STATE_FILE_SYSTEM_OPENED;
+    client_->OnReadComplete(cdm::FileIOClient::kSuccess, NULL, 0);
+    return;
+  }
+
+  ReadFile();
 }
 
+// Call sequence:
+//                               fully read
+// ReadFile() ---> OnFileRead() ------------> Done.
+//     ^                |
+//     | partially read |
+//     |----------------|
 void CdmFileIOImpl::ReadFile() {
-  PP_DCHECK(state_ == READING_FILE);
+  PP_DCHECK(state_ == STATE_READING);
   PP_DCHECK(!io_buffer_.empty());
 
   pp::CompletionCallback cb =
@@ -308,11 +330,12 @@ void CdmFileIOImpl::ReadFile() {
 void CdmFileIOImpl::OnFileRead(int32_t bytes_read) {
   CDM_DLOG() << __FUNCTION__ << ": " << bytes_read;
   PP_DCHECK(IsMainThread());
-  PP_DCHECK(state_ == READING_FILE);
+  PP_DCHECK(state_ == STATE_READING);
 
   // 0 |bytes_read| indicates end-of-file reached.
   if (bytes_read < PP_OK) {
     CDM_DLOG() << "Read file failed.";
+    state_ = STATE_ERROR;
     OnError(READ_ERROR);
     return;
   }
@@ -324,21 +347,19 @@ void CdmFileIOImpl::OnFileRead(int32_t bytes_read) {
                                  io_buffer_.begin() + bytes_read);
   io_offset_ += bytes_read;
 
-  // Not received end-of-file yet.
+  // Not received end-of-file yet. Keep reading.
   if (bytes_read > 0) {
     ReadFile();
     return;
   }
 
   // We hit end-of-file. Return read data to the client.
-  io_buffer_.clear();
-  io_offset_ = 0;
+
   // Clear |cumulative_read_buffer_| in case OnReadComplete() calls Read() or
   // Write().
   std::vector<char> local_buffer;
   std::swap(cumulative_read_buffer_, local_buffer);
 
-  state_ = FILE_OPENED;
   const uint8_t* data = local_buffer.empty() ?
       NULL : reinterpret_cast<const uint8_t*>(&local_buffer[0]);
 
@@ -349,44 +370,75 @@ void CdmFileIOImpl::OnFileRead(int32_t bytes_read) {
     first_file_read_reported_ = true;
   }
 
+  Reset();
+
+  state_ = STATE_FILE_SYSTEM_OPENED;
   client_->OnReadComplete(
       cdm::FileIOClient::kSuccess, data, local_buffer.size());
 }
 
-void CdmFileIOImpl::SetLength(uint32_t length) {
-  PP_DCHECK(state_ == WRITING_FILE);
+void CdmFileIOImpl::OpenTempFileForWrite() {
+  PP_DCHECK(state_ == STATE_WRITING);
+
+  PP_DCHECK(file_name_.size() > 1 && file_name_[0] == '/');
+  // Temporary file name format: /_<requested_file_name>
+  std::string temp_file_name = "/_" + file_name_.substr(1);
+
+  PP_DCHECK(file_io_.is_null());
+  PP_DCHECK(file_ref_.is_null());
+  file_io_ = pp::FileIO(pp_instance_handle_);
+  file_ref_ = pp::FileRef(file_system_, temp_file_name.c_str());
+
+  // Create the file if it doesn't exist. Truncate the file to length 0 if it
+  // exists.
+  // TODO(xhwang): Find a good way to report to UMA cases where the temporary

[ddorwin, 2014/09/17 23:16:38]

Thanks. Would the following work (separate CL is fine)?
open(WRITE);
if (success)
  setlength(0);
else
  open(WRITE|CREATE);

[xhwang, 2014/09/18 05:58:07]

That'll work, but we are less efficient. As I said, I was a bit hesitating to
sacrifice performance/simplicity for logging/UMA purpose...

+  // file already exists (due to previous interruption or failure).
+  int32_t file_open_flag = PP_FILEOPENFLAG_WRITE |
+                           PP_FILEOPENFLAG_TRUNCATE |
+                           PP_FILEOPENFLAG_CREATE;
 
   pp::CompletionCallback cb =
-      callback_factory_.NewCallback(&CdmFileIOImpl::OnLengthSet);
-  CHECK_PP_OK_COMPLETIONPENDING(file_io_.SetLength(length, cb), WRITE_ERROR);
+      callback_factory_.NewCallback(&CdmFileIOImpl::OnTempFileOpenedForWrite);
+  CHECK_PP_OK_COMPLETIONPENDING(
+      file_io_.Open(file_ref_, file_open_flag, cb), WRITE_ERROR);
 }
 
-void CdmFileIOImpl::OnLengthSet(int32_t result) {
+void CdmFileIOImpl::OnTempFileOpenedForWrite(int32_t result) {
   CDM_DLOG() << __FUNCTION__ << ": " << result;
   PP_DCHECK(IsMainThread());
-  PP_DCHECK(state_ == WRITING_FILE);
+  PP_DCHECK(state_ == STATE_WRITING);
 
   if (result != PP_OK) {
-    CDM_DLOG() << "File SetLength failed.";
+    CDM_DLOG() << "Open temporary file failed.";
+    state_ = STATE_ERROR;
     OnError(WRITE_ERROR);
     return;
   }
 
+  // We were told to write 0 bytes (to clear the file). In this case, there's
+  // no need to write anything.
   if (io_buffer_.empty()) {
-    state_ = FILE_OPENED;
-    client_->OnWriteComplete(cdm::FileIOClient::kSuccess);
+    RenameTempFile();
     return;
   }
 
-  WriteFile();
+  PP_DCHECK(io_offset_ == 0);
+  io_offset_ = 0;
+  WriteTempFile();
 }
 
-void CdmFileIOImpl::WriteFile() {
-  PP_DCHECK(state_ == WRITING_FILE);
+// Call sequence:
+//                                         fully written
+// WriteTempFile() -> OnTempFileWritten() ---------------> RenameTempFile().
+//      ^                     |
+//      |  partially written  |
+//      |---------------------|
+void CdmFileIOImpl::WriteTempFile() {
+  PP_DCHECK(state_ == STATE_WRITING);
   PP_DCHECK(io_offset_ < io_buffer_.size());
 
   pp::CompletionCallback cb =
-      callback_factory_.NewCallback(&CdmFileIOImpl::OnFileWritten);
+      callback_factory_.NewCallback(&CdmFileIOImpl::OnTempFileWritten);
   CHECK_PP_OK_COMPLETIONPENDING(file_io_.Write(io_offset_,
                                                &io_buffer_[io_offset_],
                                                io_buffer_.size() - io_offset_,
@@ -394,14 +446,15 @@ void CdmFileIOImpl::WriteFile() {
                                 WRITE_ERROR);
 }
 
-void CdmFileIOImpl::OnFileWritten(int32_t bytes_written) {
+void CdmFileIOImpl::OnTempFileWritten(int32_t bytes_written) {
   CDM_DLOG() << __FUNCTION__ << ": " << bytes_written;
   PP_DCHECK(IsMainThread());
-  PP_DCHECK(state_ == WRITING_FILE);
+  PP_DCHECK(state_ == STATE_WRITING);
 
   if (bytes_written <= PP_OK) {
-    CDM_DLOG() << "Write file failed.";
-    OnError(READ_ERROR);
+    CDM_DLOG() << "Write temporary file failed.";
+    state_ = STATE_ERROR;
+    OnError(WRITE_ERROR);
     return;
   }
 
@@ -409,35 +462,58 @@ void CdmFileIOImpl::OnFileWritten(int32_t bytes_written) {
   PP_DCHECK(io_offset_ <= io_buffer_.size());
 
   if (io_offset_ < io_buffer_.size()) {
-    WriteFile();
+    WriteTempFile();
     return;
   }
 
-  io_buffer_.clear();
-  io_offset_ = 0;
-  state_ = FILE_OPENED;
-  client_->OnWriteComplete(cdm::FileIOClient::kSuccess);
+  // All data written. Now rename the temporary file to the real file.
+  RenameTempFile();
 }
 
-void CdmFileIOImpl::CloseFile() {
+void CdmFileIOImpl::RenameTempFile() {
+  PP_DCHECK(state_ == STATE_WRITING);
+
+  pp::CompletionCallback cb =
+      callback_factory_.NewCallback(&CdmFileIOImpl::OnTempFileRenamed);
+  CHECK_PP_OK_COMPLETIONPENDING(
+      file_ref_.Rename(pp::FileRef(file_system_, file_name_.c_str()), cb),
+      WRITE_ERROR);
+}
+
+void CdmFileIOImpl::OnTempFileRenamed(int32_t result) {
+  CDM_DLOG() << __FUNCTION__ << ": " << result;
   PP_DCHECK(IsMainThread());
+  PP_DCHECK(state_ == STATE_WRITING);
 
-  state_ = FILE_CLOSED;
+  if (result != PP_OK) {
+    CDM_DLOG() << "Rename temporary file failed.";
+    state_ = STATE_ERROR;
+    OnError(WRITE_ERROR);
+    return;
+  }
 
-  file_io_.Close();
+  Reset();
+
+  state_ = STATE_FILE_SYSTEM_OPENED;
+  client_->OnWriteComplete(cdm::FileIOClient::kSuccess);
+}
+
+void CdmFileIOImpl::Reset() {
+  PP_DCHECK(IsMainThread());
   io_buffer_.clear();
   io_offset_ = 0;
   cumulative_read_buffer_.clear();
+  file_io_.Close();
+  file_io_ = pp::FileIO();
+  file_ref_ = pp::FileRef();
 }
 
 void CdmFileIOImpl::OnError(ErrorType error_type) {
   // For *_WHILE_IN_USE errors, do not reset these values. Otherwise, the
   // existing read/write operation will fail.
-  if (error_type == READ_ERROR || error_type == WRITE_ERROR) {
-    io_buffer_.clear();
-    io_offset_ = 0;
-    cumulative_read_buffer_.clear();
-  }
+  if (error_type == READ_ERROR || error_type == WRITE_ERROR)
+    Reset();
+
   PostOnMain(callback_factory_.NewCallback(&CdmFileIOImpl::NotifyClientOfError,
                                            error_type));
 }