No dbus::FileDescriptor in DebugDaemonClient

No dbus::FileDescriptor in DebugDaemonClient

This is the first step to replace dbus::FileDescriptor with base::ScopedFD.

Add a new variant of dbus::MessageWriter::AppendFileDescriptor and dbus::MessageReader::PopFileDescriptor.
Use the new variant in DebugDaemonClient.

BUG=621841
Committed: https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229
Cr-Commit-Position: refs/heads/master@{#414093}

[hashimoto, 2016-06-21 11:04:33]

Description was changed from

==========
No dbus::FileDescriptor in debug_daemon_client

BUG=
==========

to

==========
No dbus::FileDescriptor in debug_daemon_client

BUG=621841
==========

[hashimoto, 2016-08-22 05:01:03]

Description was changed from

==========
No dbus::FileDescriptor in debug_daemon_client

BUG=621841
==========

to

==========
No dbus::FileDescriptor in DebugDaemonClient

This is the first step to replace dbus::FileDescriptor with base::ScopedFD.

Add a new variant of dbus::MessageWriter::AppendFileDescriptor and
dbus::MessageReader::PopFileDescriptor.
Use the new variant in DebugDaemonClient.

BUG=621841
==========

[hashimoto, 2016-08-22 05:11:33]

The CQ bit was checked by hashimoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 05:11:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 06:12:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 06:12:41]

Dry run: This issue passed the CQ dry run.

[hashimoto, 2016-08-22 06:15:19]

hashimoto@chromium.org changed reviewers:
+ satorux@chromium.org, stevenjb@chromium.org

[stevenjb, 2016-08-22 16:04:38]

Thanks for working on this. This lgtm, but I'm really not an expert on file
descriptors, hopefully satourx@ will be able to provide a better review.

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/system_logs/debug_log_writer.cc (right):

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/debug_log_writer.cc:231: auto* file_ptr =
file.get();
nit: base::File* is slightly more clear here.

[satorux1, 2016-08-23 04:43:37]

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/system_logs/debug_log_writer.cc (right):

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/debug_log_writer.cc:206: void
IntializeLogFile(base::File* file,
-> InitializeLogFile. Please fix the typo while you are at it. :)

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_output.cc (right):

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_output.cc:35: }
|pipe_write_end| is deleted here hence closed? is it ok to close the file
descriptor on this thread?

https://codereview.chromium.org/2081153002/diff/60001/chromeos/dbus/debug_dae...
File chromeos/dbus/debug_daemon_client.h (right):

https://codereview.chromium.org/2081153002/diff/60001/chromeos/dbus/debug_dae...
chromeos/dbus/debug_daemon_client.h:39: int file_descriptor,
How about taking ownership of the file descriptor as base::File? I thought
that'd make ownership clearer.

https://codereview.chromium.org/2081153002/diff/60001/chromeos/dbus/debug_dae...
chromeos/dbus/debug_daemon_client.h:103: int file_descriptor,
ditto. maybe take ownership?

[hashimoto, 2016-08-23 06:38:42]

Patchset #5 (id:80001) has been deleted

[hashimoto, 2016-08-23 06:38:50]

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/system_logs/debug_log_writer.cc (right):

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/debug_log_writer.cc:206: void
IntializeLogFile(base::File* file,
On 2016/08/23 04:43:37, satorux1 wrote:
> -> InitializeLogFile. Please fix the typo while you are at it. :)

Done.

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/system_logs/debug_log_writer.cc:231: auto* file_ptr =
file.get();
On 2016/08/22 16:04:38, stevenjb wrote:
> nit: base::File* is slightly more clear here.

Done.

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_output.cc (right):

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_output.cc:35: }
On 2016/08/23 04:43:37, satorux1 wrote:
> |pipe_write_end| is deleted here hence closed? is it ok to close the file
> descriptor on this thread?

Closing a pipe FD doesn't result in disk IO so it should not take longer than a
typical syscall.

https://codereview.chromium.org/2081153002/diff/60001/chromeos/dbus/debug_dae...
File chromeos/dbus/debug_daemon_client.h (right):

https://codereview.chromium.org/2081153002/diff/60001/chromeos/dbus/debug_dae...
chromeos/dbus/debug_daemon_client.h:39: int file_descriptor,
On 2016/08/23 04:43:37, satorux1 wrote:
> How about taking ownership of the file descriptor as base::File? I thought
> that'd make ownership clearer.
Here we want to avoid owning the given FD because there is no need to do it [1]
and it can result in UI jank [2].

1. There is no need to own the FD because appending a FD to a D-Bus message
doesn't require ownership as dbus_message_iter_append_basic() duplicates the FD.

2. The user code (debug_log_writer.cc) passes a FD of a regular file which can
cause UI jank when being closed on the UI thread, instead of a pipe FD, to this
method. From DebugDaemonClient's point of view, there is no way to know if it's
OK to close the given FD on the UI thread.

https://codereview.chromium.org/2081153002/diff/60001/chromeos/dbus/debug_dae...
chromeos/dbus/debug_daemon_client.h:103: int file_descriptor,
On 2016/08/23 04:43:37, satorux1 wrote:
> ditto. maybe take ownership?

Acknowledged.

[hashimoto, 2016-08-23 06:38:55]

The CQ bit was checked by hashimoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 06:39:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2016-08-23 07:02:02]

LGTM with some comments

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_output.cc (right):

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_output.cc:35: }
On 2016/08/23 06:38:50, hashimoto wrote:
> On 2016/08/23 04:43:37, satorux1 wrote:
> > |pipe_write_end| is deleted here hence closed? is it ok to close the file
> > descriptor on this thread?
> 
> Closing a pipe FD doesn't result in disk IO so it should not take longer than
a
> typical syscall.

Ah OK. I thought it'd be caught by AssertIOAllowed() but it didn't seem to be
the case.

https://codereview.chromium.org/2081153002/diff/60001/chromeos/dbus/debug_dae...
File chromeos/dbus/debug_daemon_client.h (right):

https://codereview.chromium.org/2081153002/diff/60001/chromeos/dbus/debug_dae...
chromeos/dbus/debug_daemon_client.h:39: int file_descriptor,
Makes sense. In that case, could you add a comment that says this function does
not close |file_descriptor|?

https://codereview.chromium.org/2081153002/diff/60001/chromeos/dbus/debug_dae...
chromeos/dbus/debug_daemon_client.h:103: int file_descriptor,
On 2016/08/23 06:38:50, hashimoto wrote:
> On 2016/08/23 04:43:37, satorux1 wrote:
> > ditto. maybe take ownership?
> 
> Acknowledged.

ditto. a comment about file_descriptor ownership would be helpful here too.

https://codereview.chromium.org/2081153002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/log_private/log_private_apitest_chromeos.cc
(right):

https://codereview.chromium.org/2081153002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/log_private/log_private_apitest_chromeos.cc:37:
base::File* file_param = new base::File(dup(file_descriptor));
I think it's not so obvious why dup() is needed here, maybe add something like:

// dup() is needed as the file descriptor will be closed on the client side.

[commit-bot: I haz the power, 2016-08-23 07:31:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 07:31:46]

Dry run: This issue passed the CQ dry run.

[hashimoto, 2016-08-23 07:41:28]

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_output.cc (right):

https://codereview.chromium.org/2081153002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_output.cc:35: }
On 2016/08/23 07:02:02, satorux1 wrote:
> On 2016/08/23 06:38:50, hashimoto wrote:
> > On 2016/08/23 04:43:37, satorux1 wrote:
> > > |pipe_write_end| is deleted here hence closed? is it ok to close the file
> > > descriptor on this thread?
> > 
> > Closing a pipe FD doesn't result in disk IO so it should not take longer
than
> a
> > typical syscall.
> 
> Ah OK. I thought it'd be caught by AssertIOAllowed() but it didn't seem to be
> the case.

Good point.
This actually would cause a DCHECK failure if someone tries to enable DCHECK on
a device.

Changed PipeReader::StartIO to return ScopedFD which doesn't have
AssertIOAllowed in its dtor.

https://codereview.chromium.org/2081153002/diff/60001/chromeos/dbus/debug_dae...
File chromeos/dbus/debug_daemon_client.h (right):

https://codereview.chromium.org/2081153002/diff/60001/chromeos/dbus/debug_dae...
chromeos/dbus/debug_daemon_client.h:39: int file_descriptor,
On 2016/08/23 07:02:02, satorux1 wrote:
> Makes sense. In that case, could you add a comment that says this function
does
> not close |file_descriptor|?

Added a comment about FD duplication.

https://codereview.chromium.org/2081153002/diff/60001/chromeos/dbus/debug_dae...
chromeos/dbus/debug_daemon_client.h:103: int file_descriptor,
On 2016/08/23 07:02:02, satorux1 wrote:
> On 2016/08/23 06:38:50, hashimoto wrote:
> > On 2016/08/23 04:43:37, satorux1 wrote:
> > > ditto. maybe take ownership?
> > 
> > Acknowledged.
> 
> ditto. a comment about file_descriptor ownership would be helpful here too.

Done.

https://codereview.chromium.org/2081153002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/log_private/log_private_apitest_chromeos.cc
(right):

https://codereview.chromium.org/2081153002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/log_private/log_private_apitest_chromeos.cc:37:
base::File* file_param = new base::File(dup(file_descriptor));
On 2016/08/23 07:02:02, satorux1 wrote:
> I think it's not so obvious why dup() is needed here, maybe add something
like:
> 
> // dup() is needed as the file descriptor will be closed on the client side.

Done.

[hashimoto, 2016-08-23 07:41:32]

The CQ bit was checked by hashimoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 07:42:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 08:37:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 08:37:19]

Dry run: This issue passed the CQ dry run.

[hashimoto, 2016-08-23 09:46:41]

hashimoto@chromium.org changed reviewers:
+ isherman@chromium.org, rockot@chromium.org

[hashimoto, 2016-08-23 09:46:42]

isherman@: Please review chrome/browser/metrics/perf/
rockot@: Please review
chrome/browser/extensions/api/log_private/log_private_apitest_chromeos.cc

[Ken Rockot(use gerrit already), 2016-08-23 16:03:00]

lgtm

[Ilya Sherman, 2016-08-23 19:40:39]

+Simon as FYI

https://codereview.chromium.org/2081153002/diff/120001/chrome/browser/metrics...
File chrome/browser/metrics/perf/perf_output.cc (right):

https://codereview.chromium.org/2081153002/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/perf/perf_output.cc:27: base::ScopedFD pipe_write_end =
perf_data_pipe_reader_->StartIO();
FWIW, the only reason that I know what "FD" means in this context is because the
CL is replacing something that was named "FileDescriptor".  IMO it would be
great to rename "ScopedFD" for greater clarity.  (Not intended to block this CL,
just food for thought.)

https://codereview.chromium.org/2081153002/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/perf/perf_output.cc:28: 
Optional nit: I'd omit this newline.

https://codereview.chromium.org/2081153002/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/perf/perf_output.cc:31: 
Ditto

[Ilya Sherman, 2016-08-23 19:41:00]

Sorry, sent to fast.  Meant to say: LGTM % comments/nits.

[hashimoto, 2016-08-24 15:59:11]

https://codereview.chromium.org/2081153002/diff/120001/chrome/browser/metrics...
File chrome/browser/metrics/perf/perf_output.cc (right):

https://codereview.chromium.org/2081153002/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/perf/perf_output.cc:27: base::ScopedFD pipe_write_end =
perf_data_pipe_reader_->StartIO();
On 2016/08/23 19:40:38, Ilya Sherman wrote:
> FWIW, the only reason that I know what "FD" means in this context is because
the
> CL is replacing something that was named "FileDescriptor".  IMO it would be
> great to rename "ScopedFD" for greater clarity.  (Not intended to block this
CL,
> just food for thought.)
It was unfortunate that ScopedFD was introduced with the current name 7 years
ago with almost no discussion in https://codereview.chromium.org/496007.
While I agree it's generally better to avoid unnecessary abbreviation, renaming
ScopedFD to, say, ScopedFileDescriptor would require some effort ("git grep
ScopedFD | wc -l" gives 516).
It's not impossible, but the cost will be a bit too large compared to the merit
IMHO.

BTW, the term "FD" frequently appears in man pages (e.g. man open) and other
POSIX documentations, so it might be acceptable to use this abbreviation as long
as it's in platform-specific code where the reader is expected to be familiar
with POSIX terms.

https://codereview.chromium.org/2081153002/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/perf/perf_output.cc:28: 
On 2016/08/23 19:40:38, Ilya Sherman wrote:
> Optional nit: I'd omit this newline.

Done.

https://codereview.chromium.org/2081153002/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/perf/perf_output.cc:31: 
On 2016/08/23 19:40:38, Ilya Sherman wrote:
> Ditto

Done.

[hashimoto, 2016-08-24 15:59:42]

The CQ bit was checked by hashimoto@chromium.org

[hashimoto, 2016-08-24 15:59:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, satorux@chromium.org, rockot@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2081153002/#ps140001
(title: "Address comments")

[commit-bot: I haz the power, 2016-08-24 16:00:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 16:51:53]

Description was changed from

==========
No dbus::FileDescriptor in DebugDaemonClient

This is the first step to replace dbus::FileDescriptor with base::ScopedFD.

Add a new variant of dbus::MessageWriter::AppendFileDescriptor and
dbus::MessageReader::PopFileDescriptor.
Use the new variant in DebugDaemonClient.

BUG=621841
==========

to

==========
No dbus::FileDescriptor in DebugDaemonClient

This is the first step to replace dbus::FileDescriptor with base::ScopedFD.

Add a new variant of dbus::MessageWriter::AppendFileDescriptor and
dbus::MessageReader::PopFileDescriptor.
Use the new variant in DebugDaemonClient.

BUG=621841
==========

[commit-bot: I haz the power, 2016-08-24 16:51:55]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-08-24 16:53:56]

Description was changed from

==========
No dbus::FileDescriptor in DebugDaemonClient

This is the first step to replace dbus::FileDescriptor with base::ScopedFD.

Add a new variant of dbus::MessageWriter::AppendFileDescriptor and
dbus::MessageReader::PopFileDescriptor.
Use the new variant in DebugDaemonClient.

BUG=621841
==========

to

==========
No dbus::FileDescriptor in DebugDaemonClient

This is the first step to replace dbus::FileDescriptor with base::ScopedFD.

Add a new variant of dbus::MessageWriter::AppendFileDescriptor and
dbus::MessageReader::PopFileDescriptor.
Use the new variant in DebugDaemonClient.

BUG=621841

Committed: https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229
Cr-Commit-Position: refs/heads/master@{#414093}
==========

[commit-bot: I haz the power, 2016-08-24 16:53:58]

Patchset 7 (id:??) landed as
https://crrev.com/89822579003fc4f5128c58f9bbd1a22b347ea229
Cr-Commit-Position: refs/heads/master@{#414093}