Skip to content

Commit

Permalink
Remove PlatformFile from ppapi
Browse files Browse the repository at this point in the history
BUG=322664

Review URL: https://codereview.chromium.org/304513007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@274377 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
rvargas@chromium.org committed Jun 3, 2014
1 parent 7a8106e commit 35b05b1
Show file tree
Hide file tree
Showing 13 changed files with 88 additions and 112 deletions.
2 changes: 1 addition & 1 deletion ppapi/nacl_irt/ppapi_dispatcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
#include <set>
#include <string>

#include "base/files/file.h"
#include "base/memory/ref_counted.h"
#include "base/platform_file.h"
#include "base/process/process_handle.h"
#include "ipc/ipc_listener.h"
#include "ipc/ipc_platform_file.h"
Expand Down
87 changes: 37 additions & 50 deletions ppapi/proxy/file_io_resource.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,38 +39,33 @@ void* DummyGetDataBuffer(void* user_data, uint32_t count, uint32_t size) {
}

// File thread task to close the file handle.
void DoClose(base::PlatformFile file) {
base::ClosePlatformFile(file);
void DoClose(base::File auto_close_file) {
}

} // namespace

namespace ppapi {
namespace proxy {

FileIOResource::QueryOp::QueryOp(scoped_refptr<FileHandleHolder> file_handle)
: file_handle_(file_handle) {
DCHECK(file_handle_);
FileIOResource::QueryOp::QueryOp(scoped_refptr<FileHolder> file_holder)
: file_holder_(file_holder) {
DCHECK(file_holder_);
}

FileIOResource::QueryOp::~QueryOp() {
}

int32_t FileIOResource::QueryOp::DoWork() {
// TODO(rvargas): Convert this code to use base::File.
base::File file(file_handle_->raw_handle());
bool success = file.GetInfo(&file_info_);
file.TakePlatformFile();
return success ? PP_OK : PP_ERROR_FAILED;
return file_holder_->file()->GetInfo(&file_info_) ? PP_OK : PP_ERROR_FAILED;
}

FileIOResource::ReadOp::ReadOp(scoped_refptr<FileHandleHolder> file_handle,
FileIOResource::ReadOp::ReadOp(scoped_refptr<FileHolder> file_holder,
int64_t offset,
int32_t bytes_to_read)
: file_handle_(file_handle),
: file_holder_(file_holder),
offset_(offset),
bytes_to_read_(bytes_to_read) {
DCHECK(file_handle_);
DCHECK(file_holder_);
}

FileIOResource::ReadOp::~ReadOp() {
Expand All @@ -79,16 +74,15 @@ FileIOResource::ReadOp::~ReadOp() {
int32_t FileIOResource::ReadOp::DoWork() {
DCHECK(!buffer_.get());
buffer_.reset(new char[bytes_to_read_]);
return base::ReadPlatformFile(
file_handle_->raw_handle(), offset_, buffer_.get(), bytes_to_read_);
return file_holder_->file()->Read(offset_, buffer_.get(), bytes_to_read_);
}

FileIOResource::WriteOp::WriteOp(scoped_refptr<FileHandleHolder> file_handle,
FileIOResource::WriteOp::WriteOp(scoped_refptr<FileHolder> file_holder,
int64_t offset,
scoped_ptr<char[]> buffer,
int32_t bytes_to_write,
bool append)
: file_handle_(file_handle),
: file_holder_(file_holder),
offset_(offset),
buffer_(buffer.Pass()),
bytes_to_write_(bytes_to_write),
Expand All @@ -102,11 +96,10 @@ int32_t FileIOResource::WriteOp::DoWork() {
// In append mode, we can't call WritePlatformFile, since NaCl doesn't
// implement fcntl, causing the function to call pwrite, which is incorrect.
if (append_) {
return base::WritePlatformFileAtCurrentPos(
file_handle_->raw_handle(), buffer_.get(), bytes_to_write_);
return file_holder_->file()->WriteAtCurrentPos(buffer_.get(),
bytes_to_write_);
} else {
return base::WritePlatformFile(
file_handle_->raw_handle(), offset_, buffer_.get(), bytes_to_write_);
return file_holder_->file()->Write(offset_, buffer_.get(), bytes_to_write_);
}
}

Expand Down Expand Up @@ -183,7 +176,7 @@ int32_t FileIOResource::Query(PP_FileInfo* info,
return rv;
if (!info)
return PP_ERROR_BADARGUMENT;
if (!FileHandleHolder::IsValid(file_handle_))
if (!FileHolder::IsValid(file_holder_))
return PP_ERROR_FAILED;

state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_EXCLUSIVE);
Expand All @@ -198,11 +191,7 @@ int32_t FileIOResource::Query(PP_FileInfo* info,
{
// Release the proxy lock while making a potentially slow file call.
ProxyAutoUnlock unlock;
// TODO(rvargas): Convert this code to base::File.
base::File file(file_handle_->raw_handle());
bool success = file.GetInfo(&file_info);
file.TakePlatformFile();
if (success)
if (file_holder_->file()->GetInfo(&file_info))
result = PP_OK;
}
if (result == PP_OK) {
Expand All @@ -217,7 +206,7 @@ int32_t FileIOResource::Query(PP_FileInfo* info,

// For the non-blocking case, post a task to the file thread and add a
// completion task to write the result.
scoped_refptr<QueryOp> query_op(new QueryOp(file_handle_));
scoped_refptr<QueryOp> query_op(new QueryOp(file_holder_));
base::PostTaskAndReplyWithResult(
PpapiGlobals::Get()->GetFileTaskRunner(),
FROM_HERE,
Expand Down Expand Up @@ -282,7 +271,7 @@ int32_t FileIOResource::Write(int64_t offset,
return PP_ERROR_FAILED;
if (offset < 0 || bytes_to_write < 0)
return PP_ERROR_FAILED;
if (!FileHandleHolder::IsValid(file_handle_))
if (!FileHolder::IsValid(file_holder_))
return PP_ERROR_FAILED;

int32_t rv = state_manager_.CheckOperationState(
Expand Down Expand Up @@ -408,8 +397,8 @@ void FileIOResource::Close() {
pp_resource());
}

if (file_handle_)
file_handle_ = NULL;
if (file_holder_)
file_holder_ = NULL;

Post(BROWSER, PpapiHostMsg_FileIO_Close(
FileGrowth(max_written_offset_, append_mode_write_amount_)));
Expand All @@ -432,22 +421,22 @@ int32_t FileIOResource::RequestOSFileHandle(
return PP_OK_COMPLETIONPENDING;
}

FileIOResource::FileHandleHolder::FileHandleHolder(PP_FileHandle file_handle)
: raw_handle_(file_handle) {
FileIOResource::FileHolder::FileHolder(PP_FileHandle file_handle)
: file_(file_handle) {
}

// static
bool FileIOResource::FileHandleHolder::IsValid(
const scoped_refptr<FileIOResource::FileHandleHolder>& handle) {
return handle && (handle->raw_handle() != base::kInvalidPlatformFileValue);
bool FileIOResource::FileHolder::IsValid(
const scoped_refptr<FileIOResource::FileHolder>& handle) {
return handle && handle->file_.IsValid();
}

FileIOResource::FileHandleHolder::~FileHandleHolder() {
if (raw_handle_ != base::kInvalidPlatformFileValue) {
FileIOResource::FileHolder::~FileHolder() {
if (file_.IsValid()) {
base::TaskRunner* file_task_runner =
PpapiGlobals::Get()->GetFileTaskRunner();
file_task_runner->PostTask(FROM_HERE,
base::Bind(&DoClose, raw_handle_));
base::Bind(&DoClose, Passed(&file_)));
}
}

Expand All @@ -457,7 +446,7 @@ int32_t FileIOResource::ReadValidated(int64_t offset,
scoped_refptr<TrackedCallback> callback) {
if (bytes_to_read < 0)
return PP_ERROR_FAILED;
if (!FileHandleHolder::IsValid(file_handle_))
if (!FileHolder::IsValid(file_holder_))
return PP_ERROR_FAILED;

state_manager_.SetPendingOperation(FileIOStateManager::OPERATION_READ);
Expand All @@ -473,8 +462,7 @@ int32_t FileIOResource::ReadValidated(int64_t offset,
if (buffer) {
// Release the proxy lock while making a potentially slow file call.
ProxyAutoUnlock unlock;
result = base::ReadPlatformFile(
file_handle_->raw_handle(), offset, buffer, bytes_to_read);
result = file_holder_->file()->Read(offset, buffer, bytes_to_read);
if (result < 0)
result = PP_ERROR_FAILED;
}
Expand All @@ -484,7 +472,7 @@ int32_t FileIOResource::ReadValidated(int64_t offset,

// For the non-blocking case, post a task to the file thread.
scoped_refptr<ReadOp> read_op(
new ReadOp(file_handle_, offset, bytes_to_read));
new ReadOp(file_holder_, offset, bytes_to_read));
base::PostTaskAndReplyWithResult(
PpapiGlobals::Get()->GetFileTaskRunner(),
FROM_HERE,
Expand All @@ -508,11 +496,10 @@ int32_t FileIOResource::WriteValidated(
// Release the proxy lock while making a potentially slow file call.
ProxyAutoUnlock unlock;
if (append) {
result = base::WritePlatformFileAtCurrentPos(
file_handle_->raw_handle(), buffer, bytes_to_write);
result = file_holder_->file()->WriteAtCurrentPos(buffer,
bytes_to_write);
} else {
result = base::WritePlatformFile(
file_handle_->raw_handle(), offset, buffer, bytes_to_write);
result = file_holder_->file()->Write(offset, buffer, bytes_to_write);
}
}
if (result < 0)
Expand All @@ -527,7 +514,7 @@ int32_t FileIOResource::WriteValidated(
scoped_ptr<char[]> copy(new char[bytes_to_write]);
memcpy(copy.get(), buffer, bytes_to_write);
scoped_refptr<WriteOp> write_op(
new WriteOp(file_handle_, offset, copy.Pass(), bytes_to_write, append));
new WriteOp(file_holder_, offset, copy.Pass(), bytes_to_write, append));
base::PostTaskAndReplyWithResult(
PpapiGlobals::Get()->GetFileTaskRunner(),
FROM_HERE,
Expand Down Expand Up @@ -619,7 +606,7 @@ void FileIOResource::OnRequestWriteQuotaComplete(
} else {
bool append = (open_flags_ & PP_FILEOPENFLAG_APPEND) != 0;
scoped_refptr<WriteOp> write_op(new WriteOp(
file_handle_, offset, buffer.Pass(), bytes_to_write, append));
file_holder_, offset, buffer.Pass(), bytes_to_write, append));
base::PostTaskAndReplyWithResult(
PpapiGlobals::Get()->GetFileTaskRunner(),
FROM_HERE,
Expand Down Expand Up @@ -693,7 +680,7 @@ void FileIOResource::OnPluginMsgOpenFileComplete(

IPC::PlatformFileForTransit transit_file;
if (params.TakeFileHandleAtIndex(0, &transit_file)) {
file_handle_ = new FileHandleHolder(
file_holder_ = new FileHolder(
IPC::PlatformFileForTransitToPlatformFile(transit_file));
}
}
Expand Down
55 changes: 28 additions & 27 deletions ppapi/proxy/file_io_resource.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ class PPAPI_PROXY_EXPORT FileIOResource
PP_FileHandle* handle,
scoped_refptr<TrackedCallback> callback) OVERRIDE;

// FileHandleHolder is used to guarantee that file operations will have a
// valid FD to operate on, even if they're in a different thread.
// FileHolder is used to guarantee that file operations will have a valid FD
// to operate on, even if they're in a different thread.
// If instead we just passed the raw FD, the FD could be closed before the
// file operation has a chance to run. It could interact with an invalid FD,
// or worse, the FD value could be reused if another file is opened quickly
Expand All @@ -80,36 +80,37 @@ class PPAPI_PROXY_EXPORT FileIOResource
//
// Operations that run on a background thread should hold one of these to
// ensure they have a valid file descriptor. The file handle is only closed
// when the last reference to the FileHandleHolder is removed, so we are
// guaranteed to operate on the correct file descriptor. It *is* still
// possible that the FileIOResource will be destroyed and "Abort" callbacks
// just before the operation does its task (e.g., Reading). In that case, we
// might for example Read from a file even though the FileIO has been
// destroyed and the plugin's callback got a PP_ERROR_ABORTED result. In the
// case of a write, we could write some data to the file despite the plugin
// receiving a PP_ERROR_ABORTED instead of a successful result.
class FileHandleHolder : public base::RefCountedThreadSafe<FileHandleHolder> {
// when the last reference to the FileHolder is removed, so we are guaranteed
// to operate on the correct file descriptor. It *is* still possible that the
// FileIOResource will be destroyed and "Abort" callbacks just before the
// operation does its task (e.g., Reading). In that case, we might for example
// Read from a file even though the FileIO has been destroyed and the plugin's
// callback got a PP_ERROR_ABORTED result. In the case of a write, we could
// write some data to the file despite the plugin receiving a
// PP_ERROR_ABORTED instead of a successful result.
class FileHolder : public base::RefCountedThreadSafe<FileHolder> {
public:
explicit FileHandleHolder(PP_FileHandle file_handle_);
PP_FileHandle raw_handle() {
return raw_handle_;
explicit FileHolder(PP_FileHandle file_handle);
base::File* file() {
return &file_;
}
static bool IsValid(
const scoped_refptr<FileIOResource::FileHandleHolder>& handle);
const scoped_refptr<FileIOResource::FileHolder>& handle);
private:
friend class base::RefCountedThreadSafe<FileHandleHolder>;
~FileHandleHolder();
PP_FileHandle raw_handle_;
friend class base::RefCountedThreadSafe<FileHolder>;
~FileHolder();
base::File file_;
};
scoped_refptr<FileHandleHolder> file_handle() {
return file_handle_;

scoped_refptr<FileHolder> file_holder() {
return file_holder_;
}

private:
// Class to perform file query operations across multiple threads.
class QueryOp : public base::RefCountedThreadSafe<QueryOp> {
public:
explicit QueryOp(scoped_refptr<FileHandleHolder> file_handle);
explicit QueryOp(scoped_refptr<FileHolder> file_holder);

// Queries the file. Called on the file thread (non-blocking) or the plugin
// thread (blocking). This should not be called when we hold the proxy lock.
Expand All @@ -121,14 +122,14 @@ class PPAPI_PROXY_EXPORT FileIOResource
friend class base::RefCountedThreadSafe<QueryOp>;
~QueryOp();

scoped_refptr<FileHandleHolder> file_handle_;
scoped_refptr<FileHolder> file_holder_;
base::File::Info file_info_;
};

// Class to perform file read operations across multiple threads.
class ReadOp : public base::RefCountedThreadSafe<ReadOp> {
public:
ReadOp(scoped_refptr<FileHandleHolder> file_handle,
ReadOp(scoped_refptr<FileHolder> file_holder,
int64_t offset,
int32_t bytes_to_read);

Expand All @@ -142,7 +143,7 @@ class PPAPI_PROXY_EXPORT FileIOResource
friend class base::RefCountedThreadSafe<ReadOp>;
~ReadOp();

scoped_refptr<FileHandleHolder> file_handle_;
scoped_refptr<FileHolder> file_holder_;
int64_t offset_;
int32_t bytes_to_read_;
scoped_ptr<char[]> buffer_;
Expand All @@ -151,7 +152,7 @@ class PPAPI_PROXY_EXPORT FileIOResource
// Class to perform file write operations across multiple threads.
class WriteOp : public base::RefCountedThreadSafe<WriteOp> {
public:
WriteOp(scoped_refptr<FileHandleHolder> file_handle,
WriteOp(scoped_refptr<FileHolder> file_holder,
int64_t offset,
scoped_ptr<char[]> buffer,
int32_t bytes_to_write,
Expand All @@ -165,7 +166,7 @@ class PPAPI_PROXY_EXPORT FileIOResource
friend class base::RefCountedThreadSafe<WriteOp>;
~WriteOp();

scoped_refptr<FileHandleHolder> file_handle_;
scoped_refptr<FileHolder> file_holder_;
int64_t offset_;
scoped_ptr<char[]> buffer_;
int32_t bytes_to_write_;
Expand Down Expand Up @@ -213,7 +214,7 @@ class PPAPI_PROXY_EXPORT FileIOResource
PP_FileHandle* output_handle,
const ResourceMessageReplyParams& params);

scoped_refptr<FileHandleHolder> file_handle_;
scoped_refptr<FileHolder> file_holder_;
PP_FileSystemType file_system_type_;
scoped_refptr<Resource> file_system_resource_;
FileIOStateManager state_manager_;
Expand Down
8 changes: 4 additions & 4 deletions ppapi/proxy/file_mapping_resource.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ int32_t FileMappingResource::Map(PP_Instance /* instance */,
return PP_ERROR_BADARGUMENT;
FileIOResource* file_io_resource =
static_cast<FileIOResource*>(enter.object());
scoped_refptr<FileIOResource::FileHandleHolder> file_handle =
file_io_resource->file_handle();
if (!FileIOResource::FileHandleHolder::IsValid(file_handle))
scoped_refptr<FileIOResource::FileHolder> file_holder =
file_io_resource->file_holder();
if (!FileIOResource::FileHolder::IsValid(file_holder))
return PP_ERROR_FAILED;
if (length < 0 || offset < 0 ||
!base::IsValueInRangeForNumericType<off_t>(offset)) {
Expand Down Expand Up @@ -75,7 +75,7 @@ int32_t FileMappingResource::Map(PP_Instance /* instance */,
return PP_ERROR_BADARGUMENT;

base::Callback<MapResult()> map_cb(
base::Bind(&FileMappingResource::DoMapBlocking, file_handle, *address,
base::Bind(&FileMappingResource::DoMapBlocking, file_holder, *address,
length, protection, flags, offset));
if (callback->is_blocking()) {
// The plugin could release its reference to this instance when we release
Expand Down
2 changes: 1 addition & 1 deletion ppapi/proxy/file_mapping_resource.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class PPAPI_PROXY_EXPORT FileMappingResource
// implementation is platform specific. See file_mapping_resource_posix.cc and
// file_mapping_resource_win.cc.
static MapResult DoMapBlocking(
scoped_refptr<FileIOResource::FileHandleHolder> handle,
scoped_refptr<FileIOResource::FileHolder> file_holder,
void* address_hint,
int64_t length,
uint32_t protection,
Expand Down
Loading

0 comments on commit 35b05b1

Please sign in to comment.