Fix SELinux violation when opening Chrome on Android

Fix SELinux violation when opening Chrome on Android

This was caused by calling PathUtils.setPrivateDataDirectorySuffix()
on the renderer process. Moving that call into
ChromeBrowserInitializer should fix this.

BUG=560152
Committed: https://crrev.com/b5ce1bf969e760539c96fd215756dd1716415f25
Cr-Commit-Position: refs/heads/master@{#373820}

[hartmanng, 2016-02-01 20:27:55]

Description was changed from

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

BUG=560152
==========

to

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

This was caused by calling PathUtils.setPrivateDataDirectorySuffix()
on the renderer process. Moving that call into
ChromeBrowserInitializer should fix this.

BUG=560152
==========

[hartmanng, 2016-02-01 20:27:55]

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

[hartmanng, 2016-02-01 20:28:24]

Yaron, please take a look.

https://codereview.chromium.org/1655983002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/library_loader/ModernLinker.java
(left):

https://codereview.chromium.org/1655983002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/library_loader/ModernLinker.java:65:
private String mDataDirectory = null;
It's unnecessary to cache this - PathUtils already caches it internally.

[Yaron, 2016-02-01 20:50:40]

lgtm - thanks!
+simonb as FYI for modern linker change - I think it's probably harmless and
potentially even beneficial since we won't block on the AsyncTask completing as
earlier as we used to (potentially) block.

https://codereview.chromium.org/1655983002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
(left):

https://codereview.chromium.org/1655983002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:417:
PathUtils.setPrivateDataDirectorySuffix(PRIVATE_DATA_DIRECTORY_SUFFIX, this);
Maybe easier as a follow-up, but we should also move ContentShell's call out of
this function for parallelism. Then we can say that this isn't called for
anybody in initializeLibraryDependencies.

[hartmanng, 2016-02-01 21:22:48]

thanks!

https://codereview.chromium.org/1655983002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java
(left):

https://codereview.chromium.org/1655983002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:417:
PathUtils.setPrivateDataDirectorySuffix(PRIVATE_DATA_DIRECTORY_SUFFIX, this);
On 2016/02/01 20:50:39, Yaron wrote:
> Maybe easier as a follow-up, but we should also move ContentShell's call out
of
> this function for parallelism. Then we can say that this isn't called for
> anybody in initializeLibraryDependencies.

Probably easier as a follow-up. Filed http://crbug.com/583086

[hartmanng, 2016-02-01 21:25:17]

The CQ bit was checked by hartmanng@chromium.org

[commit-bot: I haz the power, 2016-02-01 21:25:44]

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

[hartmanng, 2016-02-01 22:29:15]

The CQ bit was unchecked by hartmanng@chromium.org

[simonb (inactive), 2016-02-02 11:42:52]

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

[simonb (inactive), 2016-02-02 11:42:52]

lgtm

[hartmanng, 2016-02-03 15:21:12]

Yaron, PTAL, I had to fix a few tests. Is it expected that the
ChromeBrowserInitializer code doesn't get run at all for InstrumentationTests?

[Yaron, 2016-02-03 22:10:24]

https://codereview.chromium.org/1655983002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerTest.java
(right):

https://codereview.chromium.org/1655983002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerTest.java:472:
// TODO: http://crbug.com/583442. This should be refactored to use
ChromeBrowserInitializer instead.
Per your comment this is actually expected so the TODO isn't really accurate.
InstrumentationTestCase is nice in that it's basically a java unit test.
Requiring CBI seems heavy-weight for this. Perhaps it's indicating that our
addition is in the wrong layer and this could be in BrowserStartupController and
not CBI since a content browser needs this too? But as we just discussed that
means the PRIVATE_DATA_DIRECTORY_SUFFIX can't be specified as chrome.

Don't have a very clean solution atm but definitely drop the TODOs.

https://codereview.chromium.org/1655983002/diff/40001/content/public/test/and...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/NativeLibraryTestBase.java
(right):

https://codereview.chromium.org/1655983002/diff/40001/content/public/test/and...
content/public/test/android/javatests/src/org/chromium/content/browser/test/NativeLibraryTestBase.java:22:
private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "chrome";
shouldn't be "chrome" - could be "content"

[hartmanng, 2016-02-05 15:17:33]

https://codereview.chromium.org/1655983002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerTest.java
(right):

https://codereview.chromium.org/1655983002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerTest.java:472:
// TODO: http://crbug.com/583442. This should be refactored to use
ChromeBrowserInitializer instead.
On 2016/02/03 22:10:24, Yaron wrote:
> Per your comment this is actually expected so the TODO isn't really accurate.
> InstrumentationTestCase is nice in that it's basically a java unit test.
> Requiring CBI seems heavy-weight for this. Perhaps it's indicating that our
> addition is in the wrong layer and this could be in BrowserStartupController
and
> not CBI since a content browser needs this too? But as we just discussed that
> means the PRIVATE_DATA_DIRECTORY_SUFFIX can't be specified as chrome.
> 
> Don't have a very clean solution atm but definitely drop the TODOs.

Yeah, I'm not sure what's best here - I've looked around but can't really find a
better spot for this. I've removed the TODOs and changed the
PRIVATE_DATA_DIRECTORY_SUFFIX in the tests to "content" for now.

https://codereview.chromium.org/1655983002/diff/40001/content/public/test/and...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/NativeLibraryTestBase.java
(right):

https://codereview.chromium.org/1655983002/diff/40001/content/public/test/and...
content/public/test/android/javatests/src/org/chromium/content/browser/test/NativeLibraryTestBase.java:22:
private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "chrome";
On 2016/02/03 22:10:24, Yaron wrote:
> shouldn't be "chrome" - could be "content"

Done.

[Yaron, 2016-02-05 15:21:14]

lgtm

https://codereview.chromium.org/1655983002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java
(right):

https://codereview.chromium.org/1655983002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:32:
private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "content";
this could be chrome but I guess it's irrelevant

[hartmanng, 2016-02-05 15:22:50]

The CQ bit was checked by hartmanng@chromium.org

[hartmanng, 2016-02-05 15:22:51]

The patchset sent to the CQ was uploaded after l-g-t-m from simonb@chromium.org
Link to the patchset: https://codereview.chromium.org/1655983002/#ps60001
(title: "review comments")

[commit-bot: I haz the power, 2016-02-05 15:22:59]

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

[hartmanng, 2016-02-05 15:23:04]

On 2016/02/05 15:21:14, Yaron wrote:
> lgtm
> 
>
https://codereview.chromium.org/1655983002/diff/60001/chrome/android/javatest...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java
> (right):
> 
>
https://codereview.chromium.org/1655983002/diff/60001/chrome/android/javatest...
>
chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java:32:
> private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "content";
> this could be chrome but I guess it's irrelevant

Thanks!

[commit-bot: I haz the power, 2016-02-05 16:16:59]

Description was changed from

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

This was caused by calling PathUtils.setPrivateDataDirectorySuffix()
on the renderer process. Moving that call into
ChromeBrowserInitializer should fix this.

BUG=560152
==========

to

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

This was caused by calling PathUtils.setPrivateDataDirectorySuffix()
on the renderer process. Moving that call into
ChromeBrowserInitializer should fix this.

BUG=560152
==========

[commit-bot: I haz the power, 2016-02-05 16:17:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-05 16:18:22]

Description was changed from

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

This was caused by calling PathUtils.setPrivateDataDirectorySuffix()
on the renderer process. Moving that call into
ChromeBrowserInitializer should fix this.

BUG=560152
==========

to

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

This was caused by calling PathUtils.setPrivateDataDirectorySuffix()
on the renderer process. Moving that call into
ChromeBrowserInitializer should fix this.

BUG=560152

Committed: https://crrev.com/b5ce1bf969e760539c96fd215756dd1716415f25
Cr-Commit-Position: refs/heads/master@{#373820}
==========

[commit-bot: I haz the power, 2016-02-05 16:18:23]

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

[jbudorick, 2016-02-08 16:49:24]

On 2016/02/05 16:18:23, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/b5ce1bf969e760539c96fd215756dd1716415f25
> Cr-Commit-Position: refs/heads/master@{#373820}

This broke M: https://code.google.com/p/chromium/issues/detail?id=585130

[hartmanng, 2016-02-08 18:25:10]

On 2016/02/08 16:49:24, jbudorick wrote:
> On 2016/02/05 16:18:23, commit-bot: I haz the power wrote:
> > Patchset 4 (id:??) landed as
> > https://crrev.com/b5ce1bf969e760539c96fd215756dd1716415f25
> > Cr-Commit-Position: refs/heads/master@{#373820}
> 
> This broke M: https://code.google.com/p/chromium/issues/detail?id=585130

Working on it, thanks.

[hartmanng, 2016-02-10 15:04:18]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1684123002/ by hartmanng@chromium.org.

The reason for reverting is: Given the M bot that this broke, plus
http://crbug.com/585421, I'm reverting until I can properly sort out the bugs
here..