ToolbarButton: Reset menu runner before deleting its menu's delegate

ToolbarButton: Reset menu runner before deleting its menu's delegate

ToolbarButton::OnMenuClosed() deletes the MenuModelAdapter which is a
delegate of a menu owned |menu_runner_|, but doesn't reset
|menu_runner_|.

Although the menu is closed, but the OS can still send events such as
a11y queries, which would be forwarded to a deleted delegate.

Note that the menu was made asynchronous recently in r381033, but
crashes were collected prior to that since the MenuItemView* was still
retained when the menu was synchronous as well.

BUG=597531
Committed: https://crrev.com/83bac8d9d4508a9cdebe862558db6bdc59173d67
Cr-Commit-Position: refs/heads/master@{#388667}

[tapted, 2016-04-07 03:07:22]

Patchset #3 (id:40001) has been deleted

[tapted, 2016-04-07 05:09:49]

Patchset #4 (id:70005) has been deleted

[tapted, 2016-04-07 06:35:33]

Description was changed from

==========
ToolbarButton: Reset menu runner before deleting its menu's delegate

ToolbarButton::OnMenuClosed() deletes the MenuModelAdapter which is a
delegate of a menu owned |menu_runner_|, but doesn't reset |menu_runner_|.

Although the menu is closed, but the OS can still send events such as
a11y queries, which would be forwarded to a deleted delegate.

BUG=597531
==========

to

==========
ToolbarButton: Reset menu runner before deleting its menu's delegate

ToolbarButton::OnMenuClosed() deletes the MenuModelAdapter which is a
delegate of a menu owned |menu_runner_|, but doesn't reset
|menu_runner_|.

Although the menu is closed, but the OS can still send events such as
a11y queries, which would be forwarded to a deleted delegate.

Note that the menu was made asynchronous recently in r381033, but
crashes were collected prior to that since the MenuItemView* was still
retained when the menu was asyncrhonous as well.

BUG=597531
==========

[tapted, 2016-04-07 06:37:26]

Description was changed from

==========
ToolbarButton: Reset menu runner before deleting its menu's delegate

ToolbarButton::OnMenuClosed() deletes the MenuModelAdapter which is a
delegate of a menu owned |menu_runner_|, but doesn't reset
|menu_runner_|.

Although the menu is closed, but the OS can still send events such as
a11y queries, which would be forwarded to a deleted delegate.

Note that the menu was made asynchronous recently in r381033, but
crashes were collected prior to that since the MenuItemView* was still
retained when the menu was asyncrhonous as well.

BUG=597531
==========

to

==========
ToolbarButton: Reset menu runner before deleting its menu's delegate

ToolbarButton::OnMenuClosed() deletes the MenuModelAdapter which is a
delegate of a menu owned |menu_runner_|, but doesn't reset
|menu_runner_|.

Although the menu is closed, but the OS can still send events such as
a11y queries, which would be forwarded to a deleted delegate.

Note that the menu was made asynchronous recently in r381033, but
crashes were collected prior to that since the MenuItemView* was still
retained when the menu was synchronous as well.

BUG=597531
==========

[tapted, 2016-04-07 11:32:58]

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

[tapted, 2016-04-07 11:32:58]

Hi sky, please take a look. Turns out there was no test coverage, so I added
some \o/. But that turned into an adventure of its own..

https://codereview.chromium.org/1864423002/diff/90001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button_unittest.cc (right):

https://codereview.chromium.org/1864423002/diff/90001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button_unittest.cc:61:
TestingBrowserProcess::DeleteInstance();
This was a frustrating trap to discover :/. I think I can save others from the
same trap, and make this test file a bit simpler. See
http://crrev.com/1865223002 -- if that CL makes sense I'll update the CL here to
be a bit simpler.

[sky, 2016-04-07 19:27:02]

https://codereview.chromium.org/1864423002/diff/90001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button_unittest.cc (right):

https://codereview.chromium.org/1864423002/diff/90001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button_unittest.cc:82:
button_->ShowContextMenuForView(nullptr, gfx::Point(), ui::MENU_SOURCE_MOUSE);
AFAICT this will attempt setcapture and all that fun stuff. I'm a bit worried
that may cause some level of flake. Can you use MenuRunnerHandler here?

[tapted, 2016-04-08 04:06:14]

Patchset #6 (id:130001) has been deleted

[tapted, 2016-04-08 05:32:22]

https://codereview.chromium.org/1864423002/diff/90001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button_unittest.cc (right):

https://codereview.chromium.org/1864423002/diff/90001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button_unittest.cc:82:
button_->ShowContextMenuForView(nullptr, gfx::Point(), ui::MENU_SOURCE_MOUSE);
On 2016/04/07 19:27:01, sky wrote:
> AFAICT this will attempt setcapture and all that fun stuff. I'm a bit worried
> that may cause some level of flake. Can you use MenuRunnerHandler here?

Yep - I agree a flake is quite likely :/. MenuRunnerHandler sadly wasn't very
useful - it can't handle asynchronous menus. I pulled that thread anyway and
ended up deleting MenuRunnerHandler completely and replacing it with a mock
internal::MenuRunnerImplInterface. See what you think.

It feels a little bit contrived - i.e. there's not as much coverage this way.
And I'm not sure there's a nicer way to replace the test-only virtual I added to
ToolbarButton to get the test MenuRunner hooked in (MenuRunnerHandler would have
needed this too).

One option could be to "globally" bump MenuRunner into a test-only mode that
automatically wraps the MenuRunnerImpls it creates behind a TestMenuRunnerImpl.
I would probably need to add to MenuRunnerImplInterface a bit, e.g., to expose
the MenuDelegate to the TestMenuRunnerImpl. Also the approach probably involves
some scary globals and function pointers.

... another option could be just to move this test file into
interactive_ui_tests

WDYT?

[sky, 2016-04-08 16:15:52]

I like moving to interactive_ui_tests.

On Thu, Apr 7, 2016 at 10:32 PM,  <tapted@chromium.org> wrote:
>
>
https://codereview.chromium.org/1864423002/diff/90001/chrome/browser/ui/views...
> File chrome/browser/ui/views/toolbar/toolbar_button_unittest.cc (right):
>
>
https://codereview.chromium.org/1864423002/diff/90001/chrome/browser/ui/views...
> chrome/browser/ui/views/toolbar/toolbar_button_unittest.cc:82:
> button_->ShowContextMenuForView(nullptr, gfx::Point(),
> ui::MENU_SOURCE_MOUSE);
> On 2016/04/07 19:27:01, sky wrote:
>> AFAICT this will attempt setcapture and all that fun stuff. I'm a bit
> worried
>> that may cause some level of flake. Can you use MenuRunnerHandler
> here?
>
> Yep - I agree a flake is quite likely :/. MenuRunnerHandler sadly wasn't
> very useful - it can't handle asynchronous menus. I pulled that thread
> anyway and ended up deleting MenuRunnerHandler completely and replacing
> it with a mock internal::MenuRunnerImplInterface. See what you think.
>
> It feels a little bit contrived - i.e. there's not as much coverage this
> way. And I'm not sure there's a nicer way to replace the test-only
> virtual I added to ToolbarButton to get the test MenuRunner hooked in
> (MenuRunnerHandler would have needed this too).
>
> One option could be to "globally" bump MenuRunner into a test-only mode
> that automatically wraps the MenuRunnerImpls it creates behind a
> TestMenuRunnerImpl. I would probably need to add to
> MenuRunnerImplInterface a bit, e.g., to expose the MenuDelegate to the
> TestMenuRunnerImpl. Also the approach probably involves some scary
> globals and function pointers.
>
> ... another option could be just to move this test file into
> interactive_ui_tests
>
> WDYT?
>
> https://codereview.chromium.org/1864423002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tapted, 2016-04-19 03:31:01]

Patchset #7 (id:170001) has been deleted

[tapted, 2016-04-19 06:23:05]

Patchset #8 (id:210001) has been deleted

[tapted, 2016-04-20 01:43:30]

Hi sky, PTAL

And sorry for the delay - lots of platform context-switches. Also "one does not
simply... add a test harness to Chrome". Apparently.

That is, tests wanting a TestingProfile get a lot of requirements "for free"
while in the unit_tests target, but not while in the interactive_ui_tests
target. (I think it's confusing and would rather test harnesses use things like
ViewsTestHelper as components to properly restrict dependencies just to what
they need.. maybe one day..).

So, in Patchset 9, I played with a helper:

// Performs initialization required for an interactive_ui_test to create a
// TestingProfile. Usually done by ChromeUnitTestSuite when non-interactive.
class UITestingProfileInitializer {
 public:
  UITestingProfileInitializer() {
    ChromeUnitTestSuite::InitializeProviders();
    ChromeUnitTestSuite::InitializeResourceBundle();
  }

 private:
  TestingBrowserProcessInitializer initializer_;

  DISALLOW_COPY_AND_ASSIGN(UITestingProfileInitializer);
};


That worked OK, but I decided to try out ViewsTestBase instead for a bit more
code-sharing with the other interactive ui tests in c/b/ui/views. (That
encountered another obscure issue on Linux, also fixed).

[sky, 2016-04-20 14:37:51]

LGTM

[tapted, 2016-04-21 01:48:45]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2016-04-21 01:49:07]

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

[commit-bot: I haz the power, 2016-04-21 03:02:46]

Description was changed from

==========
ToolbarButton: Reset menu runner before deleting its menu's delegate

ToolbarButton::OnMenuClosed() deletes the MenuModelAdapter which is a
delegate of a menu owned |menu_runner_|, but doesn't reset
|menu_runner_|.

Although the menu is closed, but the OS can still send events such as
a11y queries, which would be forwarded to a deleted delegate.

Note that the menu was made asynchronous recently in r381033, but
crashes were collected prior to that since the MenuItemView* was still
retained when the menu was synchronous as well.

BUG=597531
==========

to

==========
ToolbarButton: Reset menu runner before deleting its menu's delegate

ToolbarButton::OnMenuClosed() deletes the MenuModelAdapter which is a
delegate of a menu owned |menu_runner_|, but doesn't reset
|menu_runner_|.

Although the menu is closed, but the OS can still send events such as
a11y queries, which would be forwarded to a deleted delegate.

Note that the menu was made asynchronous recently in r381033, but
crashes were collected prior to that since the MenuItemView* was still
retained when the menu was synchronous as well.

BUG=597531
==========

[commit-bot: I haz the power, 2016-04-21 03:02:47]

Committed patchset #11 (id:290001)

[commit-bot: I haz the power, 2016-04-22 19:30:34]

Description was changed from

==========
ToolbarButton: Reset menu runner before deleting its menu's delegate

ToolbarButton::OnMenuClosed() deletes the MenuModelAdapter which is a
delegate of a menu owned |menu_runner_|, but doesn't reset
|menu_runner_|.

Although the menu is closed, but the OS can still send events such as
a11y queries, which would be forwarded to a deleted delegate.

Note that the menu was made asynchronous recently in r381033, but
crashes were collected prior to that since the MenuItemView* was still
retained when the menu was synchronous as well.

BUG=597531
==========

to

==========
ToolbarButton: Reset menu runner before deleting its menu's delegate

ToolbarButton::OnMenuClosed() deletes the MenuModelAdapter which is a
delegate of a menu owned |menu_runner_|, but doesn't reset
|menu_runner_|.

Although the menu is closed, but the OS can still send events such as
a11y queries, which would be forwarded to a deleted delegate.

Note that the menu was made asynchronous recently in r381033, but
crashes were collected prior to that since the MenuItemView* was still
retained when the menu was synchronous as well.

BUG=597531

Committed: https://crrev.com/83bac8d9d4508a9cdebe862558db6bdc59173d67
Cr-Commit-Position: refs/heads/master@{#388667}
==========

[commit-bot: I haz the power, 2016-04-22 19:30:36]

Patchset 11 (id:??) landed as
https://crrev.com/83bac8d9d4508a9cdebe862558db6bdc59173d67
Cr-Commit-Position: refs/heads/master@{#388667}