Commit f7302eb7 authored by morrita's avatar morrita Committed by Commit bot

Android: Get rid of extra dup()s on launching child processes

This is a regression from [1] which added a dup() call on
the renderer launching process. This CL removes these calls
by reveiling the subtle ownership of the file descriptors.

The original intention here was to completely hide and simplify
the notion of the ownership, at the cost of a few dup() calls.
However, the crash report on the reported bug indicates that
the dup() can fail and it lets the renderer initialization fail,
probably due to some per-process limit of the number of opened files.

[1] https://codereview.chromium.org/585203002

R=mdempsky@chromium.org, jam@chromium.org
BUG=455364

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

Cr-Commit-Position: refs/heads/master@{#315353}
parent aba6a96e
...@@ -128,14 +128,12 @@ void StartChildProcess( ...@@ -128,14 +128,12 @@ void StartChildProcess(
env->GetBooleanArrayElements(j_file_auto_close.obj(), NULL); env->GetBooleanArrayElements(j_file_auto_close.obj(), NULL);
base::android::CheckException(env); base::android::CheckException(env);
for (size_t i = 0; i < file_count; ++i) { for (size_t i = 0; i < file_count; ++i) {
// Owners of passed descriptors can outlive this function and we don't know
// when it is safe to close() them. So we pass dup()-ed FD and
// let ChildProcessLauncher in java take care of their lifetimes.
// TODO(morrita): Drop FileDescriptorInfo.mAutoClose on Java side.
file_auto_close[i] = true; // This indicates ownership transfer.
file_ids[i] = files_to_register->GetIDAt(i); file_ids[i] = files_to_register->GetIDAt(i);
file_fds[i] = dup(files_to_register->GetFDAt(i)); file_fds[i] = files_to_register->GetFDAt(i);
PCHECK(0 <= file_fds[i]); PCHECK(0 <= file_fds[i]);
file_auto_close[i] = files_to_register->OwnsFD(file_fds[i]);
if (file_auto_close[i])
ignore_result(files_to_register->ReleaseFD(file_fds[i]).release());
} }
env->ReleaseIntArrayElements(j_file_ids.obj(), file_ids, 0); env->ReleaseIntArrayElements(j_file_ids.obj(), file_ids, 0);
env->ReleaseIntArrayElements(j_file_fds.obj(), file_fds, 0); env->ReleaseIntArrayElements(j_file_fds.obj(), file_fds, 0);
......
...@@ -47,6 +47,27 @@ bool FileDescriptorInfoImpl::HasID(int id) const { ...@@ -47,6 +47,27 @@ bool FileDescriptorInfoImpl::HasID(int id) const {
return false; return false;
} }
bool FileDescriptorInfoImpl::OwnsFD(base::PlatformFile file) const {
return owned_descriptors_.end() !=
std::find_if(
owned_descriptors_.begin(), owned_descriptors_.end(),
[file](const base::ScopedFD* fd) { return fd->get() == file; });
}
base::ScopedFD FileDescriptorInfoImpl::ReleaseFD(base::PlatformFile file) {
DCHECK(OwnsFD(file));
base::ScopedFD fd;
auto found = std::find_if(
owned_descriptors_.begin(), owned_descriptors_.end(),
[file](const base::ScopedFD* fd) { return fd->get() == file; });
(*found)->swap(fd);
owned_descriptors_.erase(found);
return fd.Pass();
}
void FileDescriptorInfoImpl::AddToMapping(int id, base::PlatformFile fd) { void FileDescriptorInfoImpl::AddToMapping(int id, base::PlatformFile fd) {
DCHECK(!HasID(id)); DCHECK(!HasID(id));
mapping_.push_back(std::make_pair(fd, id)); mapping_.push_back(std::make_pair(fd, id));
......
...@@ -25,6 +25,8 @@ class FileDescriptorInfoImpl : public FileDescriptorInfo { ...@@ -25,6 +25,8 @@ class FileDescriptorInfoImpl : public FileDescriptorInfo {
base::PlatformFile GetFDAt(size_t i) const override; base::PlatformFile GetFDAt(size_t i) const override;
int GetIDAt(size_t i) const override; int GetIDAt(size_t i) const override;
size_t GetMappingSize() const override; size_t GetMappingSize() const override;
bool OwnsFD(base::PlatformFile file) const override;
base::ScopedFD ReleaseFD(base::PlatformFile file) override;
private: private:
FileDescriptorInfoImpl(); FileDescriptorInfoImpl();
......
...@@ -41,6 +41,11 @@ class FileDescriptorInfo { ...@@ -41,6 +41,11 @@ class FileDescriptorInfo {
virtual base::PlatformFile GetFDAt(size_t i) const = 0; virtual base::PlatformFile GetFDAt(size_t i) const = 0;
virtual int GetIDAt(size_t i) const = 0; virtual int GetIDAt(size_t i) const = 0;
virtual size_t GetMappingSize() const = 0; virtual size_t GetMappingSize() const = 0;
// True if |this| has an ownership of |file|.
virtual bool OwnsFD(base::PlatformFile file) const = 0;
// Assuming |OwnsFD(file)|, release the ownership.
virtual base::ScopedFD ReleaseFD(base::PlatformFile file) = 0;
}; };
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment