Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13674016:
alternate ntp: fix crash in ContentsContainer destructor (Closed)
|
Created:
7 years, 8 months ago by kuan Modified:
7 years, 8 months ago Reviewers:
sky CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionalternate ntp: fix crash in ContentsContainer destructor
- also added preventive coding around registering for notification.
BUG=226128
TEST=verify per bug rpt
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192831
Patch Set 1 #
Total comments: 7
Patch Set 2 : addressed scott's comments #
Total comments: 2
Patch Set 3 : addressed scott's comments #Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/13674016/diff/1/chrome/browser/ui/views/frame... File chrome/browser/ui/views/frame/contents_container.cc (right): https://codereview.chromium.org/13674016/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/contents_container.cc:163: registrar_.Add(this, Can we removeAll right here too? https://codereview.chromium.org/13674016/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/contents_container.cc:278: (content::Source<content::RenderWidgetHost>(source)).ptr() == Shouldn't this be a DCHECK?
https://codereview.chromium.org/13674016/diff/1/chrome/browser/ui/views/frame... File chrome/browser/ui/views/frame/contents_container.cc (right): https://codereview.chromium.org/13674016/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/contents_container.cc:163: registrar_.Add(this, On 2013/04/05 19:44:17, sky wrote: > Can we removeAll right here too? i already RemoveAll @151, is that enough? https://codereview.chromium.org/13674016/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/contents_container.cc:278: (content::Source<content::RenderWidgetHost>(source)).ptr() == On 2013/04/05 19:44:17, sky wrote: > Shouldn't this be a DCHECK? this is the dilemma i have: @159, GetRenderViewHost could return NULL; since my DCHECK is a nop in release mode, i'll registering for all sources in that case. i have 2 options: a) register even if rvh is NULL, then i need to keep the if cond here. b) only register if rvh is not NULL, then i can change this to DCHECK. i'm not sure which option is better.
https://codereview.chromium.org/13674016/diff/1/chrome/browser/ui/views/frame... File chrome/browser/ui/views/frame/contents_container.cc (right): https://codereview.chromium.org/13674016/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/contents_container.cc:163: registrar_.Add(this, On 2013/04/05 21:54:11, kuan wrote: > On 2013/04/05 19:44:17, sky wrote: > > Can we removeAll right here too? > > i already RemoveAll @151, is that enough? Yes, that's fine. https://codereview.chromium.org/13674016/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/contents_container.cc:278: (content::Source<content::RenderWidgetHost>(source)).ptr() == On 2013/04/05 21:54:11, kuan wrote: > On 2013/04/05 19:44:17, sky wrote: > > Shouldn't this be a DCHECK? > > this is the dilemma i have: > @159, GetRenderViewHost could return NULL; since my DCHECK is a nop in release > mode, i'll registering for all sources in that case. i have 2 options: > a) register even if rvh is NULL, then i need to keep the if cond here. > b) only register if rvh is not NULL, then i can change this to DCHECK. > > i'm not sure which option is better. Only register if rvh is non-null.
i've addressed all comments in patch set 1. ptal. thx. https://codereview.chromium.org/13674016/diff/1/chrome/browser/ui/views/frame... File chrome/browser/ui/views/frame/contents_container.cc (right): https://codereview.chromium.org/13674016/diff/1/chrome/browser/ui/views/frame... chrome/browser/ui/views/frame/contents_container.cc:278: (content::Source<content::RenderWidgetHost>(source)).ptr() == On 2013/04/05 23:00:08, sky wrote: > On 2013/04/05 21:54:11, kuan wrote: > > On 2013/04/05 19:44:17, sky wrote: > > > Shouldn't this be a DCHECK? > > > > this is the dilemma i have: > > @159, GetRenderViewHost could return NULL; since my DCHECK is a nop in release > > mode, i'll registering for all sources in that case. i have 2 options: > > a) register even if rvh is NULL, then i need to keep the if cond here. > > b) only register if rvh is not NULL, then i can change this to DCHECK. > > > > i'm not sure which option is better. > > Only register if rvh is non-null. Done.
https://codereview.chromium.org/13674016/diff/6001/chrome/browser/ui/views/fr... File chrome/browser/ui/views/frame/contents_container.cc (right): https://codereview.chromium.org/13674016/diff/6001/chrome/browser/ui/views/fr... chrome/browser/ui/views/frame/contents_container.cc:279: if (overlay_ && !draw_drop_shadow_ && overlay_web_contents_) { overlay_web_contents_ should be a DCHECK, right?
https://codereview.chromium.org/13674016/diff/6001/chrome/browser/ui/views/fr... File chrome/browser/ui/views/frame/contents_container.cc (right): https://codereview.chromium.org/13674016/diff/6001/chrome/browser/ui/views/fr... chrome/browser/ui/views/frame/contents_container.cc:279: if (overlay_ && !draw_drop_shadow_ && overlay_web_contents_) { On 2013/04/05 23:12:40, sky wrote: > overlay_web_contents_ should be a DCHECK, right? Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kuan@chromium.org/13674016/9001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kuan@chromium.org/13674016/9001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kuan@chromium.org/13674016/9001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kuan@chromium.org/13674016/9001
Message was sent while issue was closed.
Change committed as 192831 |
