Fix SELinux violation when opening Chrome on Android

Fix SELinux violation when opening Chrome on Android

This is a combination of
https://codereview.chromium.org/1655983002/,
https://codereview.chromium.org/1674283003/,
and a fix for http://crbug.com/585421

BUG=585130, 585421, 560152
Committed: https://crrev.com/4805f933d6a3209f466cfcfeaf550b2dd011f051
Cr-Commit-Position: refs/heads/master@{#380406}

[hartmanng, 2016-02-26 21:33:13]

Description was changed from

==========
Fix SELinux violation when opening Chrome on Android

BUG=585130,585421,560152
==========

to

==========
Fix SELinux violation when opening Chrome on Android

This is a combination of
https://codereview.chromium.org/1655983002/,
https://codereview.chromium.org/1674283003/,
and a fix for http://crbug.com/585421

BUG=585130,585421,560152
==========

[hartmanng, 2016-02-27 18:16:00]

hartmanng@chromium.org changed reviewers:
+ yfriedman@chromium.org

[Yaron, 2016-03-01 17:40:01]

lgtm

ideally we can still clean it up so not every instance of
LibraryLoader/BrowserStartupController needs to be preceded with PathUtils but
the benefits are worth it for this change

[Peter Beverloo, 2016-03-01 18:31:25]

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

[Peter Beverloo, 2016-03-01 18:31:26]

gcm_driver lgtm w/ a question

https://codereview.chromium.org/1745623002/diff/1/components/gcm_driver/andro...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
(right):

https://codereview.chromium.org/1745623002/diff/1/components/gcm_driver/andro...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:39:
private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "chrome";
When would use use "chrome" vs. "content"? Using "chrome" in //components feels
a bit icky, but it seems (please correct me if I'm wrong) it implies
"production" vs. "testing" more than layering?

[hartmanng, 2016-03-01 18:43:43]

https://codereview.chromium.org/1745623002/diff/1/components/gcm_driver/andro...
File
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
(right):

https://codereview.chromium.org/1745623002/diff/1/components/gcm_driver/andro...
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:39:
private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "chrome";
On 2016/03/01 18:31:25, Peter Beverloo wrote:
> When would use use "chrome" vs. "content"? Using "chrome" in //components
feels
> a bit icky, but it seems (please correct me if I'm wrong) it implies
> "production" vs. "testing" more than layering?

Yeah, I agree it is pretty icky, but effectively that is really what it turned
into. The problem is that Chrome proper can be started up through GCMDriver, and
in that case we need the PRIVATE_DATA_DIRECTORY_SUFFIX to be the same as other
Chrome entry-points.

[hartmanng, 2016-03-01 18:44:28]

hartmanng@chromium.org changed reviewers:
+ nyquist@chromium.org

[hartmanng, 2016-03-01 18:44:28]

+nyquist@ for
components/invalidation/impl/android/javatests/src/org/chromium/components/invalidation/InvalidationClientServiceTest.java.

Please take a look

[Peter Beverloo, 2016-03-01 18:47:08]

On 2016/03/01 18:43:43, hartmanng wrote:
>
https://codereview.chromium.org/1745623002/diff/1/components/gcm_driver/andro...
> File
>
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
> (right):
> 
>
https://codereview.chromium.org/1745623002/diff/1/components/gcm_driver/andro...
>
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:39:
> private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "chrome";
> On 2016/03/01 18:31:25, Peter Beverloo wrote:
> > When would use use "chrome" vs. "content"? Using "chrome" in //components
> feels
> > a bit icky, but it seems (please correct me if I'm wrong) it implies
> > "production" vs. "testing" more than layering?
> 
> Yeah, I agree it is pretty icky, but effectively that is really what it turned
> into. The problem is that Chrome proper can be started up through GCMDriver,
and
> in that case we need the PRIVATE_DATA_DIRECTORY_SUFFIX to be the same as other
> Chrome entry-points.

What about setting it at the call-sites instead, both of which live in //chrome,
with a comment in GCMDriver.java that callers should set this up themselves?
Duplicating this once more is not great, but it'd be a bit cleaner from a
layering perspective and I don't expect the number of callers to grow anytime
soon, and afaik strict mode is fatal in tests anyway.

I don't feel strongly, so please do as you think is best.

[hartmanng, 2016-03-01 19:53:57]

On 2016/03/01 18:47:08, Peter Beverloo wrote:
> On 2016/03/01 18:43:43, hartmanng wrote:
> >
>
https://codereview.chromium.org/1745623002/diff/1/components/gcm_driver/andro...
> > File
> >
>
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1745623002/diff/1/components/gcm_driver/andro...
> >
>
components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java:39:
> > private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "chrome";
> > On 2016/03/01 18:31:25, Peter Beverloo wrote:
> > > When would use use "chrome" vs. "content"? Using "chrome" in //components
> > feels
> > > a bit icky, but it seems (please correct me if I'm wrong) it implies
> > > "production" vs. "testing" more than layering?
> > 
> > Yeah, I agree it is pretty icky, but effectively that is really what it
turned
> > into. The problem is that Chrome proper can be started up through GCMDriver,
> and
> > in that case we need the PRIVATE_DATA_DIRECTORY_SUFFIX to be the same as
other
> > Chrome entry-points.
> 
> What about setting it at the call-sites instead, both of which live in
//chrome,
> with a comment in GCMDriver.java that callers should set this up themselves?
> Duplicating this once more is not great, but it'd be a bit cleaner from a
> layering perspective and I don't expect the number of callers to grow anytime
> soon, and afaik strict mode is fatal in tests anyway.
> 
> I don't feel strongly, so please do as you think is best.

sgtm, that feels a little less icky, imo. I've moved that code to the callers -
currently running it through the bots to make sure I didn't break anything in
the process.

[nyquist, 2016-03-02 08:22:53]

Is there anything else than 'content' we could use for invalidation?

Also, does this mean that you can remove wnwen's TODO for not calling doing the
path stuff from the renderer?

https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...

https://codereview.chromium.org/1745623002/diff/20001/components/invalidation...
File
components/invalidation/impl/android/javatests/src/org/chromium/components/invalidation/InvalidationClientServiceTest.java
(right):

https://codereview.chromium.org/1745623002/diff/20001/components/invalidation...
components/invalidation/impl/android/javatests/src/org/chromium/components/invalidation/InvalidationClientServiceTest.java:47:
private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "content";
Uhm. This looks awkward, given //components/invalidation/DEPS specifically
disallows depending on anything content.

[hartmanng, 2016-03-08 20:35:15]

hartmanng@chromium.org changed reviewers:
+ wnwen@chromium.org

[hartmanng, 2016-03-08 20:53:19]

Sorry for the delay.



On 2016/03/02 08:22:53, nyquist wrote:
> Is there anything else than 'content' we could use for invalidation?
> 
> Also, does this mean that you can remove wnwen's TODO for not calling doing
the
> path stuff from the renderer?
> 


I believe we can remove wnwen's TODO, yes. +wnwen for confirmation.

https://codereview.chromium.org/1745623002/diff/20001/components/invalidation...
File
components/invalidation/impl/android/javatests/src/org/chromium/components/invalidation/InvalidationClientServiceTest.java
(right):

https://codereview.chromium.org/1745623002/diff/20001/components/invalidation...
components/invalidation/impl/android/javatests/src/org/chromium/components/invalidation/InvalidationClientServiceTest.java:47:
private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "content";
On 2016/03/02 08:22:53, nyquist wrote:
> Uhm. This looks awkward, given //components/invalidation/DEPS specifically
> disallows depending on anything content.

Sure. It's just a test, so I believe it can be anything we want. How's
"invalidation_test"? Or anything else you think fits better.

[Peter Wen, 2016-03-08 21:05:03]

lgtm

If you feel renaming would bloat this CL then a follow-up CL is fine too.

https://codereview.chromium.org/1745623002/diff/60001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/PathUtils.java (right):

https://codereview.chromium.org/1745623002/diff/60001/base/android/java/src/o...
base/android/java/src/org/chromium/base/PathUtils.java:102: private static
String[] setPrivateDataDirectorySuffixInternal() {
This would be a good time to change the name of this function to be more
meaningful.

[hartmanng, 2016-03-08 21:18:45]

https://codereview.chromium.org/1745623002/diff/60001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/PathUtils.java (right):

https://codereview.chromium.org/1745623002/diff/60001/base/android/java/src/o...
base/android/java/src/org/chromium/base/PathUtils.java:102: private static
String[] setPrivateDataDirectorySuffixInternal() {
On 2016/03/08 21:05:03, Peter Wen wrote:
> This would be a good time to change the name of this function to be more
> meaningful.

Yeah, it does seem like a decent opportunity... But by my count renaming would
require adding 9 additional files into the CL. Given that this particular CL is
already a combination of 2-3 others, and has been in progress, in some form or
another, for a few weeks, I'd rather do it in a follow-up.

[Yaron, 2016-03-09 03:27:03]

On 2016/03/08 21:18:45, hartmanng wrote:
>
https://codereview.chromium.org/1745623002/diff/60001/base/android/java/src/o...
> File base/android/java/src/org/chromium/base/PathUtils.java (right):
> 
>
https://codereview.chromium.org/1745623002/diff/60001/base/android/java/src/o...
> base/android/java/src/org/chromium/base/PathUtils.java:102: private static
> String[] setPrivateDataDirectorySuffixInternal() {
> On 2016/03/08 21:05:03, Peter Wen wrote:
> > This would be a good time to change the name of this function to be more
> > meaningful.
> 
> Yeah, it does seem like a decent opportunity... But by my count renaming would
> require adding 9 additional files into the CL. Given that this particular CL
is
> already a combination of 2-3 others, and has been in progress, in some form or
> another, for a few weeks, I'd rather do it in a follow-up.

+1 - there's enough going on already

[hartmanng, 2016-03-09 22:51:45]

nyquist@, PTAL

[nyquist, 2016-03-10 00:28:18]

lgtm, thanks!

[hartmanng, 2016-03-10 13:33:35]

thanks!

[hartmanng, 2016-03-10 13:33:41]

The CQ bit was checked by hartmanng@chromium.org

[hartmanng, 2016-03-10 13:33:41]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1745623002/#ps60001
(title: "nyquist comments")

[commit-bot: I haz the power, 2016-03-10 13:33:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1745623002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1745623002/60001

[commit-bot: I haz the power, 2016-03-10 15:11:49]

Description was changed from

==========
Fix SELinux violation when opening Chrome on Android

This is a combination of
https://codereview.chromium.org/1655983002/,
https://codereview.chromium.org/1674283003/,
and a fix for http://crbug.com/585421

BUG=585130,585421,560152
==========

to

==========
Fix SELinux violation when opening Chrome on Android

This is a combination of
https://codereview.chromium.org/1655983002/,
https://codereview.chromium.org/1674283003/,
and a fix for http://crbug.com/585421

BUG=585130,585421,560152
==========

[commit-bot: I haz the power, 2016-03-10 15:11:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-10 15:14:02]

Description was changed from

==========
Fix SELinux violation when opening Chrome on Android

This is a combination of
https://codereview.chromium.org/1655983002/,
https://codereview.chromium.org/1674283003/,
and a fix for http://crbug.com/585421

BUG=585130,585421,560152
==========

to

==========
Fix SELinux violation when opening Chrome on Android

This is a combination of
https://codereview.chromium.org/1655983002/,
https://codereview.chromium.org/1674283003/,
and a fix for http://crbug.com/585421

BUG=585130,585421,560152

Committed: https://crrev.com/4805f933d6a3209f466cfcfeaf550b2dd011f051
Cr-Commit-Position: refs/heads/master@{#380406}
==========

[commit-bot: I haz the power, 2016-03-10 15:14:03]

Patchset 4 (id:??) landed as
https://crrev.com/4805f933d6a3209f466cfcfeaf550b2dd011f051
Cr-Commit-Position: refs/heads/master@{#380406}