Implement ScopedFD in terms of ScopedGeneric.

Implement ScopedFD in terms of ScopedGeneric.

Move to a new file base/files/scoped_file.h. I will also add ScopedFILE to here (currently in file_util.h) later.

I think there is a crash in the old code in content/browser/zygote_host/zygote_host_impl_linux.cc that this patch should fix. The old ScopedFD took the address of something in a vector that is being modified.

I removed SafeScopedFD from content/common/sandbox_linux/sandbox_linux.cc since base's ScopedFD not CHECKs on close failure (this is a more recent addition).

BUG=
R=agl@chromium.org, viettrungluu@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257001

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257179

[brettw, 2014-03-08 00:53:34]

Please don't rubberstamp, I'm paranoid about breaking something.

[brettw, 2014-03-08 00:54:04]

Note: requires https://codereview.chromium.org/189613002/

[jln (very slow on Chromium), 2014-03-08 01:15:14]

Did you forget to include base/files/scoped_file.h in your CL?

Note: there is pattern of:

int fd = open(..)
{
ScopedFDCLoser(&fd);
}

if (fd > -1) { close(fd); }

i.e. ScopedFDCLoser did guarantee to switch back the file descriptor to -1.

So we'll need to be careful about this.

[viettrungluu, 2014-03-08 03:06:03]

https://codereview.chromium.org/191673003/diff/1/base/memory/discardable_memo...
File base/memory/discardable_memory_allocator_android.cc (right):

https://codereview.chromium.org/191673003/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_allocator_android.cc:70: if (!fd) {
So, it occurs to me that this is extremely confusing.

I think it'd be better for ScopedNullableGeneric to have is_valid/is_invalid
methods. (Though that throws into question the name "Nullable". D'oh.)

[agl, 2014-03-10 14:56:14]

LGTM for net/

[brettw, 2014-03-10 17:36:28]

New patch up with Testable removed, replaced with is_valid()

[jln (very slow on Chromium), 2014-03-10 17:51:02]

On 2014/03/10 17:36:28, brettw wrote:
> New patch up with Testable removed, replaced with is_valid()

Brett, did you to upload base/files/scoped_file.h? If not, could you update the
commit message? I couldn't find the
new class at quick glance.

[brettw, 2014-03-10 17:55:30]

Whoops, missing files added.

[viettrungluu, 2014-03-12 19:47:22]

https://codereview.chromium.org/191673003/diff/80001/base/memory/discardable_...
File base/memory/discardable_memory_allocator_android.cc (right):

https://codereview.chromium.org/191673003/diff/80001/base/memory/discardable_...
base/memory/discardable_memory_allocator_android.cc:91:
ignore_result(fd_closer.release());
This doesn't compile.

https://codereview.chromium.org/191673003/diff/80001/base/memory/shared_memor...
File base/memory/shared_memory_posix.cc (right):

https://codereview.chromium.org/191673003/diff/80001/base/memory/shared_memor...
base/memory/shared_memory_posix.cc:352: if (fp == NULL || readonly_fd.get() < 0)
return false;
The original check is somewhat dubious, and I'd do || !readonly_fd.is_valid()
here.

https://codereview.chromium.org/191673003/diff/80001/base/memory/shared_memor...
base/memory/shared_memory_posix.cc:380: readonly_mapped_file_ =
readonly_fd.release();
I imagine that at somepoint we should convert readonly_mapped_file_ and
mapped_file_ to ScopedFDs.

https://codereview.chromium.org/191673003/diff/80001/base/posix/unix_domain_s...
File base/posix/unix_domain_socket_linux_unittest.cc (right):

https://codereview.chromium.org/191673003/diff/80001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux_unittest.cc:29: base::ScopedFD
scoped_fd0(fds[0]);
base:: not needed (here and below).

https://codereview.chromium.org/191673003/diff/80001/base/test/launcher/test_...
File base/test/launcher/test_launcher.cc (right):

https://codereview.chromium.org/191673003/diff/80001/base/test/launcher/test_...
base/test/launcher/test_launcher.cc:269: output_file_fd.reset();
Could you fix the indentation here as a drive-by?

(Actually, it seems to me that you could do this unconditionally.)

(The Windows code also seems unusual to me -- wouldn't close imply flush?)

https://codereview.chromium.org/191673003/diff/80001/chrome/browser/extension...
File
chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc
(right):

https://codereview.chromium.org/191673003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc:221:
read_file = pipe_handles[0];
This stuff makes me wonder why we have ScopedFD at all rather than
ScopedPlatformFile (a PlatformFile being an FD on POSIX and a HANDLE on
Windows).

[brettw, 2014-03-12 20:45:29]

Review comments so far, added some advice on when to use.

https://codereview.chromium.org/191673003/diff/80001/base/test/launcher/test_...
File base/test/launcher/test_launcher.cc (right):

https://codereview.chromium.org/191673003/diff/80001/base/test/launcher/test_...
base/test/launcher/test_launcher.cc:269: output_file_fd.reset();
On 2014/03/12 19:47:23, viettrungluu wrote:
> Could you fix the indentation here as a drive-by?

Done

> (Actually, it seems to me that you could do this unconditionally.)

I don't know what you mean.

> (The Windows code also seems unusual to me -- wouldn't close imply flush?)

Flush means flush to disk. I don't think Close() implies this.

[viettrungluu, 2014-03-13 22:37:09]

On 2014/03/12 20:45:29, brettw wrote:
> Review comments so far, added some advice on when to use.
> 
>
https://codereview.chromium.org/191673003/diff/80001/base/test/launcher/test_...
> File base/test/launcher/test_launcher.cc (right):
> 
>
https://codereview.chromium.org/191673003/diff/80001/base/test/launcher/test_...
> base/test/launcher/test_launcher.cc:269: output_file_fd.reset();
> On 2014/03/12 19:47:23, viettrungluu wrote:
> > Could you fix the indentation here as a drive-by?
> 
> Done
> 
> > (Actually, it seems to me that you could do this unconditionally.)
> 
> I don't know what you mean.
> 
> > (The Windows code also seems unusual to me -- wouldn't close imply flush?)
> 
> Flush means flush to disk. I don't think Close() implies this.

Then why doesn't the POSIX version have an fsync?

[viettrungluu, 2014-03-13 22:37:19]

https://codereview.chromium.org/191673003/diff/150043/content/browser/zygote_...
File content/browser/zygote_host/zygote_host_impl_linux.cc (right):

https://codereview.chromium.org/191673003/diff/150043/content/browser/zygote_...
content/browser/zygote_host/zygote_host_impl_linux.cc:322:
std::vector<linked_ptr<base::ScopedFD> > autodelete_fds;
Side comment: I don't see why a ScopedVector isn't used instead.

https://codereview.chromium.org/191673003/diff/150043/ipc/ipc_channel_factory.cc
File ipc/ipc_channel_factory.cc (right):

https://codereview.chromium.org/191673003/diff/150043/ipc/ipc_channel_factory...
ipc/ipc_channel_factory.cc:56: if (new_fd < 0) {
Maybe move the scoped_fd to here, and do |if (!scoped_fd.is_valid())|?

https://codereview.chromium.org/191673003/diff/150043/ipc/ipc_channel_factory...
ipc/ipc_channel_factory.cc:65: if (!IsPeerAuthorized(new_fd))
And use scoped_fd.get() here.

https://codereview.chromium.org/191673003/diff/150043/net/test/spawned_test_s...
File net/test/spawned_test_server/local_test_server_posix.cc (right):

https://codereview.chromium.org/191673003/diff/150043/net/test/spawned_test_s...
net/test/spawned_test_server/local_test_server_posix.cc:151: base::ScopedFD
our_fd(child_fd_.release());
Why are you changing the name from "child" to "our"?

https://codereview.chromium.org/191673003/diff/150043/sandbox/linux/services/...
File sandbox/linux/services/scoped_process_unittest.cc (right):

https://codereview.chromium.org/191673003/diff/150043/sandbox/linux/services/...
sandbox/linux/services/scoped_process_unittest.cc:83: write_end_closer.reset();
Hrm, this changes "IGNORE_EINTR" to "HANDLE_EINTR". Dunno if that's actually
significant.

[viettrungluu, 2014-03-13 22:38:12]

Also, LGTM w/the noted things addressed/looked
at/considered/ignored/aborted/retried/failed.

[brettw, 2014-03-13 22:42:09]

https://codereview.chromium.org/191673003/diff/150043/net/test/spawned_test_s...
File net/test/spawned_test_server/local_test_server_posix.cc (right):

https://codereview.chromium.org/191673003/diff/150043/net/test/spawned_test_s...
net/test/spawned_test_server/local_test_server_posix.cc:151: base::ScopedFD
our_fd(child_fd_.release());
On 2014/03/13 22:37:20, viettrungluu wrote:
> Why are you changing the name from "child" to "our"?

I didn't want to have two variables named child_fd and child_fd_.

[brettw, 2014-03-14 05:21:26]

Committed patchset #11 manually as r257001.

[Nico, 2014-03-14 05:33:21]

A revert of this CL has been created in
https://codereview.chromium.org/197873014/ by thakis@chromium.org.

The reason for reverting is: Doesn't build on android:

FAILED: /mnt/data/b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/base/memory/base.discardable_memory_allocator_android.o.d
-DV8_DEPRECATION_WARNINGS -DBLINK_SCALE_FILTERS_AT_RECORD_TIME
-D_FILE_OFFSET_BITS=64 -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD
-DCOMPONENT_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1
-DUSE_PROPRIETARY_CODECS -DENABLE_CONFIGURATION_POLICY
-DENABLE_NEW_GAMEPAD_API=1 -DDISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DENABLE_EGLIMAGE=1
-DENABLE_AUTOFILL_DIALOG=1 -DCLD_VERSION=1 -DENABLE_PRINTING=1
-DENABLE_MANAGED_USERS=1 -DUSE_OPENSSL=1 -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -DBASE_IMPLEMENTATION -DANDROID -D__GNU_SOURCE=1
-DUSE_STLPORT=1 -D_STLP_USE_PTR_SPECIALIZATIONS=1 '-DCHROME_BUILD_ID=""'
-DHAVE_SYS_UIO_H -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1
-D_DEBUG -Igen/base
-I../../third_party/android_tools/ndk/sources/android/cpufeatures -I../..
-fstack-protector --param=ssp-buffer-size=4 -Werror -fno-exceptions
-fno-strict-aliasing -Wall -Wno-unused-parameter -Wno-missing-field-initializers
-fvisibility=hidden -pipe -fPIC -Wheader-hygiene -Wno-char-subscripts
-Wno-unneeded-internal-declaration -Wno-covered-switch-default
-Wstring-conversion -Wno-c++11-narrowing -Wno-reserved-user-defined-literal
-Wno-deprecated-register -Xclang -load -Xclang
/mnt/data/b/build/slave/Android_Clang_Builder__dbg_/build/src/tools/clang/scripts/../../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
-Xclang -add-plugin -Xclang find-bad-constructs -Xclang
-plugin-arg-find-bad-constructs -Xclang check-url-directory -fcolor-diagnostics
-Wexit-time-destructors -march=armv7-a -mfpu=vfpv3-d16 -mfloat-abi=softfp
-mthumb -no-integrated-as
-B/mnt/data/b/build/slave/Android_Clang_Builder__dbg_/build/src/third_party/android_tools/ndk//toolchains/arm-linux-androideabi-4.6/prebuilt/linux-x86_64/bin
-ffunction-sections -funwind-tables -g -fstack-protector -fno-short-enums
-Wa,--noexecstack -D__compiler_offsetof=__builtin_offsetof -Dnan=__builtin_nan
-target arm-linux-androideabi -mllvm -arm-enable-ehabi
--sysroot=/mnt/data/b/build/slave/Android_Clang_Builder__dbg_/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm
-I/mnt/data/b/build/slave/Android_Clang_Builder__dbg_/build/src/third_party/android_tools/ndk//sources/cxx-stl/stlport/stlport
-Os -g -fomit-frame-pointer -fdata-sections -ffunction-sections -funwind-tables
-g0 -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -Wsign-compare
-std=gnu++11 -Wno-implicit-exception-spec-mismatch -Wno-abi  -c
../../base/memory/discardable_memory_allocator_android.cc -o
obj/base/memory/base.discardable_memory_allocator_android.o
../../base/memory/discardable_memory_allocator_android.cc:84:25:error: no
matching function for call to 'mmap'
  void* const address = mmap(
                        ^~~~
/mnt/data/b/build/slave/Android_Clang_Builder__dbg_/build/src/third_party/android_tools/ndk//platforms/android-14/arch-arm/usr/include/sys/mman.h:47:15:
note: candidate function not viable: no known conversion from 'base::ScopedFD'
(aka 'ScopedGeneric<int, internal::ScopedFDCloseTraits>') to 'int' for 5th
argument
extern void*  mmap(void *, size_t, int, int, int, off_t);
              ^
1 error generated..

[brettw, 2014-03-14 19:37:00]

Committed patchset #13 manually as r257179.

[jochen (gone - plz use gerrit), 2014-03-15 13:33:57]

A revert of this CL has been created in
https://codereview.chromium.org/201203002/ by jochen@chromium.org.

The reason for reverting is: Doesn't correctly link

/mnt/data/b/build/slave/Chromium_Linux_Codesearch/build/src/third_party/gold/gold64:
warning: hidden symbol 'base::internal::ScopedFDCloseTraits::Free(int)' in
obj/base/files/nacl_helper.scoped_file.o is referenced by DSO lib/libipc.so.

[agl, 2014-03-18 06:52:04]

LGTM

https://codereview.chromium.org/191673003/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/automatic_reboot_manager.cc (right):

https://codereview.chromium.org/191673003/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/system/automatic_reboot_manager.cc:68: while ((length =
read(fd.get(), buffer, sizeof(buffer))) > 0)
This read should be wrapped in HANDLE_EINTR.

https://codereview.chromium.org/191673003/diff/220001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome_launcher.cc (right):

https://codereview.chromium.org/191673003/diff/220001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome_launcher.cc:258: devnull.reset(open("/dev/null",
O_WRONLY));
this open should be wrapped in HANDLE_EINTR.

https://codereview.chromium.org/191673003/diff/220001/content/common/sandbox_...
File content/common/sandbox_mac_system_access_unittest.mm (right):

https://codereview.chromium.org/191673003/diff/220001/content/common/sandbox_...
content/common/sandbox_mac_system_access_unittest.mm:90: base::ScopedFD
fdes(open("/etc/passwd", O_RDONLY));
This, line 115 and line 108 should also have HANDLE_EINTR.

https://codereview.chromium.org/191673003/diff/220001/sandbox/linux/services/...
File sandbox/linux/services/yama.cc (right):

https://codereview.chromium.org/191673003/diff/220001/sandbox/linux/services/...
sandbox/linux/services/yama.cc:82: base::ScopedFD
yama_scope(open(kPtraceScopePath, O_RDONLY));
HANDLE_EINTR

https://codereview.chromium.org/191673003/diff/220001/sandbox/linux/services/...
sandbox/linux/services/yama.cc:92: ssize_t num_read = read(yama_scope.get(),
&yama_scope_value, 1);
HANDLE_EINTR

https://codereview.chromium.org/191673003/diff/220001/tools/android/memdump/m...
File tools/android/memdump/memdump.cc (right):

https://codereview.chromium.org/191673003/diff/220001/tools/android/memdump/m...
tools/android/memdump/memdump.cc:440: base::ScopedFD pagemap_fd(open(
HANDLE_EINTR

https://codereview.chromium.org/191673003/diff/220001/tools/android/memdump/m...
tools/android/memdump/memdump.cc:492: base::ScopedFD
page_count_fd(open("/proc/kpagecount", O_RDONLY));
HANDLE_EINTR and on line 493.

[brettw, 2014-03-18 20:21:53]

Thanks, HANDLE_EINTR suggestions implemented on
https://codereview.chromium.org/203213005/