Fixed Issue 141873. Crash when BrowserActionButton get disabled or hidden.

Fixed Issue 141873. Crash when BrowserActionButton get disabled or hidden.

BUG=141873
TEST=Try to disable or hide extension buttons, should not crash.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151148

[sky, 2012-08-10 19:01:27]

http://codereview.chromium.org/10834275/diff/1/chrome/browser/ui/views/browse...
File chrome/browser/ui/views/browser_action_view.cc (left):

http://codereview.chromium.org/10834275/diff/1/chrome/browser/ui/views/browse...
chrome/browser/ui/views/browser_action_view.cc:234: menu_runner_.reset();
Why are you removing this?

http://codereview.chromium.org/10834275/diff/1/chrome/browser/ui/views/browse...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10834275/diff/1/chrome/browser/ui/views/browse...
chrome/browser/ui/views/browser_action_view.cc:70: button_->Destroy();
Why does BrowserActionButton need a Destroy()? Can't it do the cleanup it needs
in ~BrowserActionButton?

[yefimt, 2012-08-10 19:27:05]

http://codereview.chromium.org/10834275/diff/1/chrome/browser/ui/views/browse...
File chrome/browser/ui/views/browser_action_view.cc (left):

http://codereview.chromium.org/10834275/diff/1/chrome/browser/ui/views/browse...
chrome/browser/ui/views/browser_action_view.cc:234: menu_runner_.reset();
On 2012/08/10 19:01:27, sky wrote:
> Why are you removing this?

It is crashing here if BrowserActionButton is deleted by this point. I added
this line in my previous CL because it looked reasonable for me, but it causes
crash now.

http://codereview.chromium.org/10834275/diff/1/chrome/browser/ui/views/browse...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10834275/diff/1/chrome/browser/ui/views/browse...
chrome/browser/ui/views/browser_action_view.cc:70: button_->Destroy();
On 2012/08/10 19:01:27, sky wrote:
> Why does BrowserActionButton need a Destroy()? Can't it do the cleanup it
needs
> in ~BrowserActionButton?

It was the original code before my previous CL, so I just reverting it back to
how it was.

[sky, 2012-08-10 20:07:18]

LGTM for not, but as mentioned more investigation is needed here since it seems
like there may be a more subtle bug involved.

[commit-bot: I haz the power, 2012-08-10 20:10:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yefim@chromium.org/10834275/4002

[commit-bot: I haz the power, 2012-08-10 21:06:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yefim@chromium.org/10834275/4002

[sky, 2012-08-10 23:21:52]

http://codereview.chromium.org/10834275/diff/4002/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10834275/diff/4002/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:433: if (!keybinding_.get() ||
!GetFocusManager())
The way you have the code node GetFocusManager will always return NULL when we
get here from Destroy and not clean things up correctly.

[jam, 2012-08-10 23:54:13]

I reverted this change in r151154 because it broke browser_tests on Windows.

Please wait for the trybots to finish, or use the CQ.