Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6380005:
Implement RenderWidgetHostViewViews::Destroy. See RWHVGtk for reference. (Closed)
|
Created:
9 years, 11 months ago by Alex Nicolaou Modified:
9 years ago CC:
chromium-reviews, darin-cc_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement RenderWidgetHostViewViews::Destroy. See RWHVGtk for reference.
TEST=manually, trybots
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71968
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fix destruction to work as in gtk. #
Total comments: 1
Patch Set 3 : Removed unnecessary and impossible call from the destructor. #Messages
Total messages: 13 (0 generated)
There's really nothing to do here yet as this code path has no equivalent of a server grab for popups. PTAL. alex
http://codereview.chromium.org/6380005/diff/1/chrome/browser/renderer_host/re... File chrome/browser/renderer_host/render_widget_host_view_views.cc (right): http://codereview.chromium.org/6380005/diff/1/chrome/browser/renderer_host/re... chrome/browser/renderer_host/render_widget_host_view_views.cc:297: host_ = NULL; You don't need the DeleteSoon call that RWHVGtk has?
No, views get deleted somewhere else though for the life of me I can't remember who does it. Scott will probably know, or tell me I'm wrong :) alex On Wed, Jan 19, 2011 at 1:13 PM, <bryeung@chromium.org> wrote: > > > http://codereview.chromium.org/6380005/diff/1/chrome/browser/renderer_host/re... > File chrome/browser/renderer_host/render_widget_host_view_views.cc > (right): > > > http://codereview.chromium.org/6380005/diff/1/chrome/browser/renderer_host/re... > chrome/browser/renderer_host/render_widget_host_view_views.cc:297: host_ > = NULL; > You don't need the DeleteSoon call that RWHVGtk has? > > > http://codereview.chromium.org/6380005/ >
I'm not sure there either. The windows side does a DeleteWindow, which deletes the RWHV. Did you set a breakpoint to make sure ~RWHVV is getting hit? -Scott On 2011/01/19 18:27:47, Alex Nicolaou wrote: > No, views get deleted somewhere else though for the life of me I can't > remember who does it. Scott will probably know, or tell me I'm wrong :) > > alex > > On Wed, Jan 19, 2011 at 1:13 PM, <mailto:bryeung@chromium.org> wrote: > > > > > > > > http://codereview.chromium.org/6380005/diff/1/chrome/browser/renderer_host/re... > > File chrome/browser/renderer_host/render_widget_host_view_views.cc > > (right): > > > > > > > http://codereview.chromium.org/6380005/diff/1/chrome/browser/renderer_host/re... > > chrome/browser/renderer_host/render_widget_host_view_views.cc:297: host_ > > = NULL; > > You don't need the DeleteSoon call that RWHVGtk has? > > > > > > http://codereview.chromium.org/6380005/ > >
In View::~View it loops through all the child views and destroys them, so the rwhvv gets destroyed when the tcvv gets destroyed. However I thought we were also supposed to destroy the view on domain transitions, but that's not working for me. Are we supposed to be tossing only the renderer process but keeping the rwhvv, or should we be destroying the view as well? When I navigate away from the NTP I do see a case where Destroy is called, but the rwhvv may not be deleted. alex On Wed, Jan 19, 2011 at 2:23 PM, <sky@chromium.org> wrote: > I'm not sure there either. The windows side does a DeleteWindow, which > deletes > the RWHV. Did you set a breakpoint to make sure ~RWHVV is getting hit? > > -Scott > > > On 2011/01/19 18:27:47, Alex Nicolaou wrote: > >> No, views get deleted somewhere else though for the life of me I can't >> remember who does it. Scott will probably know, or tell me I'm wrong :) >> > > alex >> > > On Wed, Jan 19, 2011 at 1:13 PM, <mailto:bryeung@chromium.org> wrote: >> > > > >> > >> > >> > > > http://codereview.chromium.org/6380005/diff/1/chrome/browser/renderer_host/re... > >> > File chrome/browser/renderer_host/render_widget_host_view_views.cc >> > (right): >> > >> > >> > >> > > > http://codereview.chromium.org/6380005/diff/1/chrome/browser/renderer_host/re... > >> > chrome/browser/renderer_host/render_widget_host_view_views.cc:297: host_ >> > = NULL; >> > You don't need the DeleteSoon call that RWHVGtk has? >> > >> > >> > http://codereview.chromium.org/6380005/ >> > >> > > > > http://codereview.chromium.org/6380005/ >
I looked a little more and in the case of transitioning away from the NTP the rwhvv isn't removed from the tcvv and it gets destroyed later when the tcvv is destroyed. This isn't right. I will try to fix it by just removing the rwhvv from its parent when Destroy() is called but while I'm off doing that if someone can tell me why the deletion is done on the message loop that might save me some pain, because presumably if I delete it immediately here someone will still refer to it after deletion. alex On Wed, Jan 19, 2011 at 2:52 PM, Alex Nicolaou <anicolao@chromium.org>wrote: > In View::~View it loops through all the child views and destroys them, so > the rwhvv gets destroyed when the tcvv gets destroyed. > > However I thought we were also supposed to destroy the view on domain > transitions, but that's not working for me. Are we supposed to be tossing > only the renderer process but keeping the rwhvv, or should we be destroying > the view as well? When I navigate away from the NTP I do see a case where > Destroy is called, but the rwhvv may not be deleted. > > alex > > > On Wed, Jan 19, 2011 at 2:23 PM, <sky@chromium.org> wrote: > >> I'm not sure there either. The windows side does a DeleteWindow, which >> deletes >> the RWHV. Did you set a breakpoint to make sure ~RWHVV is getting hit? >> >> -Scott >> >> >> On 2011/01/19 18:27:47, Alex Nicolaou wrote: >> >>> No, views get deleted somewhere else though for the life of me I can't >>> remember who does it. Scott will probably know, or tell me I'm wrong :) >>> >> >> alex >>> >> >> On Wed, Jan 19, 2011 at 1:13 PM, <mailto:bryeung@chromium.org> wrote: >>> >> >> > >>> > >>> > >>> >> >> >> http://codereview.chromium.org/6380005/diff/1/chrome/browser/renderer_host/re... >> >>> > File chrome/browser/renderer_host/render_widget_host_view_views.cc >>> > (right): >>> > >>> > >>> > >>> >> >> >> http://codereview.chromium.org/6380005/diff/1/chrome/browser/renderer_host/re... >> >>> > chrome/browser/renderer_host/render_widget_host_view_views.cc:297: >>> host_ >>> > = NULL; >>> > You don't need the DeleteSoon call that RWHVGtk has? >>> > >>> > >>> > http://codereview.chromium.org/6380005/ >>> > >>> >> >> >> >> http://codereview.chromium.org/6380005/ >> > >
@sky PTAL @gspencer Your patch to send a message is potentially sending it to a destroyed object. Is this code in the right place?
LGTM
http://codereview.chromium.org/6380005/diff/9001/chrome/browser/renderer_host... File chrome/browser/renderer_host/render_widget_host_view_views.cc (right): http://codereview.chromium.org/6380005/diff/9001/chrome/browser/renderer_host... chrome/browser/renderer_host/render_widget_host_view_views.cc:134: RenderViewGone(base::TERMINATION_STATUS_NORMAL_TERMINATION, Hmm. I can see what you mean, and in fact this is a bad idea: RenderViewGone is a virtual function, and we shouldn't be calling it in the destructor. I didn't actually add that, however, you added it in: http://src.chromium.org/viewvc/chrome?view=rev&revision=64015 (I just added some parameters to the call in my patch) I'm wondering if we should be calling this in "Destroy" instead? (But I'm not sure that Destroy is reliably called for every object.)
Wow I have no recollection of that patch. But yes I think calling this in destroy is OK and destroy is called pretty reliably. At least, doing this in destroy would be better than it is right now. I'll move this in a follow-up patch. alex On Wed, Jan 19, 2011 at 4:59 PM, <gspencer@chromium.org> wrote: > > > http://codereview.chromium.org/6380005/diff/9001/chrome/browser/renderer_host... > > File chrome/browser/renderer_host/render_widget_host_view_views.cc > (right): > > > http://codereview.chromium.org/6380005/diff/9001/chrome/browser/renderer_host... > chrome/browser/renderer_host/render_widget_host_view_views.cc:134: > RenderViewGone(base::TERMINATION_STATUS_NORMAL_TERMINATION, > Hmm. I can see what you mean, and in fact this is a bad idea: > RenderViewGone is a virtual function, and we shouldn't be calling it in > the destructor. I didn't actually add that, however, you added it in: > > http://src.chromium.org/viewvc/chrome?view=rev&revision=64015 > > (I just added some parameters to the call in my patch) > > I'm wondering if we should be calling this in "Destroy" instead? (But > I'm not sure that Destroy is reliably called for every object.) > > > http://codereview.chromium.org/6380005/ >
[+sadrul] OK, I figured out why I don't recall the patch - I reviewed and committed it for Sadrul, but I didn't write it. And now, looking at the code more closely, I don't think this is the right fix. The method we want to call can't be called either in the destructor or in Destroy; in both places the host_ may be in the process of destroying itself. I will try to figure this out a little better tonight. alex On Wednesday, January 19, 2011, Alex Nicolaou <anicolao@chromium.org> wrote: > Wow I have no recollection of that patch. But yes I think calling this in destroy is OK and destroy is called pretty reliably. At least, doing this in destroy would be better than it is right now. > I'll move this in a follow-up patch. > > alex > > On Wed, Jan 19, 2011 at 4:59 PM, <gspencer@chromium.org> wrote: > > > http://codereview.chromium.org/6380005/diff/9001/chrome/browser/renderer_host... > > File chrome/browser/renderer_host/render_widget_host_view_views.cc > (right): > > http://codereview.chromium.org/6380005/diff/9001/chrome/browser/renderer_host... > chrome/browser/renderer_host/render_widget_host_view_views.cc:134: > RenderViewGone(base::TERMINATION_STATUS_NORMAL_TERMINATION, > Hmm. I can see what you mean, and in fact this is a bad idea: > RenderViewGone is a virtual function, and we shouldn't be calling it in > the destructor. I didn't actually add that, however, you added it in: > > http://src.chromium.org/viewvc/chrome?view=rev&revision=64015 > > (I just added some parameters to the call in my patch) > > I'm wondering if we should be calling this in "Destroy" instead? (But > I'm not sure that Destroy is reliably called for every object.) > > http://codereview.chromium.org/6380005/ > > >
I wound up removing the call in the destructor. In the way the code currently runs, it can never get called, and the crash it was put in place to work around no longer happens. I rather suspect I'd done something else wrong back when this crash was happening - at that time this was an interim hybrid of the Gtk code and my new pure-Views code - and now that it's more cleaned up I think the crash is no longer possible. So I think this resulting CL is truly clean. I have spent some time debugging to verify that it works like I think it should. PTAL.
LGTM |
