CdmFileIOImpl: Refactor Write() implementation.

media/cdm/ppapi/cdm_file_io_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/cdm/ppapi/cdm_file_io_impl.h"
 
 #include <algorithm>
 #include <sstream>
 
 #include "media/cdm/ppapi/cdm_logging.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/cpp/dev/url_util_dev.h"
 
 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.
 #define CHECK_PP_OK_COMPLETIONPENDING(func_call, error_type)            \
   do {                                                                  \
     int32_t result = func_call;                                         \
     PP_DCHECK(result != PP_OK);                                         \
     if (result != PP_OK_COMPLETIONPENDING) {                            \
       CDM_DLOG() << #func_call << " failed with result: " << result;    \
+      state_ = ERROR;                                                   \
       OnError(error_type);                                              \
       return;                                                           \
     }                                                                   \
   } while (0)
 
 #if !defined(NDEBUG)
 // PPAPI calls should only be made on the main thread. In this file, main thread
 // checking is only performed in public APIs and the completion callbacks. This
 // ensures all functions are running on the main thread since internal methods
 // are called either by the public APIs or by the completion callbacks.
@@ skipping 19 matching lines @@
   delete CdmFileIOImpl::file_lock_map_;
 }
 
 CdmFileIOImpl::CdmFileIOImpl(
     cdm::FileIOClient* client,
     PP_Instance pp_instance,
     const pp::CompletionCallback& first_file_read_cb)
     : 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());
 
   if (state_ != FILE_UNOPENED) {
     CDM_DLOG() << "Open() called in an invalid state.";
     OnError(OPEN_ERROR);
     return;
   }
 
+  // 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.
-  std::string file_name_str(file_name, file_name_size);
   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;
   }
 
   // pp::FileRef only accepts path that begins with a '/' character.
   file_name_ = '/' + file_name_str;
 
   if (!AcquireFileLock()) {
     CDM_DLOG() << "File is in use by other cdm::FileIO objects.";
     OnError(OPEN_WHILE_IN_USE);
     return;
   }
 
   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) {
     CDM_DLOG() << "Read() called during pending read/write.";
     OnError(READ_WHILE_IN_USE);
     return;
   }
 
   if (state_ != FILE_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();
+  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());
 
   if (state_ == READING_FILE || state_ == WRITING_FILE) {
     CDM_DLOG() << "Write() called during pending read/write.";
     OnError(WRITE_WHILE_IN_USE);
     return;
   }
 
   if (state_ != FILE_OPENED) {
     CDM_DLOG() << "Write() called in an invalid state.";
     OnError(WRITE_ERROR);
     return;
   }
 
   PP_DCHECK(io_offset_ == 0);
   PP_DCHECK(io_buffer_.empty());
   if (data_size > 0)
     io_buffer_.assign(data, data + data_size);
   else
     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;
 }
 
 bool CdmFileIOImpl::SetFileID() {
   PP_DCHECK(file_id_.empty());
   PP_DCHECK(!file_name_.empty() && file_name_[0] == '/');
 
   // Not taking ownership of |url_util_dev| (which is a singleton).
@@ skipping 57 matching lines @@
 }
 
 void CdmFileIOImpl::OnFileSystemOpened(int32_t result,
                                        pp::FileSystem file_system) {
   PP_DCHECK(IsMainThread());
   PP_DCHECK(state_ == OPENING_FILE_SYSTEM);
 
   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();
-}
-
-void CdmFileIOImpl::OpenFile() {
-  PP_DCHECK(state_ == OPENING_FILE);
-
-  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;
-  pp::CompletionCallback cb =
-      callback_factory_.NewCallback(&CdmFileIOImpl::OnFileOpened);
-  CHECK_PP_OK_COMPLETIONPENDING(file_io_.Open(file_ref_, file_open_flag, cb),
-                                OPEN_ERROR);
-}
-
-void CdmFileIOImpl::OnFileOpened(int32_t result) {
-  PP_DCHECK(IsMainThread());
-  PP_DCHECK(state_ == OPENING_FILE);
-
-  if (result != PP_OK) {
-    CDM_DLOG() << "File open failed.";
-    ReleaseFileLock();
-    OnError(OPEN_ERROR);
-    return;
-  }
 
   state_ = FILE_OPENED;
   client_->OnOpenComplete(cdm::FileIOClient::kSuccess);
 }
 
+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());
+
+  // 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::OnFileOpenedForRead);
+  CHECK_PP_OK_COMPLETIONPENDING(file_io_.Open(file_ref_, file_open_flag, cb),
+                                READ_ERROR);
+}
+
+void CdmFileIOImpl::OnFileOpenedForRead(int32_t result) {
+  PP_DCHECK(IsMainThread());
+  PP_DCHECK(state_ == READING_FILE);
+
+  if (result != PP_OK) {
+    CDM_DLOG() << "File open failed.";
+    ReleaseFileLock();
+    state_ = ERROR;
+    OnError(OPEN_ERROR);
+    return;
+  }
+
+  ReadFile();
+}
+
+// Call sequence:
+//                             finished
+// ReadFile() -> OnFileRead() ----------> Done.
+//     ^              |
+//     | not finished |
+//     |--------------|
 void CdmFileIOImpl::ReadFile() {
   PP_DCHECK(state_ == READING_FILE);
   PP_DCHECK(!io_buffer_.empty());
 
   pp::CompletionCallback cb =
       callback_factory_.NewCallback(&CdmFileIOImpl::OnFileRead);
   CHECK_PP_OK_COMPLETIONPENDING(
       file_io_.Read(io_offset_, &io_buffer_[0], io_buffer_.size(), cb),
       READ_ERROR);
 }
 
 void CdmFileIOImpl::OnFileRead(int32_t bytes_read) {
   CDM_DLOG() << __FUNCTION__ << ": " << bytes_read;
   PP_DCHECK(IsMainThread());
   PP_DCHECK(state_ == READING_FILE);
 
   // 0 |bytes_read| indicates end-of-file reached.
   if (bytes_read < PP_OK) {
     CDM_DLOG() << "Read file failed.";
+    state_ = ERROR;
     OnError(READ_ERROR);
     return;
   }
 
   PP_DCHECK(static_cast<size_t>(bytes_read) <= io_buffer_.size());
   // Append |bytes_read| in |io_buffer_| to |cumulative_read_buffer_|.
   cumulative_read_buffer_.insert(cumulative_read_buffer_.end(),
                                  io_buffer_.begin(),
                                  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]);
 
   // Call this before OnReadComplete() so that we always have the latest file
   // size before CDM fires errors.
   if (!first_file_read_reported_) {
     first_file_read_cb_.Run(local_buffer.size());
     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_,
                                                cb),
                                 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;
   }
 
   io_offset_ += bytes_written;
   PP_DCHECK(io_offset_ <= io_buffer_.size());
 
   if (io_offset_ < io_buffer_.size()) {
-    WriteFile();
+    WriteTempFile();
     return;
   }
 
-  io_buffer_.clear();
-  io_offset_ = 0;
+  // 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::OnTempFileRenamed(int32_t result) {
+  CDM_DLOG() << __FUNCTION__ << ": " << result;
+  PP_DCHECK(IsMainThread());
+  PP_DCHECK(state_ == WRITING_FILE);
+
+  if (result != PP_OK) {
+    CDM_DLOG() << "Rename temporary file failed.";
+    state_ = ERROR;
+    OnError(WRITE_ERROR);
+    return;
+  }
+
+  Reset();
+
   state_ = FILE_OPENED;
   client_->OnWriteComplete(cdm::FileIOClient::kSuccess);
 }
 
-void CdmFileIOImpl::CloseFile() {
+void CdmFileIOImpl::Reset() {
   PP_DCHECK(IsMainThread());
-
-  state_ = FILE_CLOSED;
-
-  file_io_.Close();
   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));
 }
 
 void CdmFileIOImpl::NotifyClientOfError(int32_t result,
                                         ErrorType error_type) {
   PP_DCHECK(result == PP_OK);
   switch (error_type) {
     case OPEN_ERROR:
       client_->OnOpenComplete(cdm::FileIOClient::kError);
@@ skipping 10 matching lines @@
     case READ_WHILE_IN_USE:
       client_->OnReadComplete(cdm::FileIOClient::kInUse, NULL, 0);
       break;
     case WRITE_WHILE_IN_USE:
       client_->OnWriteComplete(cdm::FileIOClient::kInUse);
       break;
   }
 }
 
 }  // namespace media