Making ChildConnectionAllocator simpler.

Making ChildConnectionAllocator simpler.

Removing the sandboxed state from ChildConnectionAllocator and also
removing the default service class names: it is now always retrieved from
the AndroidManifest.xml. BaseChildProcessConnection does not have a
sandboxed state anymore either now.
The allocators are now held and managed by the ChildProcessLauncher.

BUG=689758
Review-Url: https://codereview.chromium.org/2863063002
Cr-Commit-Position: refs/heads/master@{#470146}
Committed: https://chromium.googlesource.com/chromium/src/+/3aa41d205d3efc87258a59e4db771ba9cabee9c2

[Jay Civelli, 2017-05-05 04:59:30]

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

[commit-bot: I haz the power, 2017-05-05 04:59:49]

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, 2017-05-05 06:50:42]

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

[commit-bot: I haz the power, 2017-05-05 06:50:43]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Jay Civelli, 2017-05-05 19:55:46]

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

[commit-bot: I haz the power, 2017-05-05 19:56:22]

Dry run: CQ is trying da patch.

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

[Jay Civelli, 2017-05-05 21:18:18]

Description was changed from

==========
Making ConnectionAllocator simpler.

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

to

==========
Making ChildConnectionAllocator simpler.

Removing the sandboxed state from ChildConnectionAllocator and also
removing default service class name: it is now always retrieved from
the AndroidManifest.xml. BaseChildProcessConnection does not have a
sandboxed  state anymore.
The allocators are now held and managed by the ChildProcessLauncher.

BUG=689758
==========

[Jay Civelli, 2017-05-05 21:18:18]

jcivelli@chromium.org changed reviewers:
+ boliu@chromium.org

[Jay Civelli, 2017-05-05 21:18:36]

Patchset #1 (id:1) has been deleted

[Jay Civelli, 2017-05-05 21:19:10]

Description was changed from

==========
Making ChildConnectionAllocator simpler.

Removing the sandboxed state from ChildConnectionAllocator and also
removing default service class name: it is now always retrieved from
the AndroidManifest.xml. BaseChildProcessConnection does not have a
sandboxed  state anymore.
The allocators are now held and managed by the ChildProcessLauncher.

BUG=689758
==========

to

==========
Making ChildConnectionAllocator simpler.

Removing the sandboxed state from ChildConnectionAllocator and also
removing default service class name: it is now always retrieved from
the AndroidManifest.xml. BaseChildProcessConnection does not have a
sandboxed state anymore.
The allocators are now held and managed by the ChildProcessLauncher.

BUG=689758
==========

[Jay Civelli, 2017-05-05 21:19:33]

Description was changed from

==========
Making ChildConnectionAllocator simpler.

Removing the sandboxed state from ChildConnectionAllocator and also
removing default service class name: it is now always retrieved from
the AndroidManifest.xml. BaseChildProcessConnection does not have a
sandboxed state anymore.
The allocators are now held and managed by the ChildProcessLauncher.

BUG=689758
==========

to

==========
Making ChildConnectionAllocator simpler.

Removing the sandboxed state from ChildConnectionAllocator and also
removing the default service class names: it is now always retrieved from
the AndroidManifest.xml. BaseChildProcessConnection does not have a
sandboxed state anymore.
The allocators are now held and managed by the ChildProcessLauncher.

BUG=689758
==========

[Jay Civelli, 2017-05-05 21:19:51]

Description was changed from

==========
Making ChildConnectionAllocator simpler.

Removing the sandboxed state from ChildConnectionAllocator and also
removing the default service class names: it is now always retrieved from
the AndroidManifest.xml. BaseChildProcessConnection does not have a
sandboxed state anymore.
The allocators are now held and managed by the ChildProcessLauncher.

BUG=689758
==========

to

==========
Making ChildConnectionAllocator simpler.

Removing the sandboxed state from ChildConnectionAllocator and also
removing the default service class names: it is now always retrieved from
the AndroidManifest.xml. BaseChildProcessConnection does not have a
sandboxed state anymore either now.
The allocators are now held and managed by the ChildProcessLauncher.

BUG=689758
==========

[Jay Civelli, 2017-05-05 21:24:22]

Patchset #1 (id:20001) has been deleted

[Jay Civelli, 2017-05-05 21:24:31]

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

[commit-bot: I haz the power, 2017-05-05 21:25:33]

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, 2017-05-05 23:14:07]

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

[commit-bot: I haz the power, 2017-05-05 23:14:08]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Jay Civelli, 2017-05-05 23:37:32]

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

[commit-bot: I haz the power, 2017-05-05 23:38:45]

Dry run: CQ is trying da patch.

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

[boliu, 2017-05-06 00:04:49]

cursory look

you missed updating chrome/android/webapk/shell_apk/AndroidManifest.xml

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

https://codereview.chromium.org/2863063002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:65:
private static final Set<BaseChildProcessConnection> sSandboxedConnections = new
HashSet<>();
I really don't like this thing.

Maybe have connection have a reference back to its allocator?

Or keep a connection -> allocator map without making any distinctions whether a
connection is sandboxed or not.

https://codereview.chromium.org/2863063002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:623:
if (!sSandboxedServiceCountMap.containsKey(packageName)) {
this is not gonna work I think. there is no reason this request has to come
after we actually allocated a service

[Jay Civelli, 2017-05-06 00:49:41]

On 2017/05/06 00:04:49, boliu wrote:
> cursory look
> 
> you missed updating chrome/android/webapk/shell_apk/AndroidManifest.xml
It already has SANDBOXED_SERVICES_NAME.
> 
>
https://codereview.chromium.org/2863063002/diff/40001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
> (right):
> 
>
https://codereview.chromium.org/2863063002/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:65:
> private static final Set<BaseChildProcessConnection> sSandboxedConnections =
new
> HashSet<>();
> I really don't like this thing.
> 
> Maybe have connection have a reference back to its allocator?
> 
> Or keep a connection -> allocator map without making any distinctions whether
a
> connection is sandboxed or not.
> 
>
https://codereview.chromium.org/2863063002/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:623:
> if (!sSandboxedServiceCountMap.containsKey(packageName)) {
> this is not gonna work I think. there is no reason this request has to come
> after we actually allocated a service

[Jay Civelli, 2017-05-06 00:50:03]

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

https://codereview.chromium.org/2863063002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:65:
private static final Set<BaseChildProcessConnection> sSandboxedConnections = new
HashSet<>();
On 2017/05/06 00:04:48, boliu wrote:
> I really don't like this thing.
> 
> Maybe have connection have a reference back to its allocator?
I don't think connection should have a reference to or know about its allocator.

> Or keep a connection -> allocator map without making any distinctions whether
a
> connection is sandboxed or not.
Why would a map of all connections be a better alternative? (why do more work
than what we need)

https://codereview.chromium.org/2863063002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:623:
if (!sSandboxedServiceCountMap.containsKey(packageName)) {
On 2017/05/06 00:04:48, boliu wrote:
> this is not gonna work I think. there is no reason this request has to come
> after we actually allocated a service

Good point. Going back to reading from the manifest.

[Jay Civelli, 2017-05-06 00:50:08]

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

[commit-bot: I haz the power, 2017-05-06 00:50:43]

Dry run: CQ is trying da patch.

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

[boliu, 2017-05-06 00:55:02]

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

https://codereview.chromium.org/2863063002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:65:
private static final Set<BaseChildProcessConnection> sSandboxedConnections = new
HashSet<>();
On 2017/05/06 00:50:02, Jay Civelli wrote:
> On 2017/05/06 00:04:48, boliu wrote:
> > I really don't like this thing.
> > 
> > Maybe have connection have a reference back to its allocator?
> I don't think connection should have a reference to or know about its
allocator.
> 
> > Or keep a connection -> allocator map without making any distinctions
whether
> a
> > connection is sandboxed or not.
> Why would a map of all connections be a better alternative? (why do more work
> than what we need)

All the benefits are in freeConnection. You wouldn't have to worry about
sandboxed or not at all. And you also solve the null context problem as well.

I mean imagine if getConnectionAllocator starts taking more parameters or more
complex parameters. Then this would not scale.

[Jay Civelli, 2017-05-06 01:18:38]

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

https://codereview.chromium.org/2863063002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:65:
private static final Set<BaseChildProcessConnection> sSandboxedConnections = new
HashSet<>();
On 2017/05/06 00:55:02, boliu wrote:
> On 2017/05/06 00:50:02, Jay Civelli wrote:
> > On 2017/05/06 00:04:48, boliu wrote:
> > > I really don't like this thing.
> > > 
> > > Maybe have connection have a reference back to its allocator?
> > I don't think connection should have a reference to or know about its
> allocator.
> > 
> > > Or keep a connection -> allocator map without making any distinctions
> whether
> > a
> > > connection is sandboxed or not.
> > Why would a map of all connections be a better alternative? (why do more
work
> > than what we need)
> 
> All the benefits are in freeConnection. You wouldn't have to worry about
> sandboxed or not at all. And you also solve the null context problem as well.
> 
> I mean imagine if getConnectionAllocator starts taking more parameters or more
> complex parameters. Then this would not scale.

Very good point, done.

[Jay Civelli, 2017-05-06 01:18:52]

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

[commit-bot: I haz the power, 2017-05-06 01:19:18]

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, 2017-05-06 03:33:11]

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

[commit-bot: I haz the power, 2017-05-06 03:33:12]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Jay Civelli, 2017-05-08 00:26:35]

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

[commit-bot: I haz the power, 2017-05-08 00:26:44]

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, 2017-05-08 01:09:26]

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

[commit-bot: I haz the power, 2017-05-08 01:09:27]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-05-08 16:35:04]

lgtm

https://codereview.chromium.org/2863063002/diff/120001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
(right):

https://codereview.chromium.org/2863063002/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:106:
removeConnectionImpl(managedConnection);
existing code, but I think else block can never happen?

[Jay Civelli, 2017-05-08 17:09:26]

jcivelli@chromium.org changed reviewers:
+ alokp@chromium.org, nyquist@chromium.org, tedchoc@chromium.org

[Jay Civelli, 2017-05-08 17:09:27]

Adding OWNERS reviewers:
nyquist@ for chrome/android/java/AndroidManifest.xml
alokp@ for chromecast/browser/android/apk/AndroidManifest.xml.jinja2
tedchoc@ for components/test/android/browsertests_apk/AndroidManifest.xml.jinja2

https://codereview.chromium.org/2863063002/diff/120001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
(right):

https://codereview.chromium.org/2863063002/diff/120001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:106:
removeConnectionImpl(managedConnection);
On 2017/05/08 16:35:04, boliu wrote:
> existing code, but I think else block can never happen?

If we fail to bind the moderate binding, I believe the else block will be run.
But I am not sure if that makes any sense.
I'll clean that up as a follow up CL (filed
https://bugs.chromium.org/p/chromium/issues/detail?id=719575 for it).

[alokp, 2017-05-08 17:22:51]

chromecast/ lgtm (+sanfin@ FYI)

[Ted C, 2017-05-08 20:10:13]

On 2017/05/08 17:22:51, alokp wrote:
> chromecast/ lgtm (+sanfin@ FYI)

components/test/android/browsertests_apk/AndroidManifest.xml.jinja2 - lgtm

[nyquist, 2017-05-08 21:31:27]

chrome/android/java/AndroidManifest.xml lgtm

[Jay Civelli, 2017-05-08 21:46:54]

The CQ bit was checked by jcivelli@chromium.org

[commit-bot: I haz the power, 2017-05-08 21:47:41]

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, 2017-05-08 22:08:22]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494280014531440,
"parent_rev": "c5db504517bd1b4954ec7492e4460478eb60d7c5", "commit_rev":
"3aa41d205d3efc87258a59e4db771ba9cabee9c2"}

[commit-bot: I haz the power, 2017-05-08 22:09:29]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494280014531440,
"parent_rev": "c5db504517bd1b4954ec7492e4460478eb60d7c5", "commit_rev":
"3aa41d205d3efc87258a59e4db771ba9cabee9c2"}

[commit-bot: I haz the power, 2017-05-08 22:09:36]

Description was changed from

==========
Making ChildConnectionAllocator simpler.

Removing the sandboxed state from ChildConnectionAllocator and also
removing the default service class names: it is now always retrieved from
the AndroidManifest.xml. BaseChildProcessConnection does not have a
sandboxed state anymore either now.
The allocators are now held and managed by the ChildProcessLauncher.

BUG=689758
==========

to

==========
Making ChildConnectionAllocator simpler.

Removing the sandboxed state from ChildConnectionAllocator and also
removing the default service class names: it is now always retrieved from
the AndroidManifest.xml. BaseChildProcessConnection does not have a
sandboxed state anymore either now.
The allocators are now held and managed by the ChildProcessLauncher.

BUG=689758

Review-Url: https://codereview.chromium.org/2863063002
Cr-Commit-Position: refs/heads/master@{#470146}
Committed:
https://chromium.googlesource.com/chromium/src/+/3aa41d205d3efc87258a59e4db77...
==========

[commit-bot: I haz the power, 2017-05-08 22:09:38]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/3aa41d205d3efc87258a59e4db77...