Fix broken tests on M.

Fix broken tests on M.

https://codereview.chromium.org/1655983002/
caused a few tests to break on M.

Under certain build configurations (in particular,
my gn build was fine, but gyp has failing tests,
although there may be other differences such as
component build), the ModernLinker gets called
before PathUtils.setPrivateDataDirectorySuffix()
in some tests.

BUG=585130

[hartmanng, 2016-02-08 21:34:31]

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

[hartmanng, 2016-02-08 21:34:31]

Yaron, please take a look.

[jbudorick, 2016-02-08 21:36:43]

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

[jbudorick, 2016-02-08 21:36:43]

The gyp/gn difference is surprising, as the bot where this error is happening is
using a gn build. Were you not able to reproduce with a GN build at all?

[hartmanng, 2016-02-08 21:39:58]

On 2016/02/08 21:36:43, jbudorick wrote:
> The gyp/gn difference is surprising, as the bot where this error is happening
is
> using a gn build. Were you not able to reproduce with a GN build at all?

Hm, interesting. No I never did manage to reproduce it with a GN build, but it
could be due to some other configuration I had. I never tried a clean GN build,
whereas my gyp build was from-scratch. Also worth noting that both my GN and GYP
builds were debug builds. I can keep trying to figure out what specifically
about my GN build caused this not to reproduce, if it's helpful.

[jbudorick, 2016-02-08 21:42:30]

On 2016/02/08 21:39:58, hartmanng wrote:
> On 2016/02/08 21:36:43, jbudorick wrote:
> > The gyp/gn difference is surprising, as the bot where this error is
happening
> is
> > using a gn build. Were you not able to reproduce with a GN build at all?
> 
> Hm, interesting. No I never did manage to reproduce it with a GN build, but it
> could be due to some other configuration I had. I never tried a clean GN
build,
> whereas my gyp build was from-scratch. Also worth noting that both my GN and
GYP
> builds were debug builds. I can keep trying to figure out what specifically
> about my GN build caused this not to reproduce, if it's helpful.

The bot is using '--args=target_cpu="arm" target_os="android" is_debug=true
is_component_build=false use_goma=true symbol_level=1' + a goma_dir.

[hartmanng, 2016-02-08 21:45:25]

On 2016/02/08 21:42:30, jbudorick wrote:
> On 2016/02/08 21:39:58, hartmanng wrote:
> > On 2016/02/08 21:36:43, jbudorick wrote:
> > > The gyp/gn difference is surprising, as the bot where this error is
> happening
> > is
> > > using a gn build. Were you not able to reproduce with a GN build at all?
> > 
> > Hm, interesting. No I never did manage to reproduce it with a GN build, but
it
> > could be due to some other configuration I had. I never tried a clean GN
> build,
> > whereas my gyp build was from-scratch. Also worth noting that both my GN and
> GYP
> > builds were debug builds. I can keep trying to figure out what specifically
> > about my GN build caused this not to reproduce, if it's helpful.
> 
> The bot is using '--args=target_cpu="arm" target_os="android" is_debug=true
> is_component_build=false use_goma=true symbol_level=1' + a goma_dir.



My gn args were set to:

use_goma=true
target_os="android"
is_debug=true
is_component_build=true
is_clang=true
symbol_level=1
enable_incremental_javac=true



so in particular, I have clang, component build, and incremental javac

[Yaron, 2016-02-08 21:51:41]

https://codereview.chromium.org/1674283003/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
(right):

https://codereview.chromium.org/1674283003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:74:
public class CustomTabActivityTest extends CustomTabActivityTestBase {
I think the CustomTabs ones may be indicative of a real bug.

Note CustomTabsConnection.initializeBrowser. It has a separate call out to
LibraryLoader and uses a slightly different flow through CBI
(ChromeBrowserInitializer.handleSynchronousStartup) which in turn calls
PathUtils but we've already invoked LibraryLoader.

[Yaron, 2016-02-10 14:56:17]

On 2016/02/08 21:51:41, Yaron wrote:
>
https://codereview.chromium.org/1674283003/diff/1/chrome/android/javatests/sr...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
> (right):
> 
>
https://codereview.chromium.org/1674283003/diff/1/chrome/android/javatests/sr...
>
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:74:
> public class CustomTabActivityTest extends CustomTabActivityTestBase {
> I think the CustomTabs ones may be indicative of a real bug.
> 
> Note CustomTabsConnection.initializeBrowser. It has a separate call out to
> LibraryLoader and uses a slightly different flow through CBI
> (ChromeBrowserInitializer.handleSynchronousStartup) which in turn calls
> PathUtils but we've already invoked LibraryLoader.

It's probably best to follow-up with the initial authors but this code just
looks odd to me:
 // Loading the native library may spuriously trigger StrictMode violations when
            // running instrumentation tests. This does not happen if full
Chrome has
            // started before reaching this point.
            // crbug.com/574532
            if (!LibraryLoader.isInitialized()) {
                StrictMode.ThreadPolicy oldPolicy =
StrictMode.allowThreadDiskReads();
                LibraryLoader.get(LibraryProcessType.PROCESS_BROWSER)
                        .ensureInitialized(app.getApplicationContext());
                StrictMode.setThreadPolicy(oldPolicy);
            }

(from CustomTabsConnection.java). It seems like it's test-only code but it's in
the app itself. Perhaps it can be moved out to just the test fixture?
Regardless, I don't understand why it's necessary given that the next step was
supposed to load the native library.

[hartmanng, 2016-02-25 22:25:46]

On 2016/02/10 14:56:17, Yaron wrote:
> On 2016/02/08 21:51:41, Yaron wrote:
> >
>
https://codereview.chromium.org/1674283003/diff/1/chrome/android/javatests/sr...
> > File
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1674283003/diff/1/chrome/android/javatests/sr...
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:74:
> > public class CustomTabActivityTest extends CustomTabActivityTestBase {
> > I think the CustomTabs ones may be indicative of a real bug.
> > 
> > Note CustomTabsConnection.initializeBrowser. It has a separate call out to
> > LibraryLoader and uses a slightly different flow through CBI
> > (ChromeBrowserInitializer.handleSynchronousStartup) which in turn calls
> > PathUtils but we've already invoked LibraryLoader.
> 
> It's probably best to follow-up with the initial authors but this code just
> looks odd to me:
>  // Loading the native library may spuriously trigger StrictMode violations
when
>             // running instrumentation tests. This does not happen if full
> Chrome has
>             // started before reaching this point.
>             // crbug.com/574532
>             if (!LibraryLoader.isInitialized()) {
>                 StrictMode.ThreadPolicy oldPolicy =
> StrictMode.allowThreadDiskReads();
>                 LibraryLoader.get(LibraryProcessType.PROCESS_BROWSER)
>                         .ensureInitialized(app.getApplicationContext());
>                 StrictMode.setThreadPolicy(oldPolicy);
>             }
> 
> (from CustomTabsConnection.java). It seems like it's test-only code but it's
in
> the app itself. Perhaps it can be moved out to just the test fixture?
> Regardless, I don't understand why it's necessary given that the next step was
> supposed to load the native library.


I've been poking around in this code for a while and I believe the reason the
LibraryLoader is called here is that the StrictMode policies are reset in
ChromeBrowserInitializer.handleSynchronousStartup().

Specifically,
handleSynchronousStartup()->handlePreNativeStartup()->preInflationStartup()->ChromeStrictMode.configureStrictMode().

So in order to suppress StrictMode they had to load the library before the
StrictMode profile got reset to default.

I do believe this is test-specific code, and I should be able to move it into
the test fixture. I'm not sure I have a great solution to get rid of the extra
calls into LibraryLoader, though - as far as I can tell, we may need to call it
inside CustomTabsConnectionTest after setting the private data directory suffix.

IIUC, it looks like CustomTabsActivityTest was only affected by accident.

[Yaron, 2016-02-26 05:15:29]

On 2016/02/25 22:25:46, hartmanng wrote:
> On 2016/02/10 14:56:17, Yaron wrote:
> > On 2016/02/08 21:51:41, Yaron wrote:
> > >
> >
>
https://codereview.chromium.org/1674283003/diff/1/chrome/android/javatests/sr...
> > > File
> > >
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1674283003/diff/1/chrome/android/javatests/sr...
> > >
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:74:
> > > public class CustomTabActivityTest extends CustomTabActivityTestBase {
> > > I think the CustomTabs ones may be indicative of a real bug.
> > > 
> > > Note CustomTabsConnection.initializeBrowser. It has a separate call out to
> > > LibraryLoader and uses a slightly different flow through CBI
> > > (ChromeBrowserInitializer.handleSynchronousStartup) which in turn calls
> > > PathUtils but we've already invoked LibraryLoader.
> > 
> > It's probably best to follow-up with the initial authors but this code just
> > looks odd to me:
> >  // Loading the native library may spuriously trigger StrictMode violations
> when
> >             // running instrumentation tests. This does not happen if full
> > Chrome has
> >             // started before reaching this point.
> >             // crbug.com/574532
> >             if (!LibraryLoader.isInitialized()) {
> >                 StrictMode.ThreadPolicy oldPolicy =
> > StrictMode.allowThreadDiskReads();
> >                 LibraryLoader.get(LibraryProcessType.PROCESS_BROWSER)
> >                         .ensureInitialized(app.getApplicationContext());
> >                 StrictMode.setThreadPolicy(oldPolicy);
> >             }
> > 
> > (from CustomTabsConnection.java). It seems like it's test-only code but it's
> in
> > the app itself. Perhaps it can be moved out to just the test fixture?
> > Regardless, I don't understand why it's necessary given that the next step
was
> > supposed to load the native library.
> 
> 
> I've been poking around in this code for a while and I believe the reason the
> LibraryLoader is called here is that the StrictMode policies are reset in
> ChromeBrowserInitializer.handleSynchronousStartup().
> 
> Specifically,
>
handleSynchronousStartup()->handlePreNativeStartup()->preInflationStartup()->ChromeStrictMode.configureStrictMode().
> 
> So in order to suppress StrictMode they had to load the library before the
> StrictMode profile got reset to default.
> 
> I do believe this is test-specific code, and I should be able to move it into
> the test fixture. I'm not sure I have a great solution to get rid of the extra
> calls into LibraryLoader, though - as far as I can tell, we may need to call
it
> inside CustomTabsConnectionTest after setting the private data directory
suffix.

Thanks for digging into this.
See comment #11 on crbug/574532
Just want to double check that by moving it to the test fixture and rolling
forward this change, we don't crash in the scenario mentioned there when
ModernLinker is used

> 
> IIUC, it looks like CustomTabsActivityTest was only affected by accident.

[hartmanng, 2016-02-26 16:15:34]

On 2016/02/26 05:15:29, Yaron wrote:
> Thanks for digging into this.
> See comment #11 on crbug/574532
> Just want to double check that by moving it to the test fixture and rolling
> forward this change, we don't crash in the scenario mentioned there when
> ModernLinker is used

Not sure which crash you're talking about :)

If we move the code lizeb@ added from CustomTabsConnection into the test
fixture, then I'm confident that the PathUtils stuff I'm working on won't cause
a crash (my change will add a call to PathUtils.setPrivateDataDirectorySuffix()
into the beginning of ChromeBrowserInitializer.handlePreNativeStartup(), so when
CustomTabsConnection calls into
ChromeBrowserInitializer.handleSynchronousStartup(), it will set up the private
data directory suffix before trying any LibraryLoader stuff).


However, based on http://crbug.com/574532#c11, if that scenario is possible,
then moving the "test-only" code out of CustomTabsConnection could cause us to
have an unwhitelisted StrictMode violation later in ChromeBrowserInitializer
when the LibraryLoader stuff does happen (as part of handlePostNativeStartup()).
Unwhitelisted StrictMode violations do crash on certain types of builds.

I'm not really sure how likely/common the scenario in
http://crbug.com/574532#c11 is, or how to even simulate it, actually.

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

On 2016/02/26 16:15:34, hartmanng wrote:
> On 2016/02/26 05:15:29, Yaron wrote:
> > Thanks for digging into this.
> > See comment #11 on crbug/574532
> > Just want to double check that by moving it to the test fixture and rolling
> > forward this change, we don't crash in the scenario mentioned there when
> > ModernLinker is used
> 
> Not sure which crash you're talking about :)

Ah, I thought there was a crash with ModernLinker requiring the directory early
on, and before pathutils was setup but that's addressed belwo
> 
> If we move the code lizeb@ added from CustomTabsConnection into the test
> fixture, then I'm confident that the PathUtils stuff I'm working on won't
cause
> a crash (my change will add a call to
PathUtils.setPrivateDataDirectorySuffix()
> into the beginning of ChromeBrowserInitializer.handlePreNativeStartup(), so
when
> CustomTabsConnection calls into
> ChromeBrowserInitializer.handleSynchronousStartup(), it will set up the
private
> data directory suffix before trying any LibraryLoader stuff).
> 
> 
> However, based on http://crbug.com/574532#c11, if that scenario is possible,
> then moving the "test-only" code out of CustomTabsConnection could cause us to
> have an unwhitelisted StrictMode violation later in ChromeBrowserInitializer
> when the LibraryLoader stuff does happen (as part of
handlePostNativeStartup()).
> Unwhitelisted StrictMode violations do crash on certain types of builds.
> 
> I'm not really sure how likely/common the scenario in
> http://crbug.com/574532#c11 is, or how to even simulate it, actually.

[hartmanng, 2016-03-01 18:23:20]

Description was changed from

==========
Fix broken tests on M.

https://codereview.chromium.org/1655983002/
caused a few tests to break on M.

Under certain build configurations (in particular,
my gn build was fine, but gyp has failing tests,
although there may be other differences such as
component build), the ModernLinker gets called
before PathUtils.setPrivateDataDirectorySuffix()
in some tests.

BUG=585130
==========

to

==========
Fix broken tests on M.

https://codereview.chromium.org/1655983002/
caused a few tests to break on M.

Under certain build configurations (in particular,
my gn build was fine, but gyp has failing tests,
although there may be other differences such as
component build), the ModernLinker gets called
before PathUtils.setPrivateDataDirectorySuffix()
in some tests.

BUG=585130
==========