Add a (disabled) test to demonstrate child frame creation race.

Add a (disabled) test to demonstrate child frame creation race.

This test demonstrates that browser/ process can encounter a child
frame creation message after its parent/main frame got deleted.

This will hit a check in web_contents_observer_sanity_checker.cc,
specifically:
through WebContentsObserverSanityChecker::RenderFrameHostChange(),
AssertRenderFrameExists(new_host->GetParent()) will fail complaining:
"Check failed: render_frame_created_happened..."

An app's main frame/window is created through chrome.app.window.create().
The main frame creates a child <iframe> in its window.onload handler,
but the test also tries to shutdown inside chrome.app.window.create()'s
creation callback. This exposes the race mentioned above.

BUG=620194
Tests=Without fixing the underlying cause, the test will flake, it
locally fails at least once in every 10 runs. No --site-per-process
or --isolate-extensions flag is necessary:
./out/Debug/browser_tests --gtest_filter=PlatformAppBrowserTest.AppWindowIframe
  --gtest_also_run_disabled_tests

Committed: https://crrev.com/5c038c14402bbb9d73aeae6a8642f1595d2fc75f
Cr-Commit-Position: refs/heads/master@{#400353}

[lazyboy, 2016-06-15 03:16:32]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[lazyboy, 2016-06-15 03:16:33]

See the bug for more context.

[Devlin, 2016-06-15 15:16:53]

On 2016/06/15 03:16:33, lazyboy wrote:
> See the bug for more context.

There doesn't seem to be a linked bug (and the browsertest.cc references
crbug.com/?) :)

Also, I don't see where we try to shutdown inside the create() callback - am I
just missing it?

And, please add a commit title of <=72 chars for git log prettiness :)

[lazyboy, 2016-06-15 18:38:58]

Description was changed from

==========
Add a (disabled) test to demonstrate that browser/ process can encounter
  a child frame creation message after its parent/main frame got deleted.

This will hit a check in web_contents_observer_sanity_checker.cc,
specifically:
through WebContentsObserverSanityChecker::RenderFrameHostChange(),
AssertRenderFrameExists(new_host->GetParent()) will fail complaining:
"Check failed: render_frame_created_happened..."

An app's main frame/window is created through chrome.app.window.create().
The main frame creates a child <iframe> in its window.onload handler,
but the test also tries to shutdown inside chrome.app.window.create()'s
creation callback. This exposes the race mentioned above.

BUG=
Tests=Without fixing the underlying cause, the test will flake, it
locally fails at least once in every 10 runs. No --site-per-process
or --isolate-extensions flag is necessary:
./out/Debug/browser_tests --gtest_filter=PlatformAppBrowserTest.AppWindowIframe
  --gtest_also_run_disabled_tests
==========

to

==========
Add a (disabled) test to demonstrate child frame creation race.

This test demonstrates that browser/ process can encounter a child
frame creation message after its parent/main frame got deleted.

This will hit a check in web_contents_observer_sanity_checker.cc,
specifically:
through WebContentsObserverSanityChecker::RenderFrameHostChange(),
AssertRenderFrameExists(new_host->GetParent()) will fail complaining:
"Check failed: render_frame_created_happened..."

An app's main frame/window is created through chrome.app.window.create().
The main frame creates a child <iframe> in its window.onload handler,
but the test also tries to shutdown inside chrome.app.window.create()'s
creation callback. This exposes the race mentioned above.

BUG=620194
Tests=Without fixing the underlying cause, the test will flake, it
locally fails at least once in every 10 runs. No --site-per-process
or --isolate-extensions flag is necessary:
./out/Debug/browser_tests --gtest_filter=PlatformAppBrowserTest.AppWindowIframe
  --gtest_also_run_disabled_tests
==========

[lazyboy, 2016-06-15 18:43:47]

Bug id attached.

I've added a note about shutdown. Since the test waits for the message being
sent from the callback and it is the last step the test does, the completeness
of the test will start shutting down the app.

[Devlin, 2016-06-15 19:42:33]

https://codereview.chromium.org/2062133005/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/app_window_send_message/test.js
(right):

https://codereview.chromium.org/2062133005/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/app_window_send_message/test.js:8: //
This is the last step the test does and this step will cause the test
I don't think we explicitly shut anything down in LoadAndLaunchPlatformApp(), so
I think this is only happening as part of normal browser test shut down.  I
wonder if we could explicitly shut down the app after the call to
LoadAndLaunchPlatformApp() in the test itself?  This would a) make sure that
we're always testing the same thing and b) *maybe* make the repro more reliable,
since the destruction happens at a deterministic time.

[lazyboy, 2016-06-15 20:08:04]

https://codereview.chromium.org/2062133005/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/app_window_send_message/test.js
(right):

https://codereview.chromium.org/2062133005/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/app_window_send_message/test.js:8: //
This is the last step the test does and this step will cause the test
On 2016/06/15 19:42:32, Devlin wrote:
> I don't think we explicitly shut anything down in LoadAndLaunchPlatformApp(), 
The shutdown is normal test done shutdown here.
so
> I think this is only happening as part of normal browser test shut down.  I
> wonder if we could explicitly shut down the app after the call to
> LoadAndLaunchPlatformApp() in the test itself?  This would a) make sure that
> we're always testing the same thing and b) *maybe* make the repro more
reliable,
> since the destruction happens at a deterministic time.

1)
Do you mean  CloseAppWindow(AppWindow)? I tried this:

const Extension* extension = LoadAndLaunchPlatformApp(..)
AppWindow* window = CreateAppWindow(.., extension);
CloseAppWindow(window);

It seems to not fail/flake at all with this change.

2) I also tried calling CloseAllBrowsers() after LoadAndLaunchPlatformApp,
that seems to always hit "FATAL:profile_destroyer.cc...Profile still has 1
hosts".

[Devlin, 2016-06-15 20:13:44]

https://codereview.chromium.org/2062133005/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/app_window_send_message/test.js
(right):

https://codereview.chromium.org/2062133005/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/app_window_send_message/test.js:8: //
This is the last step the test does and this step will cause the test
On 2016/06/15 20:08:04, lazyboy wrote:
> On 2016/06/15 19:42:32, Devlin wrote:
> > I don't think we explicitly shut anything down in
LoadAndLaunchPlatformApp(), 
> The shutdown is normal test done shutdown here.
> so
> > I think this is only happening as part of normal browser test shut down.  I
> > wonder if we could explicitly shut down the app after the call to
> > LoadAndLaunchPlatformApp() in the test itself?  This would a) make sure that
> > we're always testing the same thing and b) *maybe* make the repro more
> reliable,
> > since the destruction happens at a deterministic time.
> 
> 1)
> Do you mean  CloseAppWindow(AppWindow)? I tried this:
> 
> const Extension* extension = LoadAndLaunchPlatformApp(..)
> AppWindow* window = CreateAppWindow(.., extension);
> CloseAppWindow(window);
> 
> It seems to not fail/flake at all with this change.
> 
> 2) I also tried calling CloseAllBrowsers() after LoadAndLaunchPlatformApp,
> that seems to always hit "FATAL:profile_destroyer.cc...Profile still has 1
> hosts".

The app window should already be created (that's what we do here), so what if
you do something like:
CloseAppWindow(GetFirstAppWindow());
?

[lazyboy, 2016-06-15 20:22:52]

https://codereview.chromium.org/2062133005/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/app_window_send_message/test.js
(right):

https://codereview.chromium.org/2062133005/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/app_window_send_message/test.js:8: //
This is the last step the test does and this step will cause the test
On 2016/06/15 20:13:44, Devlin wrote:
> On 2016/06/15 20:08:04, lazyboy wrote:
> > On 2016/06/15 19:42:32, Devlin wrote:
> > > I don't think we explicitly shut anything down in
> LoadAndLaunchPlatformApp(), 
> > The shutdown is normal test done shutdown here.
> > so
> > > I think this is only happening as part of normal browser test shut down. 
I
> > > wonder if we could explicitly shut down the app after the call to
> > > LoadAndLaunchPlatformApp() in the test itself?  This would a) make sure
that
> > > we're always testing the same thing and b) *maybe* make the repro more
> > reliable,
> > > since the destruction happens at a deterministic time.
> > 
> > 1)
> > Do you mean  CloseAppWindow(AppWindow)? I tried this:
> > 
> > const Extension* extension = LoadAndLaunchPlatformApp(..)
> > AppWindow* window = CreateAppWindow(.., extension);
> > CloseAppWindow(window);
> > 
> > It seems to not fail/flake at all with this change.
> > 
> > 2) I also tried calling CloseAllBrowsers() after LoadAndLaunchPlatformApp,
> > that seems to always hit "FATAL:profile_destroyer.cc...Profile still has 1
> > hosts".
> 
> The app window should already be created (that's what we do here), 
I have seem some test do this (CreateAppWindow) after calling
LoadAndLaunchPlatformApp :\
so what if
> you do something like:
> CloseAppWindow(GetFirstAppWindow());
> ?
No luck with this too, 50 runs didn't fail at all.

[Devlin, 2016-06-15 20:34:44]

https://codereview.chromium.org/2062133005/diff/40001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/app_window_send_message/test.js
(right):

https://codereview.chromium.org/2062133005/diff/40001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/app_window_send_message/test.js:8: //
This is the last step the test does and this step will cause the test
On 2016/06/15 20:22:52, lazyboy wrote:
> No luck with this too, 50 runs didn't fail at all.

Hmm... what destruction flow is the window contents normally going through that
triggers the failure?  Is it something that could happen in the wild, or is it
just indicative of us needing to clean up after ourselves during testing?  (e.g.
closing all app windows in TearDown() or something.)

[lazyboy, 2016-06-15 21:16:23]

On 2016/06/15 20:34:44, Devlin wrote:
>
https://codereview.chromium.org/2062133005/diff/40001/chrome/test/data/extens...
> File chrome/test/data/extensions/platform_apps/app_window_send_message/test.js
> (right):
> 
>
https://codereview.chromium.org/2062133005/diff/40001/chrome/test/data/extens...
> chrome/test/data/extensions/platform_apps/app_window_send_message/test.js:8:
//
> This is the last step the test does and this step will cause the test
> On 2016/06/15 20:22:52, lazyboy wrote:
> > No luck with this too, 50 runs didn't fail at all.
> 
> Hmm... what destruction flow is the window contents normally going through
that
> triggers the failure?  
The AppWindow isn't destroyed for the failures cases (yet), the
WC_observer_sanity
CHECK hits before that happens.

Is it something that could happen in the wild, or is it
> just indicative of us needing to clean up after ourselves during testing? 
(e.g.
> closing all app windows in TearDown() or something.)
That is something we want to figure out as part of the fixing 620194.

Originally, an unrelated test was failing because of this:
https://codereview.chromium.org/2066173002/, so I'm changing that test to
work around this issue. Since I didn't want to miss the one repro we have,
I wanted to document this in crbug and this CL so it is easier to figure
this out down the road. I'm hoping Nick would chime in at some point about
the underlying cause for all of this.

[Devlin, 2016-06-15 22:08:36]

On 2016/06/15 21:16:23, lazyboy wrote:
> On 2016/06/15 20:34:44, Devlin wrote:
> >
>
https://codereview.chromium.org/2062133005/diff/40001/chrome/test/data/extens...
> > File
chrome/test/data/extensions/platform_apps/app_window_send_message/test.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2062133005/diff/40001/chrome/test/data/extens...
> > chrome/test/data/extensions/platform_apps/app_window_send_message/test.js:8:
> //
> > This is the last step the test does and this step will cause the test
> > On 2016/06/15 20:22:52, lazyboy wrote:
> > > No luck with this too, 50 runs didn't fail at all.
> > 
> > Hmm... what destruction flow is the window contents normally going through
> that
> > triggers the failure?  
> The AppWindow isn't destroyed for the failures cases (yet), the
> WC_observer_sanity
> CHECK hits before that happens.
> 
> Is it something that could happen in the wild, or is it
> > just indicative of us needing to clean up after ourselves during testing? 
> (e.g.
> > closing all app windows in TearDown() or something.)
> That is something we want to figure out as part of the fixing 620194.
> 
> Originally, an unrelated test was failing because of this:
> https://codereview.chromium.org/2066173002/, so I'm changing that test to
> work around this issue. Since I didn't want to miss the one repro we have,
> I wanted to document this in crbug and this CL so it is easier to figure
> this out down the road. I'm hoping Nick would chime in at some point about
> the underlying cause for all of this.

Makes sense.  LGTM.

[lazyboy, 2016-06-15 23:09:49]

lazyboy@chromium.org changed reviewers:
+ asargent@chromium.org

[lazyboy, 2016-06-15 23:09:50]

+Antony for chrome/browser/apps/app_browsertest.cc

[asargent_no_longer_on_chrome, 2016-06-15 23:59:58]

lgtm

[lazyboy, 2016-06-16 01:30:00]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2016-06-16 01:30:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/2062133005/#ps80001
(title: "actually disable the test")

[commit-bot: I haz the power, 2016-06-16 01:30:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2062133005/80001

[commit-bot: I haz the power, 2016-06-16 02:33:34]

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

[commit-bot: I haz the power, 2016-06-16 02:33:36]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[lazyboy, 2016-06-17 03:24:05]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2016-06-17 03:24:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2062133005/80001

[commit-bot: I haz the power, 2016-06-17 04:21:16]

Description was changed from

==========
Add a (disabled) test to demonstrate child frame creation race.

This test demonstrates that browser/ process can encounter a child
frame creation message after its parent/main frame got deleted.

This will hit a check in web_contents_observer_sanity_checker.cc,
specifically:
through WebContentsObserverSanityChecker::RenderFrameHostChange(),
AssertRenderFrameExists(new_host->GetParent()) will fail complaining:
"Check failed: render_frame_created_happened..."

An app's main frame/window is created through chrome.app.window.create().
The main frame creates a child <iframe> in its window.onload handler,
but the test also tries to shutdown inside chrome.app.window.create()'s
creation callback. This exposes the race mentioned above.

BUG=620194
Tests=Without fixing the underlying cause, the test will flake, it
locally fails at least once in every 10 runs. No --site-per-process
or --isolate-extensions flag is necessary:
./out/Debug/browser_tests --gtest_filter=PlatformAppBrowserTest.AppWindowIframe
  --gtest_also_run_disabled_tests
==========

to

==========
Add a (disabled) test to demonstrate child frame creation race.

This test demonstrates that browser/ process can encounter a child
frame creation message after its parent/main frame got deleted.

This will hit a check in web_contents_observer_sanity_checker.cc,
specifically:
through WebContentsObserverSanityChecker::RenderFrameHostChange(),
AssertRenderFrameExists(new_host->GetParent()) will fail complaining:
"Check failed: render_frame_created_happened..."

An app's main frame/window is created through chrome.app.window.create().
The main frame creates a child <iframe> in its window.onload handler,
but the test also tries to shutdown inside chrome.app.window.create()'s
creation callback. This exposes the race mentioned above.

BUG=620194
Tests=Without fixing the underlying cause, the test will flake, it
locally fails at least once in every 10 runs. No --site-per-process
or --isolate-extensions flag is necessary:
./out/Debug/browser_tests --gtest_filter=PlatformAppBrowserTest.AppWindowIframe
  --gtest_also_run_disabled_tests
==========

[commit-bot: I haz the power, 2016-06-17 04:21:17]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-17 04:22:44]

Description was changed from

==========
Add a (disabled) test to demonstrate child frame creation race.

This test demonstrates that browser/ process can encounter a child
frame creation message after its parent/main frame got deleted.

This will hit a check in web_contents_observer_sanity_checker.cc,
specifically:
through WebContentsObserverSanityChecker::RenderFrameHostChange(),
AssertRenderFrameExists(new_host->GetParent()) will fail complaining:
"Check failed: render_frame_created_happened..."

An app's main frame/window is created through chrome.app.window.create().
The main frame creates a child <iframe> in its window.onload handler,
but the test also tries to shutdown inside chrome.app.window.create()'s
creation callback. This exposes the race mentioned above.

BUG=620194
Tests=Without fixing the underlying cause, the test will flake, it
locally fails at least once in every 10 runs. No --site-per-process
or --isolate-extensions flag is necessary:
./out/Debug/browser_tests --gtest_filter=PlatformAppBrowserTest.AppWindowIframe
  --gtest_also_run_disabled_tests
==========

to

==========
Add a (disabled) test to demonstrate child frame creation race.

This test demonstrates that browser/ process can encounter a child
frame creation message after its parent/main frame got deleted.

This will hit a check in web_contents_observer_sanity_checker.cc,
specifically:
through WebContentsObserverSanityChecker::RenderFrameHostChange(),
AssertRenderFrameExists(new_host->GetParent()) will fail complaining:
"Check failed: render_frame_created_happened..."

An app's main frame/window is created through chrome.app.window.create().
The main frame creates a child <iframe> in its window.onload handler,
but the test also tries to shutdown inside chrome.app.window.create()'s
creation callback. This exposes the race mentioned above.

BUG=620194
Tests=Without fixing the underlying cause, the test will flake, it
locally fails at least once in every 10 runs. No --site-per-process
or --isolate-extensions flag is necessary:
./out/Debug/browser_tests --gtest_filter=PlatformAppBrowserTest.AppWindowIframe
  --gtest_also_run_disabled_tests

Committed: https://crrev.com/5c038c14402bbb9d73aeae6a8642f1595d2fc75f
Cr-Commit-Position: refs/heads/master@{#400353}
==========

[commit-bot: I haz the power, 2016-06-17 04:22:45]

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