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..ef844aaa3e2a97a9df5f2d0d23adac2c2e25ad6b 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_ = ERROR;                                                   \
       OnError(error_type);                                              \
       return;                                                           \
     }                                                                   \
@@ -62,19 +66,22 @@ CdmFileIOImpl::CdmFileIOImpl(
     : state_(FILE_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() {
+  // The dtor is private. |this| can only be destructed through Close().
   PP_DCHECK(state_ == FILE_CLOSED);
 }
 
-// Call sequence: Open() -> OpenFileSystem() -> OpenFile() -> FILE_OPENED.
+// Call sequence: Open() -> OpenFileSystem() -> FILE_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());
@@ -85,11 +92,20 @@ void CdmFileIOImpl::Open(const char* file_name, uint32_t file_name_size) {
     return;
   }
 
-  // File name should not contain any path separators.
+  // File name should not be empty and should not start with '_'.
   std::string file_name_str(file_name, file_name_size);
+  if (file_name_str.empty() || file_name_str[0] == '_') {
+    CDM_DLOG() << "Invalid file name.";
+    state_ = ERROR;
+    OnError(OPEN_ERROR);
+    return;
+  }
+
+  // File name should not contain any path separators.
   if (file_name_str.find('/') != std::string::npos ||
       file_name_str.find('\\') != std::string::npos) {
     CDM_DLOG() << "Invalid file name.";
+    state_ = ERROR;
     OnError(OPEN_ERROR);
     return;
   }
@@ -108,11 +124,7 @@ void CdmFileIOImpl::Open(const char* file_name, uint32_t file_name_size) {
 }
 
 // Call sequence:
-//                                       finished
-// Read() -> ReadFile() -> OnFileRead() ----------> Done.
-//               ^              |
-//               | not finished |
-//               |--------------|
+// Read() -> OpenFileForRead() -> ReadFile() -> Done.
 void CdmFileIOImpl::Read() {
   CDM_DLOG() << __FUNCTION__;
   PP_DCHECK(IsMainThread());
@@ -129,22 +141,19 @@ void CdmFileIOImpl::Read() {
     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();
+  OpenFileForRead();
 }
 
 // Call sequence:
-//                                            finished
-// Write() -> WriteFile() -> OnFileWritten() ----------> Done.
-//                ^                  |
-//                |                  | not finished
-//                |------------------|
+// Write() -> OpenTempFileForWrite() -> WriteTempFile() -> RenameTempFile().
+// The file name of the temparory file is /_<original_file_name>.
 void CdmFileIOImpl::Write(const uint8_t* data, uint32_t data_size) {
   CDM_DLOG() << __FUNCTION__;
   PP_DCHECK(IsMainThread());
@@ -169,16 +178,15 @@ void CdmFileIOImpl::Write(const uint8_t* data, uint32_t data_size) {
     PP_DCHECK(!data);
 
   state_ = WRITING_FILE;
-
-  // Always SetLength() in case |data_size| is less than the file size.
-  SetLength(data_size);
+  OpenTempFileForWrite();
 }
 
 void CdmFileIOImpl::Close() {
   CDM_DLOG() << __FUNCTION__;
   PP_DCHECK(IsMainThread());
   PP_DCHECK(state_ != FILE_CLOSED);
-  CloseFile();
+  Reset();
+  state_ = FILE_CLOSED;
   ReleaseFileLock();
   // All pending callbacks are canceled since |callback_factory_| is destroyed.
   delete this;
@@ -256,44 +264,55 @@ void CdmFileIOImpl::OnFileSystemOpened(int32_t result,
   if (result != PP_OK) {
     CDM_DLOG() << "File system open failed asynchronously.";
     ReleaseFileLock();
+    state_ = ERROR;
     OnError(OPEN_ERROR);
     return;
   }
 
   file_system_ = file_system;
-  state_ = OPENING_FILE;
-  OpenFile();
+
+  state_ = FILE_OPENED;
+  client_->OnOpenComplete(cdm::FileIOClient::kSuccess);
 }
 
-void CdmFileIOImpl::OpenFile() {
-  PP_DCHECK(state_ == OPENING_FILE);
+void CdmFileIOImpl::OpenFileForRead() {
+  PP_DCHECK(state_ == READING_FILE);
 
+  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, create a new empty file.
+  int32_t file_open_flag = PP_FILEOPENFLAG_READ | PP_FILEOPENFLAG_CREATE;
+
   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) {
   PP_DCHECK(IsMainThread());
-  PP_DCHECK(state_ == OPENING_FILE);
+  PP_DCHECK(state_ == READING_FILE);
 
   if (result != PP_OK) {
     CDM_DLOG() << "File open failed.";
     ReleaseFileLock();
+    state_ = ERROR;
     OnError(OPEN_ERROR);
     return;
   }
 
-  state_ = FILE_OPENED;
-  client_->OnOpenComplete(cdm::FileIOClient::kSuccess);
+  ReadFile();
 }
 
+// Call sequence:
+//                             finished
+// ReadFile() -> OnFileRead() ----------> Done.
+//     ^              |
+//     | not finished |
+//     |--------------|
 void CdmFileIOImpl::ReadFile() {
   PP_DCHECK(state_ == READING_FILE);
   PP_DCHECK(!io_buffer_.empty());
@@ -313,6 +332,7 @@ void CdmFileIOImpl::OnFileRead(int32_t bytes_read) {
   // 0 |bytes_read| indicates end-of-file reached.
   if (bytes_read < PP_OK) {
     CDM_DLOG() << "Read file failed.";
+    state_ = ERROR;
     OnError(READ_ERROR);
     return;
   }
@@ -324,21 +344,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 +367,71 @@ void CdmFileIOImpl::OnFileRead(int32_t bytes_read) {
     first_file_read_reported_ = true;
   }
 
+  Reset();
+
+  state_ = FILE_OPENED;
   client_->OnReadComplete(
       cdm::FileIOClient::kSuccess, data, local_buffer.size());
 }
 
-void CdmFileIOImpl::SetLength(uint32_t length) {
+void CdmFileIOImpl::OpenTempFileForWrite() {
   PP_DCHECK(state_ == WRITING_FILE);
 
+  PP_DCHECK(file_name_.size() > 1 && file_name_[0] == '/');
+  // Temporary file name format: /_<original_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.
+  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);
 
   if (result != PP_OK) {
-    CDM_DLOG() << "File SetLength failed.";
+    CDM_DLOG() << "Open temporary file failed.";
+    state_ = ERROR;
     OnError(WRITE_ERROR);
     return;
   }
 
   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() {
+// Call sequence:
+//                                         finished
+// WriteTempFile() -> OnTempFileWritten() ----------> RenameTempFile().
+//      ^                      |
+//      |      not finished    |
+//      |----------------------|
+void CdmFileIOImpl::WriteTempFile() {
   PP_DCHECK(state_ == WRITING_FILE);
   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 +439,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);
 
   if (bytes_written <= PP_OK) {
-    CDM_DLOG() << "Write file failed.";
-    OnError(READ_ERROR);
+    CDM_DLOG() << "Write temporary file failed.";
+    state_ = ERROR;
+    OnError(WRITE_ERROR);
     return;
   }
 
@@ -409,35 +455,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::RenameTempFile() {
+  PP_DCHECK(state_ == WRITING_FILE);
+
+  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);

[xhwang, 2014/09/15 23:56:15]

tzik: It seems when calling Rename(), file_ref_ should be opened, but the target
file-ref (pp::FileRef(file_system_, file_name_.c_str()) here) should not be
opened. This works for me, but I'd like you to help check whether I am using the
API correctly.

 }
 
-void CdmFileIOImpl::CloseFile() {
+void CdmFileIOImpl::OnTempFileRenamed(int32_t result) {
+  CDM_DLOG() << __FUNCTION__ << ": " << result;
   PP_DCHECK(IsMainThread());
+  PP_DCHECK(state_ == WRITING_FILE);
 
-  state_ = FILE_CLOSED;
+  if (result != PP_OK) {
+    CDM_DLOG() << "Rename temporary file failed.";
+    state_ = ERROR;
+    OnError(WRITE_ERROR);
+    return;
+  }
 
-  file_io_.Close();
+  Reset();
+
+  state_ = FILE_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();

[xhwang, 2014/09/15 23:56:15]

It seems this is not needed if I reset the resource immediately below:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/file_io....

But it doesn't hurt to be explicit I guess...

+  file_io_.detach();

[xhwang, 2014/09/15 23:56:15]

Is this the correct way to set the resource to null? 

I ignored the returned resource, will I leak the resource?

https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/resource...

[ddorwin, 2014/09/16 18:07:36]

Reading the documentation, this appears to leak. Var's Detach is more explicit
about this:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/var.h&q=...

Resource has a protected Clear() function, but it does not appear to be exposed.
I think you might need to delete the object. :(

[xhwang, 2014/09/16 19:56:21]

+dmichael: What's the recommended way to destroy a Resource (e.g. pp::FileRef or
pp::FileIO)? We can't use Clear() and detach() seems to leak the resource....

[dmichael (off chromium), 2014/09/16 20:46:46]

You can just do file_io_ = pp::FileIO();

[xhwang, 2014/09/17 21:02:31]

Done.

+  file_ref_.detach();
 }
 
 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));
 }