Test AX events fired when opening a select popup.

Test AX events fired when opening a select popup.

BUG=323462
Committed: https://crrev.com/948fbc73b9dc64a748cb5645aada883205f04a19
Cr-Commit-Position: refs/heads/master@{#330305}

[dmazzoni, 2015-05-08 19:45:29]

dmazzoni@chromium.org changed reviewers:
+ sky@chromium.org

[dmazzoni, 2015-05-08 19:45:29]

Scott, could you take a look at this? When I add a browser test that opens a
<select> popup, I get an assertion failure in env.cc:

[20752:29952:0508/123818:873376421:FATAL:env.cc(40)] Check failed: env.
Env::CreateInstance must be called before getting the instance of Env.
Backtrace:
        base::debug::StackTrace::StackTrace [0x07C10581+33]
        logging::LogMessage::~LogMessage [0x07C5BABF+63]
        aura::Env::GetInstance [0x0CF0E365+261]
        wm::ShadowController::Impl::~Impl [0x0D0FBDDC+188]
        wm::ShadowController::Impl::`scalar deleting destructor' [0x0D0FCB06+22]
        base::RefCounted<wm::ShadowController::Impl>::Release [0x0D0FD672+114]

I can try to work around it by closing the popup before the test exits, but I
just wanted to make sure there wasn't a real issue here if the browser process
tries to exit while a popup happens to be open. Plus I'd definitely prefer if I
didn't have to special-case this test and if browser tests could be resilient to
tests that happen to open a popup.

[sky, 2015-05-08 21:11:40]

I share your worry as well. Unfortunately I'm not sure why you're seeing this.
You'll have to dig a bit.

[dmazzoni, 2015-05-13 23:09:22]

OK, I debugged it. Here's what's happening:

* When you pop up a select, the pop up widget is created by
RenderWidgetHostViewAura::InitAsPopup. It's a top-level window.
* Shell::PlatformExit closes all of the browser windows, but the popup doesn't
get closed immediately.
* PlatformExit calls aura::Env::DeleteInstance - now there's no Env
* The popup does get closed inside RunUntilIdle, when the HWND gets destroyed
and we handle the HWND message - but that triggers the assertion failure when
tearing down the popup window because the static Env instance doesn't exist
anymore.

The easiest fix seems to be to call aura::Env::DeleteInstance after
RunUntilIdle. I added Shell::PlatformDestroyEnv, you may have an idea for a
better name.

The only other solution that seemed plausible would be to fix the parts of the
code that assume an Env exists and allow them to tolerate no static Env during
shutdown only. That seems less desirable at first glance but I could go down
that route instead if you prefer.

[sky, 2015-05-13 23:48:34]

On Wed, May 13, 2015 at 4:09 PM,  <dmazzoni@chromium.org> wrote:
> OK, I debugged it. Here's what's happening:
>
> * When you pop up a select, the pop up widget is created by
> RenderWidgetHostViewAura::InitAsPopup. It's a top-level window.
> * Shell::PlatformExit closes all of the browser windows, but the popup
> doesn't
> get closed immediately.

How does PlatformExit close all the browser windows?

> * PlatformExit calls aura::Env::DeleteInstance - now there's no Env
> * The popup does get closed inside RunUntilIdle, when the HWND gets
> destroyed
> and we handle the HWND message - but that triggers the assertion failure
> when
> tearing down the popup window because the static Env instance doesn't exist
> anymore.
>
> The easiest fix seems to be to call aura::Env::DeleteInstance after
> RunUntilIdle. I added Shell::PlatformDestroyEnv, you may have an idea for a
> better name.
>
> The only other solution that seemed plausible would be to fix the parts of
> the
> code that assume an Env exists and allow them to tolerate no static Env
> during
> shutdown only. That seems less desirable at first glance but I could go down
> that route instead if you prefer.
>
>
> https://codereview.chromium.org/1128963004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2015-05-13 23:52:41]

On Wed, May 13, 2015 at 4:48 PM, Scott Violet <sky@chromium.org> wrote:

> On Wed, May 13, 2015 at 4:09 PM,  <dmazzoni@chromium.org> wrote:
> > OK, I debugged it. Here's what's happening:
> >
> > * When you pop up a select, the pop up widget is created by
> > RenderWidgetHostViewAura::InitAsPopup. It's a top-level window.
> > * Shell::PlatformExit closes all of the browser windows, but the popup
> > doesn't
> > get closed immediately.
>
> How does PlatformExit close all the browser windows?
>

Oops, sorry.

Shell::CloseAllWindows iterates over all of the browser windows and closes
them.
Then it calls PlatformExit.


> > * PlatformExit calls aura::Env::DeleteInstance - now there's no Env
> > * The popup does get closed inside RunUntilIdle, when the HWND gets
> > destroyed
> > and we handle the HWND message - but that triggers the assertion failure
> > when
> > tearing down the popup window because the static Env instance doesn't
> exist
> > anymore.
> >
> > The easiest fix seems to be to call aura::Env::DeleteInstance after
> > RunUntilIdle. I added Shell::PlatformDestroyEnv, you may have an idea
> for a
> > better name.
> >
> > The only other solution that seemed plausible would be to fix the parts
> of
> > the
> > code that assume an Env exists and allow them to tolerate no static Env
> > during
> > shutdown only. That seems less desirable at first glance but I could go
> down
> > that route instead if you prefer.
> >
> >
> > https://codereview.chromium.org/1128963004/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-05-14 16:04:57]

What about moving PlatformExit after the
base::MessageLoop::current()->RunUntilIdle() ?

On Wed, May 13, 2015 at 4:52 PM, Dominic Mazzoni <dmazzoni@chromium.org> wrote:
> On Wed, May 13, 2015 at 4:48 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> On Wed, May 13, 2015 at 4:09 PM,  <dmazzoni@chromium.org> wrote:
>> > OK, I debugged it. Here's what's happening:
>> >
>> > * When you pop up a select, the pop up widget is created by
>> > RenderWidgetHostViewAura::InitAsPopup. It's a top-level window.
>> > * Shell::PlatformExit closes all of the browser windows, but the popup
>> > doesn't
>> > get closed immediately.
>>
>> How does PlatformExit close all the browser windows?
>
>
> Oops, sorry.
>
> Shell::CloseAllWindows iterates over all of the browser windows and closes
> them.
> Then it calls PlatformExit.
>
>>
>> > * PlatformExit calls aura::Env::DeleteInstance - now there's no Env
>> > * The popup does get closed inside RunUntilIdle, when the HWND gets
>> > destroyed
>> > and we handle the HWND message - but that triggers the assertion failure
>> > when
>> > tearing down the popup window because the static Env instance doesn't
>> > exist
>> > anymore.
>> >
>> > The easiest fix seems to be to call aura::Env::DeleteInstance after
>> > RunUntilIdle. I added Shell::PlatformDestroyEnv, you may have an idea
>> > for a
>> > better name.
>> >
>> > The only other solution that seemed plausible would be to fix the parts
>> > of
>> > the
>> > code that assume an Env exists and allow them to tolerate no static Env
>> > during
>> > shutdown only. That seems less desirable at first glance but I could go
>> > down
>> > that route instead if you prefer.
>> >
>> >
>> > https://codereview.chromium.org/1128963004/
>>
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to chromium-reviews+unsubscribe@chromium.org.
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2015-05-14 17:21:32]

On 2015/05/14 16:04:57, sky wrote:
> What about moving PlatformExit after the
> base::MessageLoop::current()->RunUntilIdle() ?

Ah yes, that works too! Done.

[sky, 2015-05-14 20:20:37]

LGTM - but I'm not an owner of content

[dmazzoni, 2015-05-14 20:30:45]

dmazzoni@chromium.org changed reviewers:
+ mkwst@chromium.org

[dmazzoni, 2015-05-14 20:30:46]

Thanks!

+mkwst

[Mike West, 2015-05-15 05:02:39]

On 2015/05/14 at 20:30:46, dmazzoni wrote:
> Thanks!
> 
> +mkwst

LGTM, but you'll need someone else for content/browser.

Also, the test failures on Mac and Windows look related.

[dmazzoni, 2015-05-15 18:02:44]

dmazzoni@chromium.org changed reviewers:
+ je_julie.kim@samsung.com

[dmazzoni, 2015-05-15 18:02:44]

Thanks.

+je_julie.kim to review the new test itself now that the fix that makes it not
crash is addressed.

I forgot to add the test expectation files; it should pass now.

[je_julie(Not used), 2015-05-16 09:12:17]

On 2015/05/15 18:02:44, dmazzoni wrote:
> Thanks.
> 
> +je_julie.kim to review the new test itself now that the fix that makes it not
> crash is addressed.
> 
> I forgot to add the test expectation files; it should pass now.

LGTM!

[dmazzoni, 2015-05-17 07:02:46]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-05-17 07:02:47]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1128963004/#ps80001
(title: "Add test expectations")

[commit-bot: I haz the power, 2015-05-17 07:02:56]

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

[commit-bot: I haz the power, 2015-05-17 07:57:46]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-18 11:32:38]

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