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

Issue 7796010: Attempt at fixing crash in menus shown from infobars. Here's what the (Closed)

Created:
9 years, 3 months ago by sky
Modified:
9 years, 3 months ago
Reviewers:
Peter Kasting
CC:
chromium-reviews, Aaron Boodman, Erik does not do reviews, mihaip+watch_chromium.org
Visibility:
Public.

Description

Attempt at fixing crash in menus shown from infobars. Here's what the current code does when closing an infobar: . The animation ends, resulting in a delayed deletion of InfoBarView (InfoBarContainerView::PlatformSpecificRemoveInfoBar). . The InfoBarDelegate deletes itself (InfoBarDelegate::InfoBarClosed). . Eventually the InfoBarView is deleted. This leaves a window of time between which the view is alive, but the delegate has been deleted. The view doesn't directly reference the delegate anymore, but the menu models created by the infobarviews do. This means if a paint comes in to the menu it's going to query the deleted delegate and we crash. I made CancelMenu pure virtual in hopes of avoiding this in the future by making subclasses think about it. BUG=93314 TEST=make sure menus on infobars still work. R=pkasting@chromium.org Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101958

Patch Set 1 #

Total comments: 2

Patch Set 2 : Incorporate review feedback #

Patch Set 3 : Promote more code #

Patch Set 4 : Change comment and remove change to views #

Total comments: 4

Patch Set 5 : ignore return value #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+98 lines, -66 lines) Patch
M chrome/browser/ui/views/infobars/after_translate_infobar.h View 1 3 chunks +10 lines, -10 lines 0 comments Download
M chrome/browser/ui/views/infobars/after_translate_infobar.cc View 1 2 3 4 2 chunks +8 lines, -11 lines 0 comments Download
M chrome/browser/ui/views/infobars/before_translate_infobar.h View 1 3 chunks +9 lines, -9 lines 0 comments Download
M chrome/browser/ui/views/infobars/before_translate_infobar.cc View 1 2 3 4 2 chunks +7 lines, -10 lines 0 comments Download
M chrome/browser/ui/views/infobars/confirm_infobar.h View 1 1 chunk +7 lines, -4 lines 0 comments Download
M chrome/browser/ui/views/infobars/extension_infobar.h View 1 3 chunks +5 lines, -6 lines 0 comments Download
M chrome/browser/ui/views/infobars/extension_infobar.cc View 1 2 3 4 2 chunks +3 lines, -10 lines 0 comments Download
M chrome/browser/ui/views/infobars/infobar_view.h View 1 2 3 4 4 chunks +14 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/infobars/infobar_view.cc View 1 2 3 4 3 chunks +24 lines, -0 lines 1 comment Download
M chrome/browser/ui/views/infobars/link_infobar.h View 1 1 chunk +4 lines, -2 lines 0 comments Download
chrome/browser/ui/views/infobars/translate_message_infobar.h View 1 1 chunk +7 lines, -4 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
sky
An alternative fix for this is to delete the infobarview immediately, but I'm assuming the ...
9 years, 3 months ago (2011-09-09 19:21:43 UTC) #1
Peter Kasting
http://codereview.chromium.org/7796010/diff/1/chrome/browser/ui/views/infobars/infobar_view.cc File chrome/browser/ui/views/infobars/infobar_view.cc (right): http://codereview.chromium.org/7796010/diff/1/chrome/browser/ui/views/infobars/infobar_view.cc#newcode214 chrome/browser/ui/views/infobars/infobar_view.cc:214: // We're being removed. Cancel any menus we may ...
9 years, 3 months ago (2011-09-12 18:06:00 UTC) #2
sky
I promoted SetMenuRunner to InfoBarView and moved the description to PlatformSpecificHide. -Scott
9 years, 3 months ago (2011-09-12 22:33:02 UTC) #3
sky
I promoted more code to InfoBarView and changed the alignment of the translate menus to ...
9 years, 3 months ago (2011-09-14 21:20:20 UTC) #4
Peter Kasting
Based on the nits below, it looks like no one actually cares about RunMenuAt()'s return ...
9 years, 3 months ago (2011-09-15 21:43:12 UTC) #5
sky
On Thu, Sep 15, 2011 at 2:43 PM, <pkasting@chromium.org> wrote: > Based on the nits ...
9 years, 3 months ago (2011-09-15 22:29:48 UTC) #6
sky
Nuked return value and added comments. Want to take another look? -Scott
9 years, 3 months ago (2011-09-16 18:00:51 UTC) #7
Peter Kasting
9 years, 3 months ago (2011-09-19 19:08:14 UTC) #8
LGTM

http://codereview.chromium.org/7796010/diff/17001/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/infobar_view.cc (right):

http://codereview.chromium.org/7796010/diff/17001/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/infobar_view.cc:302: // closed.
Nit: This comment is a bit confusing, partly because "here" seemingly doesn't
mean "where the comment is" but rather "after the call returns".

Powered by Google App Engine
This is Rietveld 408576698