Pass file Regions along with FDs to child processes on Android

Pass file Regions along with FDs to child processes on Android

This is just sending kWholeFile for every FD atm, but will be used in
the future to load v8 snapshots, icu data, and .pak files from the APK
without needing to extract them first.

BUG=394502
Committed: https://crrev.com/0f9183135b5292ee3ee7bf9f496cb7754a3d4c7a
Cr-Commit-Position: refs/heads/master@{#332608}

[agrieve, 2015-05-28 17:26:23]

https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
File base/files/memory_mapped_file.h (right):

https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
base/files/memory_mapped_file.h:31: Region();
Note: Need this so Region can be used in std::Map, and so it's less annoying to
have one as a member of a class. 

There is no place where kWholeFile has its address taken, so there is no need
for it to be the unique (0,0) instance. Having no default ctor also doesn't
ensure uniqueness since |offset| and |size| are public non-const.

[agrieve, 2015-05-28 18:28:34]

agrieve@chromium.org changed reviewers:
+ jochen@chromium.org, thestig@chromium.org, yfriedman@chromium.org

[agrieve, 2015-05-28 18:28:34]

thestig@chromium.org: Please review changes in 
base/files

jochen@chromium.org, yfriedman@chromium.org: Please review changes in everywhere

[Lei Zhang, 2015-05-28 18:55:54]

+primiano FYI since he wrote/modified a big part of
base/files/memory_mapped_file.cc.

https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
File base/files/memory_mapped_file.cc (left):

https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
base/files/memory_mapped_file.cc:21: DCHECK_GE(offset, 0);
why remove the DCHECKs?

https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
File base/files/memory_mapped_file.cc (right):

https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
base/files/memory_mapped_file.cc:19: MemoryMappedFile::Region::Region() :
offset(0), size(0) {
Perhaps this one should be (-1, -1) to indicate invalidness.

[agrieve, 2015-05-28 19:13:24]

https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
File base/files/memory_mapped_file.cc (left):

https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
base/files/memory_mapped_file.cc:21: DCHECK_GE(offset, 0);
On 2015/05/28 18:55:53, Lei Zhang wrote:
> why remove the DCHECKs?

There are times (like when going through jni) where it's nice to be able to
convert to two int64s and back again. These DCHECKs prevent doing so for
kWholeFile.

https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
File base/files/memory_mapped_file.cc (right):

https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
base/files/memory_mapped_file.cc:19: MemoryMappedFile::Region::Region() :
offset(0), size(0) {
On 2015/05/28 18:55:53, Lei Zhang wrote:
> Perhaps this one should be (-1, -1) to indicate invalidness.

sgtm (done)

[Primiano Tucci (use gerrit), 2015-05-29 10:44:34]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-05-29 10:44:34]

primiano@chromium.org changed reviewers:
+ primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-05-29 10:44:34]

https://codereview.chromium.org/1156183003/diff/20001/base/files/memory_mappe...
File base/files/memory_mapped_file.cc (right):

https://codereview.chromium.org/1156183003/diff/20001/base/files/memory_mappe...
base/files/memory_mapped_file.cc:20: }
Not sure I get what is the use case for the empty ctor here. Is it different
from kWholeFile? Why kWholeFile is (0,0) and the default ctor is (-1,-1).
If this is just to get this into a map, isn't just a matter of having explicit
copy and assignment ctors?

https://codereview.chromium.org/1156183003/diff/20001/base/files/memory_mappe...
File base/files/memory_mapped_file.h (right):

https://codereview.chromium.org/1156183003/diff/20001/base/files/memory_mappe...
base/files/memory_mapped_file.h:43: // This ctor is used only by kWholeFile, to
construct kWholeFile.
This comment is a bit tautological now :) maybe remove the "to construct
kWholeFile"

[Primiano Tucci (use gerrit), 2015-05-29 10:44:34]

https://codereview.chromium.org/1156183003/diff/20001/base/files/memory_mappe...
File base/files/memory_mapped_file.cc (right):

https://codereview.chromium.org/1156183003/diff/20001/base/files/memory_mappe...
base/files/memory_mapped_file.cc:20: }
Not sure I get what is the use case for the empty ctor here. Is it different
from kWholeFile? Why kWholeFile is (0,0) and the default ctor is (-1,-1).
If this is just to get this into a map, isn't just a matter of having explicit
copy and assignment ctors?

https://codereview.chromium.org/1156183003/diff/20001/base/files/memory_mappe...
File base/files/memory_mapped_file.h (right):

https://codereview.chromium.org/1156183003/diff/20001/base/files/memory_mappe...
base/files/memory_mapped_file.h:43: // This ctor is used only by kWholeFile, to
construct kWholeFile.
This comment is a bit tautological now :) maybe remove the "to construct
kWholeFile"

[Yaron, 2015-05-29 18:29:22]

https://codereview.chromium.org/1156183003/diff/20001/content/browser/android...
File content/browser/android/child_process_launcher_android.cc (right):

https://codereview.chromium.org/1156183003/diff/20001/content/browser/android...
content/browser/android/child_process_launcher_android.cc:127: bool autoClose =
files_to_register->OwnsFD(fd);
s/autoClose/auto_close/

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:59:
// Pairs IDs and file descriptors that should be registered natively.
nit: "File descriptors that should be registered natively."

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:94:
System.arraycopy(fdInfosAsParcelable, 0, mFdInfos, 0,
fdInfosAsParcelable.length);
Can you add a comment with reference to the stack overflow article. This
definitely confused me

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java
(right):

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:321:

nit: no ws

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:489:
static FileDescriptorInfo makeFdInfo(
private

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:518:
static void start(Context context, final String[] commandLine, int
childProcessId,
private

[Yaron, 2015-05-29 18:30:14]

On 2015/05/29 18:29:22, Yaron wrote:
>
https://codereview.chromium.org/1156183003/diff/20001/content/browser/android...
> File content/browser/android/child_process_launcher_android.cc (right):
> 
>
https://codereview.chromium.org/1156183003/diff/20001/content/browser/android...
> content/browser/android/child_process_launcher_android.cc:127: bool autoClose
=
> files_to_register->OwnsFD(fd);
> s/autoClose/auto_close/
> 
>
https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
> (right):
> 
>
https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:59:
> // Pairs IDs and file descriptors that should be registered natively.
> nit: "File descriptors that should be registered natively."
> 
>
https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:94:
> System.arraycopy(fdInfosAsParcelable, 0, mFdInfos, 0,
> fdInfosAsParcelable.length);
> Can you add a comment with reference to the stack overflow article. This
> definitely confused me
> 
>
https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java
> (right):
> 
>
https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:321:
> 
> nit: no ws
> 
>
https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
> (right):
> 
>
https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:489:
> static FileDescriptorInfo makeFdInfo(
> private
> 
>
https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:518:
> static void start(Context context, final String[] commandLine, int
> childProcessId,
> private

err lgtm after that

[agrieve, 2015-05-29 19:16:40]

https://codereview.chromium.org/1156183003/diff/20001/base/files/memory_mappe...
File base/files/memory_mapped_file.cc (right):

https://codereview.chromium.org/1156183003/diff/20001/base/files/memory_mappe...
base/files/memory_mapped_file.cc:20: }
On 2015/05/29 10:44:34, Primiano Tucci wrote:
> Not sure I get what is the use case for the empty ctor here. Is it different
> from kWholeFile? Why kWholeFile is (0,0) and the default ctor is (-1,-1).
> If this is just to get this into a map, isn't just a matter of having explicit
> copy and assignment ctors?

There's two parts to this:
1. Why have a default constructor at all?
- I think we could continue to get away without one, but since this is a struct
that is now being used as an out parameter, it's quite annoying for it to not
have a default constructor. Unless there's a compelling reason to not have one,
I think it should.

2. What should the default values be:
-There's two decent options here: kWholeFile, or -1, -1 (invalid values). 
-I think either are reasonable, and I don't see a strong case for one over the
other (although I do slightly favour invalid values).

https://codereview.chromium.org/1156183003/diff/20001/base/files/memory_mappe...
File base/files/memory_mapped_file.h (right):

https://codereview.chromium.org/1156183003/diff/20001/base/files/memory_mappe...
base/files/memory_mapped_file.h:43: // This ctor is used only by kWholeFile, to
construct kWholeFile.
On 2015/05/29 10:44:34, Primiano Tucci wrote:
> This comment is a bit tautological now :) maybe remove the "to construct
> kWholeFile"

lol, done

https://codereview.chromium.org/1156183003/diff/20001/content/browser/android...
File content/browser/android/child_process_launcher_android.cc (right):

https://codereview.chromium.org/1156183003/diff/20001/content/browser/android...
content/browser/android/child_process_launcher_android.cc:127: bool autoClose =
files_to_register->OwnsFD(fd);
On 2015/05/29 18:29:22, Yaron wrote:
> s/autoClose/auto_close/

Done.

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:59:
// Pairs IDs and file descriptors that should be registered natively.
On 2015/05/29 18:29:22, Yaron wrote:
> nit: "File descriptors that should be registered natively."

Done.

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:94:
System.arraycopy(fdInfosAsParcelable, 0, mFdInfos, 0,
fdInfosAsParcelable.length);
On 2015/05/29 18:29:22, Yaron wrote:
> Can you add a comment with reference to the stack overflow article. This
> definitely confused me

Done.

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java
(right):

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java:321:

On 2015/05/29 18:29:22, Yaron wrote:
> nit: no ws

Done.

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:489:
static FileDescriptorInfo makeFdInfo(
On 2015/05/29 18:29:22, Yaron wrote:
> private

Done.

https://codereview.chromium.org/1156183003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:518:
static void start(Context context, final String[] commandLine, int
childProcessId,
On 2015/05/29 18:29:22, Yaron wrote:
> private

Done.

[Lei Zhang, 2015-05-29 21:14:10]

https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
File base/files/memory_mapped_file.cc (left):

https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
base/files/memory_mapped_file.cc:21: DCHECK_GE(offset, 0);
On 2015/05/28 19:13:24, agrieve wrote:
> On 2015/05/28 18:55:53, Lei Zhang wrote:
> > why remove the DCHECKs?
> 
> There are times (like when going through jni) where it's nice to be able to
> convert to two int64s and back again. These DCHECKs prevent doing so for
> kWholeFile.

Assuming we stick with -1 to indicate invalidness, and you would only want to
reconstruct valid Regions from JNI, how about:

if (size == 0) {
  DCHECK_EQ(0, offset);
} else {
  DCHECK_GE(offset, 0);
  DCHECK_GT(size, 0);
}

[Primiano Tucci (use gerrit), 2015-06-01 09:14:53]

https://codereview.chromium.org/1156183003/diff/20001/base/files/memory_mappe...
File base/files/memory_mapped_file.cc (right):

https://codereview.chromium.org/1156183003/diff/20001/base/files/memory_mappe...
base/files/memory_mapped_file.cc:20: }
> There's two parts to this:
> 1. Why have a default constructor at all?
> - I think we could continue to get away without one, but since this is a
struct
> that is now being used as an out parameter, it's quite annoying for it to not
> have a default constructor. Unless there's a compelling reason to not have
one,
> I think it should.
Oh, I see the point, makes sense.

> 2. What should the default values be:
> -There's two decent options here: kWholeFile, or -1, -1 (invalid values). 
> -I think either are reasonable, and I don't see a strong case for one over the
> other (although I do slightly favour invalid values).
SGTM.

[agrieve, 2015-06-01 14:50:06]

https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
File base/files/memory_mapped_file.cc (left):

https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
base/files/memory_mapped_file.cc:21: DCHECK_GE(offset, 0);
On 2015/05/29 21:14:09, Lei Zhang wrote:
> On 2015/05/28 19:13:24, agrieve wrote:
> > On 2015/05/28 18:55:53, Lei Zhang wrote:
> > > why remove the DCHECKs?
> > 
> > There are times (like when going through jni) where it's nice to be able to
> > convert to two int64s and back again. These DCHECKs prevent doing so for
> > kWholeFile.
> 
> Assuming we stick with -1 to indicate invalidness, and you would only want to
> reconstruct valid Regions from JNI, how about:
> 
> if (size == 0) {
>   DCHECK_EQ(0, offset);
> } else {
>   DCHECK_GE(offset, 0);
>   DCHECK_GT(size, 0);
> }

This wouldn't allow you to convert an invalid region (-1,-1) to size/offset and
back to a region (as is done in JNI).

How about I move this check to Initialize (which I've gone ahead and done)? This
way we still have the same check where it counts, and Region gets to behave more
like a POD.

[agrieve, 2015-06-02 03:34:29]

On 2015/06/01 14:50:06, agrieve wrote:
>
https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
> File base/files/memory_mapped_file.cc (left):
> 
>
https://codereview.chromium.org/1156183003/diff/1/base/files/memory_mapped_fi...
> base/files/memory_mapped_file.cc:21: DCHECK_GE(offset, 0);
> On 2015/05/29 21:14:09, Lei Zhang wrote:
> > On 2015/05/28 19:13:24, agrieve wrote:
> > > On 2015/05/28 18:55:53, Lei Zhang wrote:
> > > > why remove the DCHECKs?
> > > 
> > > There are times (like when going through jni) where it's nice to be able
to
> > > convert to two int64s and back again. These DCHECKs prevent doing so for
> > > kWholeFile.
> > 
> > Assuming we stick with -1 to indicate invalidness, and you would only want
to
> > reconstruct valid Regions from JNI, how about:
> > 
> > if (size == 0) {
> >   DCHECK_EQ(0, offset);
> > } else {
> >   DCHECK_GE(offset, 0);
> >   DCHECK_GT(size, 0);
> > }
> 
> This wouldn't allow you to convert an invalid region (-1,-1) to size/offset
and
> back to a region (as is done in JNI).
> 
> How about I move this check to Initialize (which I've gone ahead and done)?
This
> way we still have the same check where it counts, and Region gets to behave
more
> like a POD.

bump

[Lei Zhang, 2015-06-02 18:59:11]

lgtm

https://codereview.chromium.org/1156183003/diff/100001/base/files/memory_mapp...
File base/files/memory_mapped_file.cc (right):

https://codereview.chromium.org/1156183003/diff/100001/base/files/memory_mapp...
base/files/memory_mapped_file.cc:63: if (!(region == Region::kWholeFile)) {
should we implement operator!= ?

[agrieve, 2015-06-02 19:21:38]

https://codereview.chromium.org/1156183003/diff/100001/base/files/memory_mapp...
File base/files/memory_mapped_file.cc (right):

https://codereview.chromium.org/1156183003/diff/100001/base/files/memory_mapp...
base/files/memory_mapped_file.cc:63: if (!(region == Region::kWholeFile)) {
On 2015/06/02 18:59:11, Lei Zhang wrote:
> should we implement operator!= ?

Seems reasonable doesn't it? :) Done!

[Lei Zhang, 2015-06-02 20:25:44]

lgtm++

https://codereview.chromium.org/1156183003/diff/120001/base/files/memory_mapp...
File base/files/memory_mapped_file.cc (right):

https://codereview.chromium.org/1156183003/diff/120001/base/files/memory_mapp...
base/files/memory_mapped_file.cc:33: return other.offset != offset || other.size
!= size;
Some people just do !(this == other) here, especially if the comparison gets
more complicated.

[agrieve, 2015-06-03 01:17:36]

On 2015/06/02 20:25:44, Lei Zhang wrote:
> lgtm++
> 
>
https://codereview.chromium.org/1156183003/diff/120001/base/files/memory_mapp...
> File base/files/memory_mapped_file.cc (right):
> 
>
https://codereview.chromium.org/1156183003/diff/120001/base/files/memory_mapp...
> base/files/memory_mapped_file.cc:33: return other.offset != offset ||
other.size
> != size;
> Some people just do !(this == other) here, especially if the comparison gets
> more complicated.

Good idea. Will use this trick next time.

[agrieve, 2015-06-03 01:18:20]

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

[agrieve, 2015-06-03 01:18:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1156183003/#ps120001
(title: "Add operator!= for Region")

[commit-bot: I haz the power, 2015-06-03 01:21:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1156183003/120001

[commit-bot: I haz the power, 2015-06-03 04:21:48]

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

[commit-bot: I haz the power, 2015-06-03 04:21:49]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2015-06-03 11:17:47]

lgtm

[agrieve, 2015-06-03 13:38:15]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-06-03 13:38:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1156183003/120001

[commit-bot: I haz the power, 2015-06-03 13:42:00]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-06-03 13:42:54]

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