Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(873)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 10826262: Fixed Issue 141873. Crash when BrowserActionButton get disabled or hidden. (Closed)

Created:
8 years, 4 months ago by yefimt
Modified:
8 years, 4 months ago
Reviewers:
yefim, sky
CC:
chromium-reviews, tfarina
Visibility:
Public.

Description

Fixed Issue 141873. Crash when BrowserActionButton get disabled or hidden. BUG=141873 TEST=Try to disable or hide extension buttons, should not crash. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151550

Patch Set 1 : Fixed Issue 141873 #

Patch Set 2 : Fixed Issue 141873 #

Patch Set 3 : Fixed Issue 141873 #

Total comments: 4

Patch Set 4 : Fixed Issue 141873 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -13 lines) Patch
M chrome/browser/ui/views/browser_action_view.h View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/browser_action_view.cc View 1 2 3 3 chunks +12 lines, -11 lines 0 comments Download

Messages

Total messages: 17 (0 generated)
sky
LGTM
8 years, 4 months ago (2012-08-13 16:47:48 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yefim@chromium.org/10826262/7004
8 years, 4 months ago (2012-08-13 16:48:32 UTC) #2
commit-bot: I haz the power
Try job failure for 10826262-7004 (retry) (retry) on win_rel for step "browser_tests". It's a second ...
8 years, 4 months ago (2012-08-13 20:23:51 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yefim@chromium.org/10826262/7004
8 years, 4 months ago (2012-08-13 20:41:12 UTC) #4
sky
http://codereview.chromium.org/10826262/diff/4007/chrome/browser/ui/views/browser_action_view.cc File chrome/browser/ui/views/browser_action_view.cc (left): http://codereview.chromium.org/10826262/diff/4007/chrome/browser/ui/views/browser_action_view.cc#oldcode234 chrome/browser/ui/views/browser_action_view.cc:234: menu_runner_.reset(); This should stay. http://codereview.chromium.org/10826262/diff/4007/chrome/browser/ui/views/browser_action_view.cc File chrome/browser/ui/views/browser_action_view.cc (right): http://codereview.chromium.org/10826262/diff/4007/chrome/browser/ui/views/browser_action_view.cc#newcode103 ...
8 years, 4 months ago (2012-08-13 23:48:37 UTC) #5
yefimt
http://codereview.chromium.org/10826262/diff/4007/chrome/browser/ui/views/browser_action_view.cc File chrome/browser/ui/views/browser_action_view.cc (left): http://codereview.chromium.org/10826262/diff/4007/chrome/browser/ui/views/browser_action_view.cc#oldcode234 chrome/browser/ui/views/browser_action_view.cc:234: menu_runner_.reset(); On 2012/08/13 23:48:37, sky wrote: > This should ...
8 years, 4 months ago (2012-08-13 23:56:08 UTC) #6
sky
LGTM
8 years, 4 months ago (2012-08-13 23:56:56 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yefim@chromium.org/10826262/9006
8 years, 4 months ago (2012-08-13 23:59:07 UTC) #8
commit-bot: I haz the power
Try job failure for 10826262-9006 (retry) on win_rel for step "content_unittests". It's a second try, ...
8 years, 4 months ago (2012-08-14 02:17:06 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yefim@chromium.org/10826262/9006
8 years, 4 months ago (2012-08-14 04:15:39 UTC) #10
commit-bot: I haz the power
Try job failure for 10826262-9006 (retry) on linux_chromeos for steps "browser_tests, content_browsertests". It's a second ...
8 years, 4 months ago (2012-08-14 08:37:50 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yefim@chromium.org/10826262/9006
8 years, 4 months ago (2012-08-14 17:52:04 UTC) #12
commit-bot: I haz the power
Change committed as 151550
8 years, 4 months ago (2012-08-14 20:10:13 UTC) #13
tfarina
On Monday, August 13, 2012, wrote: > Reviewers: sky, > > > http://codereview.chromium.**org/10826262/diff/4007/chrome/** > browser/ui/views/browser_**action_view.cc<http://codereview.chromium.org/10826262/diff/4007/chrome/browser/ui/views/browser_action_view.cc> ...
8 years, 4 months ago (2012-08-15 01:10:59 UTC) #14
sky
On Tue, Aug 14, 2012 at 6:10 PM, Thiago Farina <tfarina@chromium.org> wrote: > > > ...
8 years, 4 months ago (2012-08-15 15:06:21 UTC) #15
yefim
The problem was that BAV gets deleted from menu, so button could be deleted before ...
8 years, 4 months ago (2012-08-16 01:46:49 UTC) #16
sky
8 years, 4 months ago (2012-08-16 02:39:29 UTC) #17 
As commented though, MenuRunner is suppose to handle this case. That's
what bothers me about your patch.

  -Scott

On Wed, Aug 15, 2012 at 6:46 PM, Yefim Tetelman <yefim@google.com> wrote:
> The problem was that BAV gets deleted from menu, so button could be deleted
> before menu exits.
> So making BAV button owner allows to postpone button deletion.
>
> On Tue, Aug 14, 2012 at 6:10 PM, Thiago Farina <tfarina@chromium.org> wrote:
>>
>>
>>
>> On Monday, August 13, 2012, wrote:
>>>
>>> Reviewers: sky,
>>>
>>>
>>>
>>>
http://codereview.chromium.org/10826262/diff/4007/chrome/browser/ui/views/bro...
>>> File chrome/browser/ui/views/browser_action_view.cc (left):
>>>
>>>
>>>
http://codereview.chromium.org/10826262/diff/4007/chrome/browser/ui/views/bro...
>>> chrome/browser/ui/views/browser_action_view.cc:234:
>>> menu_runner_.reset();
>>> On 2012/08/13 23:48:37, sky wrote:
>>>>
>>>> This should stay.
>>>
>>>
>>> Done, but I looked how other code does it and nobody reset pointer,
>>> including menu sample :)
>>>
>>>
>>>
http://codereview.chromium.org/10826262/diff/4007/chrome/browser/ui/views/bro...
>>> File chrome/browser/ui/views/browser_action_view.cc (right):
>>>
>>>
>>>
http://codereview.chromium.org/10826262/diff/4007/chrome/browser/ui/views/bro...
>>> chrome/browser/ui/views/browser_action_view.cc:103: if (is_add && (child
>>> == this)) {
>>> On 2012/08/13 23:48:37, sky wrote:
>>>>
>>>> Can you also add GetWidget() != NULL && button_ == NULL
>>>
>>>
>>> Done.
>>>
>>> Description:
>>> Fixed Issue 141873. Crash when BrowserActionButton get disabled or
>>> hidden.
>>>
>>> BUG=141873
>>> TEST=Try to disable or hide extension buttons, should not crash.
>>>
>>>
>>> Please review this at http://codereview.chromium.org/10826262/
>>>
>>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>>
>>> Affected files:
>>>   M chrome/browser/ui/views/browser_action_view.h
>>>   M chrome/browser/ui/views/browser_action_view.cc
>>>
>>>
>>> Index: chrome/browser/ui/views/browser_action_view.cc
>>> diff --git a/chrome/browser/ui/views/browser_action_view.cc
>>> b/chrome/browser/ui/views/browser_action_view.cc
>>> index
>>>
f4275e6ecce2a2a2f2f753ce2bb2b23b576b4c11..787a35091252dda316409a60abd5f578b1c59d46
>>> 100644
>>> --- a/chrome/browser/ui/views/browser_action_view.cc
>>> +++ b/chrome/browser/ui/views/browser_action_view.cc
>>> @@ -62,6 +62,7 @@ BrowserActionView::BrowserActionView(const Extension*
>>> extension,
>>>  }
>>>
>>>  BrowserActionView::~BrowserActionView() {
>>> +  button_->Destroy();
>>>  }
>>>
>>>  gfx::Canvas* BrowserActionView::GetIconWithBadge() {
>>> @@ -82,7 +83,7 @@ gfx::Canvas* BrowserActionView::GetIconWithBadge() {
>>>
>>>    return canvas;
>>>  }
>>> -
>>> +
>>>  void BrowserActionView::Layout() {
>>>    // We can't rely on button_->GetPreferredSize() here because that's
>>> not set
>>>    // correctly until the first call to
>>> @@ -96,16 +97,16 @@ void BrowserActionView::Layout() {
>>>                       BrowserActionsContainer::IconHeight());
>>>  }
>>>
>>> -void BrowserActionView::ViewHierarchyChanged(bool is_add,
>>> -                                             View* parent,
>>> -                                             View* child) {
>>> -  if (is_add && (child == this)) {
>>> -    button_ = new BrowserActionButton(extension_, browser_, delegate_);
>>> -    button_->set_drag_controller(delegate_);
>>> -
>>> -    AddChildView(button_);
>>> -    button_->UpdateState();
>>> -  }
>>> +void BrowserActionView::ViewHierarchyChanged(bool is_add,
>>> +                                             View* parent,
>>> +                                             View* child) {
>>> +  if (is_add && (child == this) && (GetWidget() != NULL) && (button_ ==
>>> NULL)) {
>>> +    button_ = new BrowserActionButton(extension_, browser_, delegate_);
>>> +    button_->set_drag_controller(delegate_);
>>> +    button_->set_owned_by_client();
>>
>> Wait, is this necessary? If so, why? This tells views framework that the
>> client is now responsible for managing this memory. So BrowserActionView is
>> now the owner of button_ and responsible for deleting it when his done with
>> it. Where we delete button_? It looks like a raw pointer to me.
>>
>>> +    AddChildView(button_);
>>> +    button_->UpdateState();
>>> +  }
>>>  }
>>>
>>>  void BrowserActionView::GetAccessibleState(ui::AccessibleViewState*
>>> state) {
>>> Index: chrome/browser/ui/views/browser_action_view.h
>>> diff --git a/chrome/browser/ui/views/browser_action_view.h
>>> b/chrome/browser/ui/views/browser_action_view.h
>>> index
>>>
528278d8b6917a0fbfca958fea3c72c04631fd51..2585c824007853a0c2809269835cb94ea886e516
>>> 100644
>>> --- a/chrome/browser/ui/views/browser_action_view.h
>>> +++ b/chrome/browser/ui/views/browser_action_view.h
>>> @@ -77,8 +77,8 @@ class BrowserActionView : public views::View {
>>>   protected:
>>>    // Overridden from views::View to paint the badge on top of children.
>>>    virtual void PaintChildren(gfx::Canvas* canvas) OVERRIDE;
>>> -  virtual void ViewHierarchyChanged(bool is_add,
>>> -                                    View* parent,
>>> +  virtual void ViewHierarchyChanged(bool is_add,
>>> +                                    View* parent,
>>>                                      View* child) OVERRIDE;
>>>
>>>   private:
>>>
>>>
>>
>>
>> --
>> Thiago
>
>

Powered by Google App Engine
This is Rietveld 408576698