PlatformAppBrowserTest::SetUp should call at the end of AppShimInteractiveTest::SetUp()

PlatformAppBrowserTest::SetUp should call at the end of AppShimInteractiveTest::SetUp()

AppShimInteractiveTest initializes a ScopedFeatureList in its SetUp()
method after PlatformAppBrowserTest::SetUp. But testcases run in:

PlatformAppBrowserTest ->
ExtensionApiTest ->
ExtensionBrowserTest -> InProcessBrowserTest ->
BrowserTestBase::SetUp[1]

BrowserTestBase::SetUp calls ContentMain() to launch the browser and
run testcase and the browser destroys its FieldTrialList and
FeatureList on shutdown.

Stack trace:
base::FieldTrialList::~FieldTrialList()
ChromeBrowserMainParts::~ChromeBrowserMainParts()
ChromeBrowserMainPartsMac::~ChromeBrowserMainPartsMac()
content::BrowserMainLoop::~BrowserMainLoop()
content::BrowserMainLoop::~BrowserMainLoop()
content::BrowserMainRunnerImpl::Shutdown()
content::BrowserMain(content::MainFunctionParams const&)
content::RunNamedProcessTypeMain(std::__1::basic_string<char, 
content::MainFunctionParams const&, content::ContentMainDelegate*)
content::ContentMainRunnerImpl::Run()
service_manager::Main(service_manager::MainParams const&)
content::ContentMain(content::ContentMainParams const&)
content::BrowserTestBase::SetUp()
InProcessBrowserTest::SetUp()


That is why it caused the memory issue. So we should call
PlatformAppBrowserTest::SetUp call at the end of
AppShimInteractiveTest::SetUp()

BUG=

Review-Url: https://codereview.chromium.org/2963193004
Cr-Commit-Position: refs/heads/master@{#484416}
Committed: https://chromium.googlesource.com/chromium/src/+/9d126b1389be2431d6c95eae379f5f0f059089c6

[chaopeng, 2017-07-01 03:49:51]

chaopeng@chromium.org changed reviewers:
+ isherman@chromium.org

[chaopeng, 2017-07-01 03:49:51]

We got a memory regression from Change ScopedFeatureList to overrides
FeatureList not reset. Need this patch before reland.

Ilya, PTAL. Thank you.

[Ilya Sherman, 2017-07-03 02:27:00]

LGTM

[chaopeng, 2017-07-03 03:24:58]

chaopeng@chromium.org changed reviewers:
+ thestig@chromium.org

[chaopeng, 2017-07-03 03:24:58]

thestig@chromium.org: Please review changes。 Thank you.

[chaopeng, 2017-07-03 03:25:01]

thestig@chromium.org: Please review changes。 Thank you.

[Lei Zhang, 2017-07-05 18:37:47]

lgtm

For the CL description:
1) There's a typo: BroswerTest
2) It's unclear what the "[1]" is suppose to reference. Right now it points to a
#if defined() line.
3) Can you refer to real class names? There's no class called BrowserTest or
Super. The inheritance chain is PlatformAppBrowserTest -> ExtensionApiTest ->
ExtensionBrowserTest -> InProcessBrowserTest -> content::BrowserTestBase.

[chaopeng, 2017-07-05 21:19:26]

Description was changed from

==========
Super::SetUp must call at the end of SetUp in BrowserTest

For BroswerTest, testcase runs in BrowserTestBase::SetUp, so
Super::SetUp must call at the end of SetUp in BrowserTest.

[1]
https://cs.chromium.org/chromium/src/content/public/test/browser_test_base.cc...

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

to

==========
PlatformAppBrowserTest::SetUp should call at the end of
AppShimInteractiveTest::SetUp()

For BrowserTest, testcase runs in BrowserTest Base::SetUp, so
Super::SetUp must call at the end of SetUp in AppShimInteractiveTest.

[1]
https://cs.chromium.org/chromium/src/content/public/test/browser_test_base.cc...

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

[chaopeng, 2017-07-05 21:19:31]

The CQ bit was checked by chaopeng@chromium.org

[chaopeng, 2017-07-05 21:19:39]

The CQ bit was unchecked by chaopeng@chromium.org

[commit-bot: I haz the power, 2017-07-05 21:19:51]

CQ is trying da patch.

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

[chaopeng, 2017-07-05 21:20:51]

Description was changed from

==========
PlatformAppBrowserTest::SetUp should call at the end of
AppShimInteractiveTest::SetUp()

For BrowserTest, testcase runs in BrowserTest Base::SetUp, so
Super::SetUp must call at the end of SetUp in AppShimInteractiveTest.

[1]
https://cs.chromium.org/chromium/src/content/public/test/browser_test_base.cc...

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

to

==========
PlatformAppBrowserTest::SetUp should call at the end of
AppShimInteractiveTest::SetUp()

For BrowserTest, testcase runs in PlatformAppBrowserTest ->
ExtensionApiTest ->
ExtensionBrowserTest -> InProcessBrowserTest ->
content::BrowserTestBase::SetUp, so
PlatformAppBrowserTest::SetUp must call at the end of SetUp in
AppShimInteractiveTest.

[1]
https://cs.chromium.org/chromium/src/content/public/test/browser_test_base.cc...

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

[chaopeng, 2017-07-05 21:21:16]

The CQ bit was checked by chaopeng@chromium.org

[commit-bot: I haz the power, 2017-07-05 21:21:34]

CQ is trying da patch.

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

[Lei Zhang, 2017-07-05 21:47:32]

On 2017/07/05 18:37:47, Lei Zhang wrote:
> lgtm
> 
> For the CL description:
> 1) There's a typo: BroswerTest
> 2) It's unclear what the "[1]" is suppose to reference. Right now it points to
a
> #if defined() line.
> 3) Can you refer to real class names? There's no class called BrowserTest or
> Super. The inheritance chain is PlatformAppBrowserTest -> ExtensionApiTest ->
> ExtensionBrowserTest -> InProcessBrowserTest -> content::BrowserTestBase.

For (2), now [1] points to line 270, which as of this writing is:

EXPECT_EQ(expected_exit_code_, ContentMain(*GetContentMainParams()));

Are you really trying to point out that line, or that
base::FeatureList::ClearInstanceForTesting() gets called in
BrowserTestBase::SetUp() ? Links to cs.chromium.org probably won't stand up to
the test of time. In a few months, that line will point to some other part of
the code, and future readers who click on that link will see something that is
not what you intended to link to.


I'm guessing what the commit message is really trying to say something like:

AppShimInteractiveTest initializes a ScopedFeatureList in its SetUp() method.
AppShimInteractiveTest ultimately inherits from content::BrowserTestBase, and
BrowserTestBase::SetUp() calls base::FeatureList::ClearInstanceForTesting(). To
make the interaction between these two calls work with an upcoming
ScopedFeatureList change, AppShimInteractiveTest::SetUp() should initialize the
ScopedFeatureList first, and then call its parent class's SetUp() method.

[chaopeng, 2017-07-05 21:57:40]

The CQ bit was unchecked by chaopeng@chromium.org

[chaopeng, 2017-07-05 21:58:56]

On 2017/07/05 21:47:32, Lei Zhang wrote:
> On 2017/07/05 18:37:47, Lei Zhang wrote:
> > lgtm
> > 
> > For the CL description:
> > 1) There's a typo: BroswerTest
> > 2) It's unclear what the "[1]" is suppose to reference. Right now it points
to
> a
> > #if defined() line.
> > 3) Can you refer to real class names? There's no class called BrowserTest or
> > Super. The inheritance chain is PlatformAppBrowserTest -> ExtensionApiTest
->
> > ExtensionBrowserTest -> InProcessBrowserTest -> content::BrowserTestBase.
> 
> For (2), now [1] points to line 270, which as of this writing is:
> 
> EXPECT_EQ(expected_exit_code_, ContentMain(*GetContentMainParams()));
> 
> Are you really trying to point out that line, or that
> base::FeatureList::ClearInstanceForTesting() gets called in
> BrowserTestBase::SetUp() ? Links to http://cs.chromium.org probably won't
stand up to
> the test of time. In a few months, that line will point to some other part of
> the code, and future readers who click on that link will see something that is
> not what you intended to link to.
> 
> 
> I'm guessing what the commit message is really trying to say something like:
> 
> AppShimInteractiveTest initializes a ScopedFeatureList in its SetUp() method.
> AppShimInteractiveTest ultimately inherits from content::BrowserTestBase, and
> BrowserTestBase::SetUp() calls base::FeatureList::ClearInstanceForTesting().
To
> make the interaction between these two calls work with an upcoming
> ScopedFeatureList change, AppShimInteractiveTest::SetUp() should initialize
the
> ScopedFeatureList first, and then call its parent class's SetUp() method.

Yes, the test case is run in ContentMain(*GetContentMainParams()). let me change
the message to clarify.

[Lei Zhang, 2017-07-05 22:03:57]

On 2017/07/05 21:58:56, chaopeng wrote:
> Yes, the test case is run in ContentMain(*GetContentMainParams()). let me
change
> the message to clarify.

Thanks! It's really helpful to have good commit messages, so people who read the
code in the future can understand what happened.

[chaopeng, 2017-07-05 22:17:02]

Description was changed from

==========
PlatformAppBrowserTest::SetUp should call at the end of
AppShimInteractiveTest::SetUp()

For BrowserTest, testcase runs in PlatformAppBrowserTest ->
ExtensionApiTest ->
ExtensionBrowserTest -> InProcessBrowserTest ->
content::BrowserTestBase::SetUp, so
PlatformAppBrowserTest::SetUp must call at the end of SetUp in
AppShimInteractiveTest.

[1]
https://cs.chromium.org/chromium/src/content/public/test/browser_test_base.cc...

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

to

==========
PlatformAppBrowserTest::SetUp should call at the end of
AppShimInteractiveTest::SetUp()

AppShimInteractiveTest initializes a ScopedFeatureList in its SetUp()
method after PlatformAppBrowserTest::SetUp. But testcases run in:

PlatformAppBrowserTest ->
ExtensionApiTest ->
ExtensionBrowserTest -> InProcessBrowserTest ->
content::BrowserTestBase::SetUp[1]

Stack trace:
base::FieldTrialList::~FieldTrialList()
ChromeBrowserMainParts::~ChromeBrowserMainParts()
ChromeBrowserMainPartsMac::~ChromeBrowserMainPartsMac()
content::BrowserMainLoop::~BrowserMainLoop()
content::BrowserMainLoop::~BrowserMainLoop()
content::BrowserMainRunnerImpl::Shutdown()
content::BrowserMain(content::MainFunctionParams const&)
content::RunNamedProcessTypeMain(std::__1::basic_string<char, 
content::MainFunctionParams const&, content::ContentMainDelegate*)
content::ContentMainRunnerImpl::Run()
service_manager::Main(service_manager::MainParams const&)
content::ContentMain(content::ContentMainParams const&)
content::BrowserTestBase::SetUp()
InProcessBrowserTest::SetUp()

In BrowserMainRunnerImpl::Shutdown, we also destruct FieldTrialList,
that is why it hit the memory issue. So we should call
PlatformAppBrowserTest::SetUp call at the end of
AppShimInteractiveTest::SetUp()

[1]
https://cs.chromium.org/chromium/src/content/public/test/browser_test_base.cc...

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

[chaopeng, 2017-07-05 22:44:08]

Description was changed from

==========
PlatformAppBrowserTest::SetUp should call at the end of
AppShimInteractiveTest::SetUp()

AppShimInteractiveTest initializes a ScopedFeatureList in its SetUp()
method after PlatformAppBrowserTest::SetUp. But testcases run in:

PlatformAppBrowserTest ->
ExtensionApiTest ->
ExtensionBrowserTest -> InProcessBrowserTest ->
content::BrowserTestBase::SetUp[1]

Stack trace:
base::FieldTrialList::~FieldTrialList()
ChromeBrowserMainParts::~ChromeBrowserMainParts()
ChromeBrowserMainPartsMac::~ChromeBrowserMainPartsMac()
content::BrowserMainLoop::~BrowserMainLoop()
content::BrowserMainLoop::~BrowserMainLoop()
content::BrowserMainRunnerImpl::Shutdown()
content::BrowserMain(content::MainFunctionParams const&)
content::RunNamedProcessTypeMain(std::__1::basic_string<char, 
content::MainFunctionParams const&, content::ContentMainDelegate*)
content::ContentMainRunnerImpl::Run()
service_manager::Main(service_manager::MainParams const&)
content::ContentMain(content::ContentMainParams const&)
content::BrowserTestBase::SetUp()
InProcessBrowserTest::SetUp()

In BrowserMainRunnerImpl::Shutdown, we also destruct FieldTrialList,
that is why it hit the memory issue. So we should call
PlatformAppBrowserTest::SetUp call at the end of
AppShimInteractiveTest::SetUp()

[1]
https://cs.chromium.org/chromium/src/content/public/test/browser_test_base.cc...

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

to

==========
PlatformAppBrowserTest::SetUp should call at the end of
AppShimInteractiveTest::SetUp()

AppShimInteractiveTest initializes a ScopedFeatureList in its SetUp()
method after PlatformAppBrowserTest::SetUp. But testcases run in:

PlatformAppBrowserTest ->
ExtensionApiTest ->
ExtensionBrowserTest -> InProcessBrowserTest ->
BrowserTestBase::SetUp[1]

BrowserTestBase::SetUp calls ContentMain() to launch the browser and
run testcase and the browser destroys its FieldTrialList and
FeatureList on shutdown.

Stack trace:
base::FieldTrialList::~FieldTrialList()
ChromeBrowserMainParts::~ChromeBrowserMainParts()
ChromeBrowserMainPartsMac::~ChromeBrowserMainPartsMac()
content::BrowserMainLoop::~BrowserMainLoop()
content::BrowserMainLoop::~BrowserMainLoop()
content::BrowserMainRunnerImpl::Shutdown()
content::BrowserMain(content::MainFunctionParams const&)
content::RunNamedProcessTypeMain(std::__1::basic_string<char, 
content::MainFunctionParams const&, content::ContentMainDelegate*)
content::ContentMainRunnerImpl::Run()
service_manager::Main(service_manager::MainParams const&)
content::ContentMain(content::ContentMainParams const&)
content::BrowserTestBase::SetUp()
InProcessBrowserTest::SetUp()


That is why it caused the memory issue. So we should call
PlatformAppBrowserTest::SetUp call at the end of
AppShimInteractiveTest::SetUp()

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

[chaopeng, 2017-07-05 22:44:26]

The CQ bit was checked by chaopeng@chromium.org

[commit-bot: I haz the power, 2017-07-05 22:45:02]

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-07-06 00:28:51]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1499294666836830, "parent_rev":
"2257ee6edf2e07aeae8105ac952ab30722311e63", "commit_rev":
"9d126b1389be2431d6c95eae379f5f0f059089c6"}

[commit-bot: I haz the power, 2017-07-06 00:29:09]

Description was changed from

==========
PlatformAppBrowserTest::SetUp should call at the end of
AppShimInteractiveTest::SetUp()

AppShimInteractiveTest initializes a ScopedFeatureList in its SetUp()
method after PlatformAppBrowserTest::SetUp. But testcases run in:

PlatformAppBrowserTest ->
ExtensionApiTest ->
ExtensionBrowserTest -> InProcessBrowserTest ->
BrowserTestBase::SetUp[1]

BrowserTestBase::SetUp calls ContentMain() to launch the browser and
run testcase and the browser destroys its FieldTrialList and
FeatureList on shutdown.

Stack trace:
base::FieldTrialList::~FieldTrialList()
ChromeBrowserMainParts::~ChromeBrowserMainParts()
ChromeBrowserMainPartsMac::~ChromeBrowserMainPartsMac()
content::BrowserMainLoop::~BrowserMainLoop()
content::BrowserMainLoop::~BrowserMainLoop()
content::BrowserMainRunnerImpl::Shutdown()
content::BrowserMain(content::MainFunctionParams const&)
content::RunNamedProcessTypeMain(std::__1::basic_string<char, 
content::MainFunctionParams const&, content::ContentMainDelegate*)
content::ContentMainRunnerImpl::Run()
service_manager::Main(service_manager::MainParams const&)
content::ContentMain(content::ContentMainParams const&)
content::BrowserTestBase::SetUp()
InProcessBrowserTest::SetUp()


That is why it caused the memory issue. So we should call
PlatformAppBrowserTest::SetUp call at the end of
AppShimInteractiveTest::SetUp()

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

to

==========
PlatformAppBrowserTest::SetUp should call at the end of
AppShimInteractiveTest::SetUp()

AppShimInteractiveTest initializes a ScopedFeatureList in its SetUp()
method after PlatformAppBrowserTest::SetUp. But testcases run in:

PlatformAppBrowserTest ->
ExtensionApiTest ->
ExtensionBrowserTest -> InProcessBrowserTest ->
BrowserTestBase::SetUp[1]

BrowserTestBase::SetUp calls ContentMain() to launch the browser and
run testcase and the browser destroys its FieldTrialList and
FeatureList on shutdown.

Stack trace:
base::FieldTrialList::~FieldTrialList()
ChromeBrowserMainParts::~ChromeBrowserMainParts()
ChromeBrowserMainPartsMac::~ChromeBrowserMainPartsMac()
content::BrowserMainLoop::~BrowserMainLoop()
content::BrowserMainLoop::~BrowserMainLoop()
content::BrowserMainRunnerImpl::Shutdown()
content::BrowserMain(content::MainFunctionParams const&)
content::RunNamedProcessTypeMain(std::__1::basic_string<char, 
content::MainFunctionParams const&, content::ContentMainDelegate*)
content::ContentMainRunnerImpl::Run()
service_manager::Main(service_manager::MainParams const&)
content::ContentMain(content::ContentMainParams const&)
content::BrowserTestBase::SetUp()
InProcessBrowserTest::SetUp()


That is why it caused the memory issue. So we should call
PlatformAppBrowserTest::SetUp call at the end of
AppShimInteractiveTest::SetUp()

BUG=

Review-Url: https://codereview.chromium.org/2963193004
Cr-Commit-Position: refs/heads/master@{#484416}
Committed:
https://chromium.googlesource.com/chromium/src/+/9d126b1389be2431d6c95eae379f...
==========

[commit-bot: I haz the power, 2017-07-06 00:29:10]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/9d126b1389be2431d6c95eae379f...