Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1457203002:
Make WebContentsImpl aware of all of its RWHs. (Closed)
|
Created:
5 years, 1 month ago by lazyboy Modified:
5 years, 1 month ago Reviewers:
Charlie Reis CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, dcheng, site-isolation-reviews_chromium.org, nasko, ncarter (slow), Avi (use Gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake WebContentsImpl aware of all of its RWHs.
It used to be that WebContentsImpl only knew about the RWHs created
through WebContentsImpl::CreateNewWidget() and stored them in
WCI::created_widgets_. However, widgets created through
FrameTree::CreateRenderViewHost() used to be left out.
This made RenderWidgetHostImpl::WasResized() to incorrectly send
ViewMsg_Resize while the WebContents was being torn down. WasResized()
has a check for !delegate_ but |delegate_| never became nullptr during
WebContentsImpl shutdown. Adding all widgets to WCI::created_widgets_
will ensure that we notify RWHs about the delegate's (WC) destruction
via DetachDelegate and do not send ViewMsg_Resize while a the delegate
(WC) is shutting down.
BUG=523644, 523451
Test=See https://code.google.com/p/chromium/issues/detail?id=523644#c16
for repro steps. Ensure the renderer does not crash.
Committed: https://crrev.com/63f5b3105ceaaf470c2c8f1e52f8dc77a591b0f2
Cr-Commit-Position: refs/heads/master@{#361169}
Patch Set 1 #Patch Set 2 : fix RW destruction by Shutdown #
Total comments: 19
Patch Set 3 : unify two deletion paths + address comments #Patch Set 4 : revert unnecessary changes #
Total comments: 2
Patch Set 5 : add caution in RenderWidgetDeleted() #Patch Set 6 : sync @tott #
Messages
Total messages: 24 (11 generated)
Description was changed from ========== Make WebContents aware of all of its RWHs. It used to be that WebContents only knew about the RWHs created through WebContents::CreateNewWidget() and stored them in WCI::created_widgets_. However, widgets created through FrameTree::CreateRenderViewHost() used to be missed. This made RenderWidgetImpl::WasResized() to incorrectly send ViewMsg_Resize while the WebContents was being torn down. WasResized() has a check for !delegate_ but |delegate_| never became nullptr during WebContentsImpl shutdown. Adding all widgets to WCI::created_widgets_ will ensure that we do not send ViewMsg_Resize while a WebContents is shutting down. BUG=523644, 523451 Test=See https://code.google.com/p/chromium/issues/detail?id=523644#c16 for repro steps. Ensure the renderer does not crash. ========== to ========== Make WebContents aware of all of its RWHs. It used to be that WebContents only knew about the RWHs created through WebContents::CreateNewWidget() and stored them in WCI::created_widgets_. However, widgets created through FrameTree::CreateRenderViewHost() used to be left out. This made RenderWidgetImpl::WasResized() to incorrectly send ViewMsg_Resize while the WebContents was being torn down. WasResized() has a check for !delegate_ but |delegate_| never became nullptr during WebContentsImpl shutdown. Adding all widgets to WCI::created_widgets_ will ensure that we do not send ViewMsg_Resize while a WebContents is shutting down. BUG=523644, 523451 Test=See https://code.google.com/p/chromium/issues/detail?id=523644#c16 for repro steps. Ensure the renderer does not crash. ==========
Description was changed from ========== Make WebContents aware of all of its RWHs. It used to be that WebContents only knew about the RWHs created through WebContents::CreateNewWidget() and stored them in WCI::created_widgets_. However, widgets created through FrameTree::CreateRenderViewHost() used to be left out. This made RenderWidgetImpl::WasResized() to incorrectly send ViewMsg_Resize while the WebContents was being torn down. WasResized() has a check for !delegate_ but |delegate_| never became nullptr during WebContentsImpl shutdown. Adding all widgets to WCI::created_widgets_ will ensure that we do not send ViewMsg_Resize while a WebContents is shutting down. BUG=523644, 523451 Test=See https://code.google.com/p/chromium/issues/detail?id=523644#c16 for repro steps. Ensure the renderer does not crash. ========== to ========== Make WebContents aware of all of its RWHs. It used to be that WebContents only knew about the RWHs created through WebContents::CreateNewWidget() and stored them in WCI::created_widgets_. However, widgets created through FrameTree::CreateRenderViewHost() used to be left out. This made RenderWidgetImpl::WasResized() to incorrectly send ViewMsg_Resize while the WebContents was being torn down. WasResized() has a check for !delegate_ but |delegate_| never became nullptr during WebContentsImpl shutdown. Adding all widgets to WCI::created_widgets_ will ensure that we notify RWHs about the delegate's (WC) destruction via DetachDelegate and do not send ViewMsg_Resize while a the delegate (WC) is shutting down. BUG=523644, 523451 Test=See https://code.google.com/p/chromium/issues/detail?id=523644#c16 for repro steps. Ensure the renderer does not crash. ==========
Description was changed from ========== Make WebContents aware of all of its RWHs. It used to be that WebContents only knew about the RWHs created through WebContents::CreateNewWidget() and stored them in WCI::created_widgets_. However, widgets created through FrameTree::CreateRenderViewHost() used to be left out. This made RenderWidgetImpl::WasResized() to incorrectly send ViewMsg_Resize while the WebContents was being torn down. WasResized() has a check for !delegate_ but |delegate_| never became nullptr during WebContentsImpl shutdown. Adding all widgets to WCI::created_widgets_ will ensure that we notify RWHs about the delegate's (WC) destruction via DetachDelegate and do not send ViewMsg_Resize while a the delegate (WC) is shutting down. BUG=523644, 523451 Test=See https://code.google.com/p/chromium/issues/detail?id=523644#c16 for repro steps. Ensure the renderer does not crash. ========== to ========== Make WebContentsImpl aware of all of its RWHs. It used to be that WebContentsImpl only knew about the RWHs created through WebContentsImpl::CreateNewWidget() and stored them in WCI::created_widgets_. However, widgets created through FrameTree::CreateRenderViewHost() used to be left out. This made RenderWidgetImpl::WasResized() to incorrectly send ViewMsg_Resize while the WebContents was being torn down. WasResized() has a check for !delegate_ but |delegate_| never became nullptr during WebContentsImpl shutdown. Adding all widgets to WCI::created_widgets_ will ensure that we notify RWHs about the delegate's (WC) destruction via DetachDelegate and do not send ViewMsg_Resize while a the delegate (WC) is shutting down. BUG=523644, 523451 Test=See https://code.google.com/p/chromium/issues/detail?id=523644#c16 for repro steps. Ensure the renderer does not crash. ==========
The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1457203002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1457203002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Make WebContentsImpl aware of all of its RWHs. It used to be that WebContentsImpl only knew about the RWHs created through WebContentsImpl::CreateNewWidget() and stored them in WCI::created_widgets_. However, widgets created through FrameTree::CreateRenderViewHost() used to be left out. This made RenderWidgetImpl::WasResized() to incorrectly send ViewMsg_Resize while the WebContents was being torn down. WasResized() has a check for !delegate_ but |delegate_| never became nullptr during WebContentsImpl shutdown. Adding all widgets to WCI::created_widgets_ will ensure that we notify RWHs about the delegate's (WC) destruction via DetachDelegate and do not send ViewMsg_Resize while a the delegate (WC) is shutting down. BUG=523644, 523451 Test=See https://code.google.com/p/chromium/issues/detail?id=523644#c16 for repro steps. Ensure the renderer does not crash. ========== to ========== Make WebContentsImpl aware of all of its RWHs. It used to be that WebContentsImpl only knew about the RWHs created through WebContentsImpl::CreateNewWidget() and stored them in WCI::created_widgets_. However, widgets created through FrameTree::CreateRenderViewHost() used to be left out. This made RenderWidgetHostImpl::WasResized() to incorrectly send ViewMsg_Resize while the WebContents was being torn down. WasResized() has a check for !delegate_ but |delegate_| never became nullptr during WebContentsImpl shutdown. Adding all widgets to WCI::created_widgets_ will ensure that we notify RWHs about the delegate's (WC) destruction via DetachDelegate and do not send ViewMsg_Resize while a the delegate (WC) is shutting down. BUG=523644, 523451 Test=See https://code.google.com/p/chromium/issues/detail?id=523644#c16 for repro steps. Ensure the renderer does not crash. ==========
lazyboy@chromium.org changed reviewers: + creis@chromium.org
/cc dcheng@ who is also aware of this issue.
Thanks for tracking this down! One question below about the need for a second erase call, and otherwise it looks great. Nasko/Nick/Avi: See my naming comment below. https://codereview.chromium.org/1457203002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/1457203002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:43: virtual void RenderWidgetCreated(RenderWidgetHostImpl* render_widget_host) {} There's some potential confusion here around the name (RenderWidgetCreated vs RenderWidgetHostCreated). I won't ask you to fix it here, since it applies to the methods below as well. Nasko/Nick/Avi, we should probably rename all these in a separate CL? The concern is that we have methods in WebContentsObserver like RenderFrameCreated which very explicitly mean the RenderFrame and not the RenderFrameHost. This is thus a bit confusing (and even inconsistent below: RenderWidgetGotFocus and RenderWidgetWasResized have comments that disagree, though I'm not sure if it's intentional). https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1301: created_widgets_.erase(iter); Why is this part necessary? Won't we take care of it in RenderWidgetDeleted? (If not, let's add a comment why it needs to be removed in both places.)
avi@chromium.org changed reviewers: + avi@chromium.org
https://codereview.chromium.org/1457203002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/1457203002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:43: virtual void RenderWidgetCreated(RenderWidgetHostImpl* render_widget_host) {} On 2015/11/20 18:41:41, Charlie Reis wrote: > There's some potential confusion here around the name (RenderWidgetCreated vs > RenderWidgetHostCreated). I won't ask you to fix it here, since it applies to > the methods below as well. Nasko/Nick/Avi, we should probably rename all these > in a separate CL? Yeah. We've been super casual here in the past, and I'm certain it's bitten us in ways we weren't aware of. (As a point of discussion, when I introduced RenderProcessHostObserver, I was careful to name things like RenderProcessExited vs RenderProcessHostDestroyed. The old names were NOTIFICATION_RENDERER_PROCESS_CLOSED and NOTIFICATION_RENDERER_PROCESS_TERMINATED, and *no one* got it right.) https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1301: created_widgets_.erase(iter); On 2015/11/20 18:41:41, Charlie Reis wrote: > Why is this part necessary? Won't we take care of it in RenderWidgetDeleted? > > (If not, let's add a comment why it needs to be removed in both places.) Side comment: Why do you check to see if the widget is in the set before erasing it? Just blindly call erase; it works fine and is simpler to reason about. https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1500: created_widgets_.find(render_widget_host); You totally can use "auto" here. https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1502: created_widgets_.insert(render_widget_host); Same thing; you should be able to blindly insert. https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1515: created_widgets_.erase(iter); ... same here, though it's not your code.
https://codereview.chromium.org/1457203002/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/1457203002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:43: virtual void RenderWidgetCreated(RenderWidgetHostImpl* render_widget_host) {} On 2015/11/20 19:06:30, Avi wrote: > On 2015/11/20 18:41:41, Charlie Reis wrote: > > There's some potential confusion here around the name (RenderWidgetCreated vs > > RenderWidgetHostCreated). I won't ask you to fix it here, since it applies to > > the methods below as well. Nasko/Nick/Avi, we should probably rename all > these > > in a separate CL? > > Yeah. We've been super casual here in the past, and I'm certain it's bitten us > in ways we weren't aware of. (As a point of discussion, when I introduced > RenderProcessHostObserver, I was careful to name things like RenderProcessExited > vs RenderProcessHostDestroyed. The old names were > NOTIFICATION_RENDERER_PROCESS_CLOSED and > NOTIFICATION_RENDERER_PROCESS_TERMINATED, and *no one* got it right.) Acknowledged. https://codereview.chromium.org/1457203002/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:43: virtual void RenderWidgetCreated(RenderWidgetHostImpl* render_widget_host) {} On 2015/11/20 18:41:41, Charlie Reis wrote: > There's some potential confusion here around the name (RenderWidgetCreated vs > RenderWidgetHostCreated). I won't ask you to fix it here, since it applies to > the methods below as well. Nasko/Nick/Avi, we should probably rename all these > in a separate CL? > > The concern is that we have methods in WebContentsObserver like > RenderFrameCreated which very explicitly mean the RenderFrame and not the > RenderFrameHost. This is thus a bit confusing (and even inconsistent below: > RenderWidgetGotFocus and RenderWidgetWasResized have comments that disagree, > though I'm not sure if it's intentional). Let me know if you want me to do sth for this newly added method. i.e. rename it to RenderWidgetHostCreated. Acknowledged. https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:431: (*iter)->DetachDelegate(); not sure if this is an issue, but: Technically the delegate can call back to WebContentsImpl and cause badness if that code tries to access |created_widgets_|. Should we consider clearing created_widgets_ first after copying it and start calling DetachDelegate() on that copy of the set? https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1301: created_widgets_.erase(iter); On 2015/11/20 19:06:30, Avi wrote: > On 2015/11/20 18:41:41, Charlie Reis wrote: > > Why is this part necessary? Won't we take care of it in RenderWidgetDeleted? > > > > (If not, let's add a comment why it needs to be removed in both places.) > > Side comment: Why do you check to see if the widget is in the set before erasing > it? Just blindly call erase; it works fine and is simpler to reason about. Thanks I've updated the relevant places. (Side effect of copy-paste). https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1301: created_widgets_.erase(iter); On 2015/11/20 18:41:41, Charlie Reis wrote: > Why is this part necessary? Won't we take care of it in RenderWidgetDeleted? Yes it would have, however there's a WCI::is_being_destroyed_ bailout at the very first line which misses exactly the case I'm trying to fix. Thinking of this, I'm not sure if comment on line 1508 is true: "// |created_widgets_| might have been destroyed." The original CL that introduced it doesn't seem to have any reasoning about this. See line 1173 on the ride side: https://codereview.chromium.org/10968037/diff/10005/content/browser/web_conte... I've gone ahead and merged this and removed the !is_being_destroyed_ requirement while deleting RWHs from |created_widgets_|. Fingers crossed. > > (If not, let's add a comment why it needs to be removed in both places.) https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1500: created_widgets_.find(render_widget_host); On 2015/11/20 19:06:30, Avi wrote: > You totally can use "auto" here. Not required anymore. https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1502: created_widgets_.insert(render_widget_host); On 2015/11/20 19:06:30, Avi wrote: > Same thing; you should be able to blindly insert. Done. https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1515: created_widgets_.erase(iter); On 2015/11/20 19:06:30, Avi wrote: > ... same here, though it's not your code. Done.
avi@chromium.org changed reviewers: - avi@chromium.org
My comments were all nitpicky; I'll yield the floor to Charlie.
Thanks. I think this is ready, but I'm curious for your answers to the questions below. https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:431: (*iter)->DetachDelegate(); On 2015/11/20 23:52:05, lazyboy wrote: > not sure if this is an issue, but: > Technically the delegate can call back to WebContentsImpl and cause > badness if that code tries to access |created_widgets_|. > Should we consider clearing created_widgets_ first after copying it and > start calling DetachDelegate() on that copy of the set? I don't follow your concern here. DetachDelegate is a method on RWHI that just sets the delegate_ field to null. It doesn't reach back into WebContents, and we haven't started deleting created_widgets_ yet anyway. Are you referring to something else? https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1301: created_widgets_.erase(iter); On 2015/11/20 23:52:05, lazyboy wrote: > On 2015/11/20 18:41:41, Charlie Reis wrote: > > Why is this part necessary? Won't we take care of it in RenderWidgetDeleted? > Yes it would have, however there's a WCI::is_being_destroyed_ bailout > at the very first line which misses exactly the case I'm trying to > fix. > Thinking of this, I'm not sure if comment on line 1508 is true: > "// |created_widgets_| might have been destroyed." > The original CL that introduced it doesn't seem to have any reasoning > about this. See line 1173 on the ride side: > https://codereview.chromium.org/10968037/diff/10005/content/browser/web_conte... > > I've gone ahead and merged this and removed the !is_being_destroyed_ requirement > while deleting RWHs from |created_widgets_|. Fingers crossed. > > > > (If not, let's add a comment why it needs to be removed in both places.) > Hmm, that CL didn't add any tests to prevent regression, so it's hard to tell from the try jobs if you've reintroduced a bug. Did you try the repro steps from the bug? https://code.google.com/p/chromium/issues/detail?id=149821 1. pick any youtube video and play it 2. go fullscreen 3. press ctrl+w At any rate, I don't see an immediate problem with your change. If the WebContents is being destroyed, all of its remaining widgets will have their delegate_ cleared before created_widgets_ is destroyed, so they won't be able to call back into RenderWidgetDeleted and access it. So it seems ok to me. https://codereview.chromium.org/1457203002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1457203002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1507: created_widgets_.erase(render_widget_host); How are you getting here after is_being_destroyed_ is set to true but before created_widgets_ is cleared? Is one of the lines in the WebContentsImpl destructor triggering this? Maybe add a comment how it's possible, as a breadcrumb to future developers.
https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:431: (*iter)->DetachDelegate(); On 2015/11/21 01:19:19, Charlie Reis wrote: > On 2015/11/20 23:52:05, lazyboy wrote: > > not sure if this is an issue, but: > > Technically the delegate can call back to WebContentsImpl and cause > > badness if that code tries to access |created_widgets_|. > > Should we consider clearing created_widgets_ first after copying it and > > start calling DetachDelegate() on that copy of the set? > > I don't follow your concern here. DetachDelegate is a method on RWHI that just > sets the delegate_ field to null. It doesn't reach back into WebContents, and > we haven't started deleting created_widgets_ yet anyway. Sorry for not being precise, this is just a hypothetical scenario I was talking about: *if* RWHI::DetachDelegate() would have called into WCI (assuming someone later on adds code to do that) and tried to access |created_widgets_|, that would become a problem. This is probably a non-issue here. > > Are you referring to something else? https://codereview.chromium.org/1457203002/diff/20001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1301: created_widgets_.erase(iter); On 2015/11/21 01:19:19, Charlie Reis wrote: > On 2015/11/20 23:52:05, lazyboy wrote: > > On 2015/11/20 18:41:41, Charlie Reis wrote: > > > Why is this part necessary? Won't we take care of it in > RenderWidgetDeleted? > > Yes it would have, however there's a WCI::is_being_destroyed_ bailout > > at the very first line which misses exactly the case I'm trying to > > fix. > > Thinking of this, I'm not sure if comment on line 1508 is true: > > "// |created_widgets_| might have been destroyed." > > The original CL that introduced it doesn't seem to have any reasoning > > about this. See line 1173 on the ride side: > > > https://codereview.chromium.org/10968037/diff/10005/content/browser/web_conte... > > > > I've gone ahead and merged this and removed the !is_being_destroyed_ > requirement > > while deleting RWHs from |created_widgets_|. Fingers crossed. > > > > > > (If not, let's add a comment why it needs to be removed in both places.) > > > > Hmm, that CL didn't add any tests to prevent regression, so it's hard to tell > from the try jobs if you've reintroduced a bug. Did you try the repro steps > from the bug? Yes, I've tried with and without flash fullscreen. Seemed fine. > > https://code.google.com/p/chromium/issues/detail?id=149821 > 1. pick any youtube video and play it > 2. go fullscreen > 3. press ctrl+w > > > At any rate, I don't see an immediate problem with your change. If the > WebContents is being destroyed, all of its remaining widgets will have their > delegate_ cleared before created_widgets_ is destroyed, so they won't be able to > call back into RenderWidgetDeleted and access it. So it seems ok to me. https://codereview.chromium.org/1457203002/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1457203002/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:1507: created_widgets_.erase(render_widget_host); On 2015/11/21 01:19:19, Charlie Reis wrote: > How are you getting here after is_being_destroyed_ is set to true but before > created_widgets_ is cleared? Is one of the lines in the WebContentsImpl > destructor triggering this? Yes. FrameTree::ForEach in ~WebContentsImpl() calls RFHM::ClearRHFsPendingShutdown() From: http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... See this shortened callstack: #2 0x32f6377 in content::RenderWidgetHostImpl::Shutdown() content/browser/renderer_host/render_widget_host_impl.cc:438 #3 0x32e5d7e in content::RenderViewHostImpl::Shutdown() content/browser/renderer_host/render_view_host_impl.cc:978:3 #4 0x2e4615f in content::FrameTree::ReleaseRenderViewHostRef(content::RenderViewHostImpl*) content/browser/frame_host/frame_tree.cc:359:7 #5 0x2e92199 in content::RenderFrameHostImpl::~RenderFrameHostImpl() content/browser/frame_host/render_frame_host_impl.cc:271:3 ... #14 0x2ebe798 in content::RenderFrameHostManager::ClearRFHsPendingShutdown(content::FrameTreeNode*) content/browser/frame_host/render_frame_host_manager.cc:206 #15 0x2e4290a in Run base/callback.h:396:12 #16 0x2e4290a in content::FrameTree::ForEach(base::Callback<bool (content::FrameTreeNode*)> const&, content::FrameTreeNode*) const content/browser/frame_host/frame_tree.cc:189 #17 0x35a6ca6 in content::WebContentsImpl::~WebContentsImpl() content/browser/web_contents/web_contents_impl.cc:424:3 > > Maybe add a comment how it's possible, as a breadcrumb to future developers. Done.
Thanks, LGTM.
The CQ bit was checked by lazyboy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1457203002/#ps100001 (title: "sync @tott")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1457203002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1457203002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/63f5b3105ceaaf470c2c8f1e52f8dc77a591b0f2 Cr-Commit-Position: refs/heads/master@{#361169} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
