Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1409693009:
Fix leaking of RenderFrames. (Closed)
|
Created:
5 years, 1 month ago by nasko Modified:
5 years ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix leaking of RenderFrames.
This CL adds an explicit IPC from the browser process to the renderer
process to delete RenderFrame objects, when the corresponding RenderFrameHost
is deleted on the browser side.
It fixes two known leaks:
* cancelling a navigation in a pending RenderFrameHost
* parent frame deleting from DOM a child frame in a remote process
BUG=548275
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/eab5c55815be54a878e9a79aa68920a3d6db4a93
Cr-Commit-Position: refs/heads/master@{#365156}
Patch Set 1 #
Total comments: 19
Patch Set 2 : Fix issues based on Charlie's review. #Patch Set 3 : Rename the IPC and use a boolean to indicate browser initiated detach. #Patch Set 4 : Enable previously disabled test. #Patch Set 5 : Make the RenderFrame detachment more generic. #
Total comments: 1
Patch Set 6 : Fixing just the pending RF leak. #
Total comments: 6
Patch Set 7 : Fixes based on another review round. #
Total comments: 1
Patch Set 8 : Rebase on ToT. #Patch Set 9 : Fixed frame checking and not detaching for main frames. #
Total comments: 5
Patch Set 10 : Check that DetachFrame is only called on subframes for now. #Patch Set 11 : Handle main frame navigating to existing process. #
Total comments: 8
Patch Set 12 : Rebased on ToT and a small change. #
Total comments: 18
Patch Set 13 : Send Detach IPC from the RFH destructor. #
Total comments: 4
Patch Set 14 : Don't send IPC for the main frame. #
Total comments: 10
Patch Set 15 : Fixes for Charlie's and Daniel's reviews. #
Total comments: 4
Patch Set 16 : Fixing another round of nits. #
Total comments: 6
Patch Set 17 : Rebased on ToT and couple of more nits fixes. #
Messages
Total messages: 49 (15 generated)
nasko@chromium.org changed reviewers: + creis@chromium.org, dcheng@chromium.org
Hey Charlie and Daniel, I've put up a fix for the leaking of pending RenderFrames. I also wrote a test for --site-per-process to ensure it works for subframes, since in RenderFrame::detachFrame we have an explicit check for !is_main_frame. The test is very similar to the one failing after Daniel's CL and it relies on subtle behavior, but I couldn't find a better way to have an automated test for this leak. Can you review it for me please? Thanks in advance! Nasko
Thanks! Glad to see a fix for this. It's not clear to me that the detach behavior is correct, so I agree on getting Daniel's thoughts there. I'm also curious if we have a similar leak in normal detach cases, since I didn't see an IPC message for it. Let's also re-enable the test in chromium.fyi.json in this CL. (Nick and I will shortly be owners of that file now, so you won't need to TBR jam.) https://codereview.chromium.org/1409693009/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1409693009/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:921: // RenderFrame that has not committed yet isn't linked into the renderer nit: Let's rephrase this. Is the part about being linked into the renderer's frame tree relevant here? Seems like we would need to tell the renderer process to delete it either way. Maybe something as simple as this? Also clean up the unused RenderFrame in the renderer process. https://codereview.chromium.org/1409693009/diff/1/content/browser/site_per_pr... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/1409693009/diff/1/content/browser/site_per_pr... content/browser/site_per_process_browsertest.cc:3630: // This test ensures that RenderFrame isn't leaked in the renderer process if nit: that the https://codereview.chromium.org/1409693009/diff/1/content/browser/site_per_pr... content/browser/site_per_process_browsertest.cc:3660: // Grab the routing id of the pending RenderFrameHost and setup a process nit: set up https://codereview.chromium.org/1409693009/diff/1/content/browser/site_per_pr... content/browser/site_per_process_browsertest.cc:3688: process->Send(new FrameMsg_NewFrame(params)); I'm a bit surprised this works cleanly. I guess we don't have sanity checks that routing IDs won't be reused. Would that cause problems for this test if someone added such a sanity check? Then again, I'm not sure of a better way to test that a RenderFrame doesn't exist, since I don't see an IPC it sends during deletion. Perhaps this is fine for the time being. https://codereview.chromium.org/1409693009/diff/1/content/common/frame_messag... File content/common/frame_messages.h (right): https://codereview.chromium.org/1409693009/diff/1/content/common/frame_messag... content/common/frame_messages.h:483: // Instructs the renderer to delete the RenderFrame object identified by Maybe we should distinguish this from how RenderFrames normally get deleted? And now that I mention it, I can't find a detach message for that. If a parent frame sends a FrameHostMsg_Detach to the browser process, how do we delete the RenderFrame in the child process? Do we? :) https://codereview.chromium.org/1409693009/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:675: frame->GetWebFrame()->detach(); We should get Daniel to confirm that detach is the right action to take here, since the frame isn't in the tree yet. https://codereview.chromium.org/1409693009/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:2369: // the RenderFrame is initialized to replace a RenderFrameProxy and its This isn't always true, is it? If the pending RenderFrame is the first in the process, will it be replacing a proxy? (We could still end up keeping the process alive in DiscardUnusedFrame in that case, since another frame might have been added to the process in the meantime.) https://codereview.chromium.org/1409693009/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:2374: nit: Remove extra blank line.
https://codereview.chromium.org/1409693009/diff/1/content/common/frame_messag... File content/common/frame_messages.h (right): https://codereview.chromium.org/1409693009/diff/1/content/common/frame_messag... content/common/frame_messages.h:483: // Instructs the renderer to delete the RenderFrame object identified by On 2015/10/29 at 17:28:58, Charlie Reis wrote: > Maybe we should distinguish this from how RenderFrames normally get deleted? > > And now that I mention it, I can't find a detach message for that. If a parent frame sends a FrameHostMsg_Detach to the browser process, how do we delete the RenderFrame in the child process? Do we? :) Ah... this is going to be fun. We're going to add some logic too, so that additional FrameHostMsg_Detach messages from the mirrored frames don't also trigger FrameMsg_DeleteFrame to be sent. Also, maybe just call this FrameMsg_Detach, since that it seems likely we'll end up reusing this for that? https://codereview.chromium.org/1409693009/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:675: frame->GetWebFrame()->detach(); On 2015/10/29 at 17:28:58, Charlie Reis wrote: > We should get Daniel to confirm that detach is the right action to take here, since the frame isn't in the tree yet. It's kind of wonky, but we should basically treat this as the entry point for "someone wants to get rid of this frame, so do the appropriate cleanups". The fact that detach normally removes the frame from the frame tree is mostly incidental: after all, you can detach the main frame, which doesn't really remove it from the frame tree.
Yay, fun! Addressed comments, but did not add handling of cleanup of mirrored frames. I would rather do this in a separate patch to keep this one more focused. https://codereview.chromium.org/1409693009/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1409693009/diff/1/content/browser/frame_host/... content/browser/frame_host/render_frame_host_manager.cc:921: // RenderFrame that has not committed yet isn't linked into the renderer On 2015/10/29 17:28:58, Charlie Reis wrote: > nit: Let's rephrase this. Is the part about being linked into the renderer's > frame tree relevant here? Seems like we would need to tell the renderer process > to delete it either way. > > Maybe something as simple as this? > Also clean up the unused RenderFrame in the renderer process. Done. https://codereview.chromium.org/1409693009/diff/1/content/browser/site_per_pr... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/1409693009/diff/1/content/browser/site_per_pr... content/browser/site_per_process_browsertest.cc:3630: // This test ensures that RenderFrame isn't leaked in the renderer process if On 2015/10/29 17:28:58, Charlie Reis wrote: > nit: that the Done. https://codereview.chromium.org/1409693009/diff/1/content/browser/site_per_pr... content/browser/site_per_process_browsertest.cc:3660: // Grab the routing id of the pending RenderFrameHost and setup a process On 2015/10/29 17:28:58, Charlie Reis wrote: > nit: set up Done. https://codereview.chromium.org/1409693009/diff/1/content/browser/site_per_pr... content/browser/site_per_process_browsertest.cc:3688: process->Send(new FrameMsg_NewFrame(params)); On 2015/10/29 17:28:58, Charlie Reis wrote: > I'm a bit surprised this works cleanly. I guess we don't have sanity checks > that routing IDs won't be reused. Would that cause problems for this test if > someone added such a sanity check? Yes, we don't have any checks for possible reuse. This would require keeping a set of all already seen routing ids and there is no such thing today. If someone were to add this check, yes, it will cause this test to fail. > Then again, I'm not sure of a better way to test that a RenderFrame doesn't > exist, since I don't see an IPC it sends during deletion. Perhaps this is fine > for the time being. There are no IPCs during deletion. I also am not eager to add one just to allow us to test memory leak. I'd rather investigate why our memory bots aren't catching this case. Or maybe they are and we didn't have a test that successfully exercised the leak. https://codereview.chromium.org/1409693009/diff/1/content/common/frame_messag... File content/common/frame_messages.h (right): https://codereview.chromium.org/1409693009/diff/1/content/common/frame_messag... content/common/frame_messages.h:483: // Instructs the renderer to delete the RenderFrame object identified by On 2015/10/29 17:28:58, Charlie Reis wrote: > Maybe we should distinguish this from how RenderFrames normally get deleted? > > And now that I mention it, I can't find a detach message for that. If a parent > frame sends a FrameHostMsg_Detach to the browser process, how do we delete the > RenderFrame in the child process? Do we? :) Unfortunately, I think you are right : (. https://codereview.chromium.org/1409693009/diff/1/content/common/frame_messag... content/common/frame_messages.h:483: // Instructs the renderer to delete the RenderFrame object identified by On 2015/10/29 18:50:52, dcheng wrote: > On 2015/10/29 at 17:28:58, Charlie Reis wrote: > > Maybe we should distinguish this from how RenderFrames normally get deleted? > > > > And now that I mention it, I can't find a detach message for that. If a > parent frame sends a FrameHostMsg_Detach to the browser process, how do we > delete the RenderFrame in the child process? Do we? :) > > Ah... this is going to be fun. We're going to add some logic too, so that > additional FrameHostMsg_Detach messages from the mirrored frames don't also > trigger FrameMsg_DeleteFrame to be sent. Yes, indeed. > Also, maybe just call this FrameMsg_Detach, since that it seems likely we'll end > up reusing this for that? Done. https://codereview.chromium.org/1409693009/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:675: frame->GetWebFrame()->detach(); On 2015/10/29 17:28:58, Charlie Reis wrote: > We should get Daniel to confirm that detach is the right action to take here, > since the frame isn't in the tree yet. It isn't in the tree, but there are a lot of supporting objects in blink::core which need to be properly cleaned up. That happens when going through WebFrame::detach/LocalFrame::detach. Just deleting the RenderFrame isn't the right behavior, as there are parts of RenderFrame::frameDetached that are doing cleanup and therefore it must run. https://codereview.chromium.org/1409693009/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:2369: // the RenderFrame is initialized to replace a RenderFrameProxy and its On 2015/10/29 17:28:58, Charlie Reis wrote: > This isn't always true, is it? If the pending RenderFrame is the first in the > process, will it be replacing a proxy? > > (We could still end up keeping the process alive in DiscardUnusedFrame in that > case, since another frame might have been added to the process in the meantime.) In the case of pending RenderFrame being first there are two variations: * Subframe - the RenderView and proxy will be created first, so it isn't really first in the process. * Main frame - the RenderView will be creating the RenderFrame and if cancelled, then the RenderView should be deleted and therefore delete the RenderFrame. I'll dig a bit more into this though. https://codereview.chromium.org/1409693009/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:2374: On 2015/10/29 17:28:59, Charlie Reis wrote: > nit: Remove extra blank line. Done.
Another update with more generic handling of RenderFrame deletion. I did find a problem right after I uploaded it, but still sending it for consideration and ideas. https://codereview.chromium.org/1409693009/diff/80001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1409693009/diff/80001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:249: // corresponding RenderFrame also needs to be cleaned up. I'm not correct here. The case of main frame and same site subframe, where the main frame removes the subframe from the tree. It will result in a detach message from the RenderFrame, which the renderer will cleanup right after the IPC is sent. In that case, we will send an extra IPC that will not have a listener, so it shouldn't really be a problem, but I still dislike it. I will think of how to fix this tomorrow, though the strawman is to add DETACHED state into rfh_state_, which we set in the FrameHostMsg_Detach and handle properly here.
The CQ bit was checked by nasko@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/1409693009/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1409693009/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
As we agreed, let's fix just be pending RF leaks. PS6 should be the same as PS4.
Thanks. Makes sense to fix the first leak, and we can come back for the second one. One concern about the new bool below. (All the comments are basically the same thing.) https://codereview.chromium.org/1409693009/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:2371: // removal. If the frame is being detached for swap or the frame isn't fully Is this comment change stale? I'm not sure how is_browser_initiated_detach_ is related to whether the frame is fully initialized. Or rather, maybe is_browser_initiated_detach_ isn't the right name for now? https://codereview.chromium.org/1409693009/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:2401: if (!is_browser_initiated_detach_ && !is_main_frame_ && The name is confusing here as well. Whether the frame is already detaching doesn't seem related to whether it's in the tree yet or not. https://codereview.chromium.org/1409693009/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1409693009/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.h:911: bool is_browser_initiated_detach_; nit: in_browser_initiated_detach_, perhaps? Although, I'm having trouble with this. On the one hand, it looks and sounds like we're trying to track whether we've already started detaching this frame, and thus shouldn't repeat some tasks (like telling the browser process). That seems like it could apply to the other leak as well. On the other hand, it's being used to indicate whether a frame is being discarded before it's used, and thus isn't in the tree yet. We should probably pick one interpretation and stick with it for this CL, and revise it later if needed.
PS7 addresses fixes and PS8 rebases to allow for tryjobs. I have forgotten to remove one LOG line, which will be gone on the next upload. https://codereview.chromium.org/1409693009/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:2371: // removal. If the frame is being detached for swap or the frame isn't fully On 2015/10/30 20:15:44, Charlie Reis wrote: > Is this comment change stale? I'm not sure how is_browser_initiated_detach_ is > related to whether the frame is fully initialized. > > Or rather, maybe is_browser_initiated_detach_ isn't the right name for now? I think the confusion came from my failure to update the comments. Sorry about that :(. https://codereview.chromium.org/1409693009/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:2401: if (!is_browser_initiated_detach_ && !is_main_frame_ && On 2015/10/30 20:15:44, Charlie Reis wrote: > The name is confusing here as well. Whether the frame is already detaching > doesn't seem related to whether it's in the tree yet or not. I think this isn't correct usage here actually. The check should be "if the parent knows about the child, remove it.". https://codereview.chromium.org/1409693009/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1409693009/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.h:911: bool is_browser_initiated_detach_; On 2015/10/30 20:15:44, Charlie Reis wrote: > nit: in_browser_initiated_detach_, perhaps? > > Although, I'm having trouble with this. On the one hand, it looks and sounds > like we're trying to track whether we've already started detaching this frame, > and thus shouldn't repeat some tasks (like telling the browser process). That > seems like it could apply to the other leak as well. It is meant to indicate that it is a browser initiated detaching. If that is the case, we shouldn't be telling the browser that we are detaching the frame, as it already initiated this action. > On the other hand, it's being used to indicate whether a frame is being > discarded before it's used, and thus isn't in the tree yet. No, I don't think it is used to indicate that. I realized that it is more generic and we will need this to fix the other leaks too. So in general, if the browser process told us to detach a frame, we need to know that and avoid sending IPCs back to it. > We should probably pick one interpretation and stick with it for this CL, and > revise it later if needed. It should only be one - browser told us to detach, so we are in the process of doing so.
Description was changed from ========== Fix leaking of RenderFrames on cancelled cross-process navigations. This CL adds an explicit IPC from the browser process to the renderer process to delete RenderFrame objects, which were used for pending cross-process navigations. All other cases are handled through either detaching or swapping frames. BUG=548275 ========== to ========== Fix leaking of RenderFrames on cancelled cross-process navigations. This CL adds an explicit IPC from the browser process to the renderer process to delete RenderFrame objects, which were used for pending cross-process navigations. BUG=548275 ==========
Maybe the question below explains the try job failures? https://codereview.chromium.org/1409693009/diff/120001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:2399: for (WebFrame* child = frame->firstChild(); child; This doesn't look right to me. Shouldn't we be looking at frame->parent()'s children? frame won't ever be equal to its own child.
Patchset #9 (id:160001) has been deleted
Thanks! LGTM.
https://codereview.chromium.org/1409693009/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:674: void RenderFrameImpl::DetachFrame(int routing_id) { Is it worth DCHECKing our current limitations? For example, this shouldn't be called on a main frame, right? https://codereview.chromium.org/1409693009/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:2408: for (WebFrame* child = frame->parent()->firstChild(); child; Is there no heuristic we can use inside //content itself? This check makes me pretty sad.
https://codereview.chromium.org/1409693009/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:674: void RenderFrameImpl::DetachFrame(int routing_id) { On 2015/11/03 04:22:54, dcheng wrote: > Is it worth DCHECKing our current limitations? For example, this shouldn't be > called on a main frame, right? Done. https://codereview.chromium.org/1409693009/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:2408: for (WebFrame* child = frame->parent()->firstChild(); child; On 2015/11/03 04:22:53, dcheng wrote: > Is there no heuristic we can use inside //content itself? This check makes me > pretty sad. Well, content/ is now in charge of maintaining the frame tree, right? So it is in content/ that we check whether this frame has been linked into the frame tree. I could start adding booleans to indicate different states, but I think this is less prone to problems in the future. Also, it is easy to understand why we aren't removing the frame from the parent - it isn't in its list of children. If we have to do this in more than one place, I'm open to exploring storing state or something else.
Patchset #11 (id:220001) has been deleted
One more update based on the discussion today. Fixes: * Moved RenderFrameImpl to use a boolean member to indicate whether it is in the frame tree. * Send detach IPC for main frame based on the active frame count.
https://codereview.chromium.org/1409693009/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:674: void RenderFrameImpl::DetachFrame(int routing_id) { On 2015/11/03 17:59:08, nasko (slow to review) wrote: > On 2015/11/03 04:22:54, dcheng wrote: > > Is it worth DCHECKing our current limitations? For example, this shouldn't be > > called on a main frame, right? > > Done. I don't see the new DCHECK. I guess it doesn't apply now that we realized we need to call it on main frames as well? https://codereview.chromium.org/1409693009/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1409693009/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:930: (render_frame_host->GetSiteInstance()->active_frame_count() > 1U || Did you have a concern about this? I'm looking at it and it seems ok to me. Can you remind me which method of RFHM does the cleanup if active_frame_count() is only 1? As long as the conditions match, I think we should be good. https://codereview.chromium.org/1409693009/diff/240001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.cc:2817: is_inserted_in_frame_tree_ = true; Is it correct to put this behind a proxy_routing_id_ check? I would think that a pending RFH in a new process wouldn't usually have a proxy for its own frame. Maybe we put the RFH into the frame tree up front in that case (since a swap apparently isn't required)? Does this bool get set to true in that case? Sorry if I'm misunderstanding. https://codereview.chromium.org/1409693009/diff/240001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1409693009/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.h:921: // which of the two states this frame is currently in. Let's move the last sentence to be the first one and rephrase to: // Indicates whether the frame has been inserted into the frame tree yet or not. // // When a frame... https://codereview.chromium.org/1409693009/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.h:922: bool is_inserted_in_frame_tree_; nit: in_frame_tree_
Hey Charlie, I've finally gotten back to this CL. It is rebased on ToT and I've addressed your comments from the last patch. There is only one additional change that I've marked with a comment for easy discovery. Thanks! Nasko https://codereview.chromium.org/1409693009/diff/240001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1409693009/diff/240001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:930: (render_frame_host->GetSiteInstance()->active_frame_count() > 1U || On 2015/11/04 21:28:30, Charlie Reis (slow til 11-16) wrote: > Did you have a concern about this? I'm looking at it and it seems ok to me. > Can you remind me which method of RFHM does the cleanup if active_frame_count() > is only 1? As long as the conditions match, I think we should be good. It is coincidentally right after this - ShutdownProxiesIfLastActiveFrameInSiteInstance. https://codereview.chromium.org/1409693009/diff/240001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.cc:2817: is_inserted_in_frame_tree_ = true; On 2015/11/04 21:28:30, Charlie Reis (slow til 11-16) wrote: > Is it correct to put this behind a proxy_routing_id_ check? I would think that > a pending RFH in a new process wouldn't usually have a proxy for its own frame. Unless it navigates back to its parent's frame process :). I don't think of this in terms of presence of proxy routing id. We are doing a frame swap and replacing some frame in the tree with this frame. As such, from this point on this frame is in the tree. > Maybe we put the RFH into the frame tree up front in that case (since a swap > apparently isn't required)? Does this bool get set to true in that case? When we aren't replacing a proxy, I think we directly add it into the frame tree. I've made sure we set this to true there. > Sorry if I'm misunderstanding. No worries, this is complicated and it is good to be paranoid about all edge cases. https://codereview.chromium.org/1409693009/diff/240001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1409693009/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.h:921: // which of the two states this frame is currently in. On 2015/11/04 21:28:30, Charlie Reis (slow til 11-16) wrote: > Let's move the last sentence to be the first one and rephrase to: > // Indicates whether the frame has been inserted into the frame tree yet or not. > // > // When a frame... Done. https://codereview.chromium.org/1409693009/diff/240001/content/renderer/rende... content/renderer/render_frame_impl.h:922: bool is_inserted_in_frame_tree_; On 2015/11/04 21:28:30, Charlie Reis (slow til 11-16) wrote: > nit: in_frame_tree_ Done. https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:704: render_frame->in_frame_tree_ = true; Charlie: This is newly added code, which was prompted by the comment in didCommitProvisionalLoad.
Hrm, I think I found another problem that could leave the browser process thinking the RFH is canceled but the renderer process thinking the RF is committed. See below. https://codereview.chromium.org/1409693009/diff/260001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1409693009/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:950: !frame_tree_node_->IsMainFrame())) { nit: Swap the active_frame_count and IsMainFrame conditions to be in the same order as the comment. https://codereview.chromium.org/1409693009/diff/260001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/1409693009/diff/260001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:4331: // The test must wait for a process to exit, but if there is no leak, the nit: s/a/the/ https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:703: // above call to createLocalChild, ensure the state is set properly. nit: Drop "Since" and "ensure the state is set properly". The remaining text justifies setting in_frame_tree_ to true. Also add a blank line before the comment. https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:704: render_frame->in_frame_tree_ = true; On 2015/12/10 21:26:52, nasko wrote: > Charlie: This is newly added code, which was prompted by the comment in > didCommitProvisionalLoad. Acknowledged. https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:742: void RenderFrameImpl::DetachFrame(int routing_id) { Is this ever meant to be called when in_frame_tree_ is true? Maybe we should DCHECK if not. Actually, I just thought of a possible race where that could happen, and it might get us into trouble. Suppose the browser process cancels a slow pending RFH just as it's committing (e.g., due to a cross-process navigation to a different site). The renderer process will have committed the RF and set in_frame_tree_ to true, while the browser process has destroyed the RFH and won't listen to the commit. The commit in the renderer process would presumably destroy the RenderFrameProxy. Would that mean that we would end up with no proxy when we should have one? Yikes. https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.h:180: static void DetachFrame(int routing_id); Sanity check: Does calling this Detach really make sense? We're calling it in cases that the frame hasn't been attached to the tree yet. I wonder if it would be clearer to say something like DestroyFrame. (It seems like we should be verifying that this is only called for pending RenderFrames. See my other comment/concern about that in the .cc file.) https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.h:949: // When a frame is detached in response to a messge from the browser process, nit: message https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.h:954: nit: Remove extra blank line.
A slight reworking of the CL. I moved the IPC sending to be done universally when an RFH is deleted, which will cover the case where a parent removes a remote child from its DOM. https://codereview.chromium.org/1409693009/diff/260001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1409693009/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:950: !frame_tree_node_->IsMainFrame())) { On 2015/12/11 00:03:17, Charlie Reis wrote: > nit: Swap the active_frame_count and IsMainFrame conditions to be in the same > order as the comment. No longer applies as the code is gone. https://codereview.chromium.org/1409693009/diff/260001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/1409693009/diff/260001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:4331: // The test must wait for a process to exit, but if there is no leak, the On 2015/12/11 00:03:17, Charlie Reis wrote: > nit: s/a/the/ Done. https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:703: // above call to createLocalChild, ensure the state is set properly. On 2015/12/11 00:03:17, Charlie Reis wrote: > nit: Drop "Since" and "ensure the state is set properly". The remaining text > justifies setting in_frame_tree_ to true. > > Also add a blank line before the comment. Done. https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:742: void RenderFrameImpl::DetachFrame(int routing_id) { On 2015/12/11 00:03:17, Charlie Reis wrote: > Is this ever meant to be called when in_frame_tree_ is true? Maybe we should > DCHECK if not. With my latest change it is supposed to. It detaches a frame regardless whether it is in the tree or not. I'm open to tweaking the name if we have a better one. > Actually, I just thought of a possible race where that could happen, and it > might get us into trouble. Suppose the browser process cancels a slow pending > RFH just as it's committing (e.g., due to a cross-process navigation to a > different site). The renderer process will have committed the RF and set > in_frame_tree_ to true, while the browser process has destroyed the RFH and > won't listen to the commit. With the latest patch version, the RFH destruction will also destroy the RF, since the IPC is sent from the RFH destructor. > The commit in the renderer process would presumably destroy the > RenderFrameProxy. Would that mean that we would end up with no proxy when we > should have one? Yikes. Actually, a bit different now - the frame will get completely removed, as the commit would have happened, the proxy will be gone and if the IPC to destroy the RenderFrame comes after that, it will totally remove the frame from the tree. : ( https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.h:180: static void DetachFrame(int routing_id); On 2015/12/11 00:03:17, Charlie Reis wrote: > Sanity check: Does calling this Detach really make sense? We're calling it in > cases that the frame hasn't been attached to the tree yet. I wonder if it would > be clearer to say something like DestroyFrame. I am calling it now in all cases, regardless of whether it is in the tree or not. I called it Detach, as it matches the semantics of Blink, though Destroy can also work as it is more universal. > (It seems like we should be verifying that this is only called for pending > RenderFrames. See my other comment/concern about that in the .cc file.) Not anymore. https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.h:949: // When a frame is detached in response to a messge from the browser process, On 2015/12/11 00:03:17, Charlie Reis wrote: > nit: message Done. https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.h:954: On 2015/12/11 00:03:17, Charlie Reis wrote: > nit: Remove extra blank line. Done.
https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:742: void RenderFrameImpl::DetachFrame(int routing_id) { On 2015/12/11 00:03:17, Charlie Reis wrote: > Is this ever meant to be called when in_frame_tree_ is true? Maybe we should > DCHECK if not. > > Actually, I just thought of a possible race where that could happen, and it > might get us into trouble. Suppose the browser process cancels a slow pending > RFH just as it's committing (e.g., due to a cross-process navigation to a > different site). The renderer process will have committed the RF and set > in_frame_tree_ to true, while the browser process has destroyed the RFH and > won't listen to the commit. > > The commit in the renderer process would presumably destroy the > RenderFrameProxy. Would that mean that we would end up with no proxy when we > should have one? Yikes. Nick had an idea for this. If we get here when is_frame_tree_ is true, then the renderer process has made the RenderFrame active and destroyed the RenderFrameProxy, while the browser process has destroyed the RenderFrameHost and assumes the RenderFrameProxyHost still exists. In this case, could we do a swap back to a RenderFrameProxy here (still destroying the RenderFrame as requested), perhaps with the same routing ID as the RenderFrameProxyHost in the browser process? The one catch is that we might have also sent an IPC from the renderer to the browser to destroy the RenderFrameProxyHost. (Is that true?) Maybe the browser process can ignore that IPC if it somehow recognizes we're in this case?
Description was changed from ========== Fix leaking of RenderFrames on cancelled cross-process navigations. This CL adds an explicit IPC from the browser process to the renderer process to delete RenderFrame objects, which were used for pending cross-process navigations. BUG=548275 ========== to ========== Fix leaking of RenderFrames on cancelled cross-process navigations. This CL adds an explicit IPC from the browser process to the renderer process to delete RenderFrame objects, which were used for pending cross-process navigations. BUG=548275 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Thanks. Mostly looks good with nits, but one question about the race I mentioned. https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:742: void RenderFrameImpl::DetachFrame(int routing_id) { On 2015/12/11 00:47:25, nasko wrote: > On 2015/12/11 00:03:17, Charlie Reis wrote: > > Is this ever meant to be called when in_frame_tree_ is true? Maybe we should > > DCHECK if not. > > With my latest change it is supposed to. It detaches a frame regardless whether > it is in the tree or not. I'm open to tweaking the name if we have a better one. > > > Actually, I just thought of a possible race where that could happen, and it > > might get us into trouble. Suppose the browser process cancels a slow pending > > RFH just as it's committing (e.g., due to a cross-process navigation to a > > different site). The renderer process will have committed the RF and set > > in_frame_tree_ to true, while the browser process has destroyed the RFH and > > won't listen to the commit. > > With the latest patch version, the RFH destruction will also destroy the RF, > since the IPC is sent from the RFH destructor. > > > The commit in the renderer process would presumably destroy the > > RenderFrameProxy. Would that mean that we would end up with no proxy when we > > should have one? Yikes. > > Actually, a bit different now - the frame will get completely removed, as the > commit would have happened, the proxy will be gone and if the IPC to destroy the > RenderFrame comes after that, it will totally remove the frame from the tree. : > ( This sounds like it's still bad, right? We could have a RenderFrameProxyHost in the browser process and no RenderFrameProxy (or RenderFrame) in the renderer process. Should we be trying to recreate the RenderFrameProxy? (Did you want to address this separately?) https://codereview.chromium.org/1409693009/diff/300001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1409693009/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:269: if (!frame_tree_node_->IsMainFrame() && render_frame_created_) Hmm, this condition and the comment above seem to be talking about different things. It's not obvious how we're skipping the IPC in the "swapping with a RenderFrameProxy" case-- maybe that makes IsRFHStateActive return false? I think both of these blocks might be easier to read if we split them up. Something like: bool is_active = IsRFHStateActive(rfh_state_); // If this is swapping out, it already decremented the active frame count // of the SiteInstance it belongs to. if (is_active) GetSiteInstance()->decrement_active_frame_count(); // If this is swapping with a RenderFrameProxy, the RenderFrame will already // be deleted in the renderer process. Main frame RenderFrames will be // cleaned up as part of deleting their RenderView. In all other cases, the // RenderFrame should be cleaned up (if it exists). if (is_active && !frame_tree_node_->IsMainFrame() && render_frame_created_) Send(new FrameMsg_Detach(routing_id_)); https://codereview.chromium.org/1409693009/diff/300001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:745: // TODO(nasko): Investigate why it is possible to receive a FrameMsg_Detach Wouldn't it be possible for the renderer process to have detached the frame just before receiving this IPC? I don't think we'd have big problems in that case, so maybe we don't need the TODO. https://codereview.chromium.org/1409693009/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:815: is_local_root_(false), We set is_main_frame_ to true by default. Should we do the same for is_local_root_ to be consistent, setting it to false later if needed? Actually, it looks like we're missing any assignments (or references) to this at all. :) Maybe it should be removed? https://codereview.chromium.org/1409693009/diff/300001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1409693009/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.h:945: // Frame is a local root if it is rendered in a process different than parent nit: A frame nit: than its parent https://codereview.chromium.org/1409693009/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.h:954: // Indicates which of the two states this frame is currently in. nit: Indicates whether the frame has been inserted into the frame tree yet or not.
https://codereview.chromium.org/1409693009/diff/280001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/1409693009/diff/280001/content/common/frame_m... content/common/frame_messages.h:515: IPC_MESSAGE_CONTROL1(FrameMsg_Detach, int /* routing_id */) I think I would personally see this as a routed message: I see it as reuse of the existing routing machinery. In spirit, this is pretty similar to RenderWidget::OnClose(), which essentially self-deletes (by calling Release()). https://codereview.chromium.org/1409693009/diff/280001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1409693009/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.h:961: bool in_frame_tree_; You can TODO(dcheng) to clean this up: FrameTreeHandle will make it trivial to detect provisional frames in the web layer, and I'll likely expose it since having to store this is silly (but necessary at the moment).
Patchset #15 (id:320001) has been deleted
Description was changed from ========== Fix leaking of RenderFrames on cancelled cross-process navigations. This CL adds an explicit IPC from the browser process to the renderer process to delete RenderFrame objects, which were used for pending cross-process navigations. BUG=548275 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix leaking of RenderFrames. This CL adds an explicit IPC from the browser process to the renderer process to delete RenderFrame objects, when the corresponding RenderFrameHost is deleted on the browser side. It fixes two known leaks: * cancelling a navigation in a pending RenderFrameHost * parent frame deleting from DOM a child frame in a remote process BUG=548275 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
This includes fixes for issues from both Charlie and Daniel. https://codereview.chromium.org/1409693009/diff/280001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/1409693009/diff/280001/content/common/frame_m... content/common/frame_messages.h:515: IPC_MESSAGE_CONTROL1(FrameMsg_Detach, int /* routing_id */) On 2015/12/11 23:40:17, dcheng wrote: > I think I would personally see this as a routed message: I see it as reuse of > the existing routing machinery. > > In spirit, this is pretty similar to RenderWidget::OnClose(), which essentially > self-deletes (by calling Release()). Done. https://codereview.chromium.org/1409693009/diff/280001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1409693009/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.h:961: bool in_frame_tree_; On 2015/12/11 23:40:17, dcheng wrote: > You can TODO(dcheng) to clean this up: FrameTreeHandle will make it trivial to > detect provisional frames in the web layer, and I'll likely expose it since > having to store this is silly (but necessary at the moment). Done. https://codereview.chromium.org/1409693009/diff/300001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1409693009/diff/300001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:269: if (!frame_tree_node_->IsMainFrame() && render_frame_created_) On 2015/12/11 20:10:14, Charlie Reis wrote: > Hmm, this condition and the comment above seem to be talking about different > things. It's not obvious how we're skipping the IPC in the "swapping with a > RenderFrameProxy" case-- maybe that makes IsRFHStateActive return false? > > I think both of these blocks might be easier to read if we split them up. > Something like: > > bool is_active = IsRFHStateActive(rfh_state_); > > // If this is swapping out, it already decremented the active frame count > // of the SiteInstance it belongs to. > if (is_active) > GetSiteInstance()->decrement_active_frame_count(); > > // If this is swapping with a RenderFrameProxy, the RenderFrame will already > // be deleted in the renderer process. Main frame RenderFrames will be > // cleaned up as part of deleting their RenderView. In all other cases, the > // RenderFrame should be cleaned up (if it exists). > if (is_active && !frame_tree_node_->IsMainFrame() && render_frame_created_) > Send(new FrameMsg_Detach(routing_id_)); Done. https://codereview.chromium.org/1409693009/diff/300001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:745: // TODO(nasko): Investigate why it is possible to receive a FrameMsg_Detach On 2015/12/11 20:10:14, Charlie Reis wrote: > Wouldn't it be possible for the renderer process to have detached the frame just > before receiving this IPC? I don't think we'd have big problems in that case, > so maybe we don't need the TODO. Done. https://codereview.chromium.org/1409693009/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:815: is_local_root_(false), On 2015/12/11 20:10:14, Charlie Reis wrote: > We set is_main_frame_ to true by default. Should we do the same for > is_local_root_ to be consistent, setting it to false later if needed? > > Actually, it looks like we're missing any assignments (or references) to this at > all. :) Maybe it should be removed? I have no recollection what that was here ... oh well, removed. Thanks for catching it! https://codereview.chromium.org/1409693009/diff/300001/content/renderer/rende... File content/renderer/render_frame_impl.h (right): https://codereview.chromium.org/1409693009/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.h:945: // Frame is a local root if it is rendered in a process different than parent On 2015/12/11 20:10:15, Charlie Reis wrote: > nit: A frame > nit: than its parent This is just gone. https://codereview.chromium.org/1409693009/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.h:954: // Indicates which of the two states this frame is currently in. On 2015/12/11 20:10:15, Charlie Reis wrote: > nit: Indicates whether the frame has been inserted into the frame tree yet or > not. Done.
Thanks, LGTM with nits. https://codereview.chromium.org/1409693009/diff/340001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/340001/content/renderer/rende... content/renderer/render_frame_impl.cc:1451: void RenderFrameImpl::OnDeleteFrame() { We're putting off the cancel/commit race bug for later, right? Can you file a bug for that one and optionally mention it in a TODO here? (If we receive this after we have just committed, we probably need to resurrect the proxy we destroyed.) https://codereview.chromium.org/1409693009/diff/340001/content/renderer/rende... content/renderer/render_frame_impl.cc:1453: frame_->detach(); Sanity check: We aren't creating UaFs by doing this, after this method returns? Let's at least add a comment saying that this will trigger a call to frameDetached and thus |this| will be deleted.
Another round of nits fixed. https://codereview.chromium.org/1409693009/diff/340001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/340001/content/renderer/rende... content/renderer/render_frame_impl.cc:1451: void RenderFrameImpl::OnDeleteFrame() { On 2015/12/14 21:22:39, Charlie Reis wrote: > We're putting off the cancel/commit race bug for later, right? Can you file a > bug for that one and optionally mention it in a TODO here? (If we receive this > after we have just committed, we probably need to resurrect the proxy we > destroyed.) Done. https://codereview.chromium.org/1409693009/diff/340001/content/renderer/rende... content/renderer/render_frame_impl.cc:1453: frame_->detach(); On 2015/12/14 21:22:39, Charlie Reis wrote: > Sanity check: We aren't creating UaFs by doing this, after this method returns? > > Let's at least add a comment saying that this will trigger a call to > frameDetached and thus |this| will be deleted. Done.
https://codereview.chromium.org/1409693009/diff/360001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:1453: // swapped a RenderFrameProxy with RenderFrame, the proxy need to be nit: with this nit: needs https://codereview.chromium.org/1409693009/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:1454: // recreated instead of the frame being deleted. Don't we need to delete the frame as well as recreate the proxy?
lgtm https://codereview.chromium.org/1409693009/diff/360001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1409693009/diff/360001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:272: if (is_active && !frame_tree_node_->IsMainFrame() && render_frame_created_) Nit: I would personally find this easier to read if this were merged into the above if (is_active)
https://codereview.chromium.org/1409693009/diff/360001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1409693009/diff/360001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:272: if (is_active && !frame_tree_node_->IsMainFrame() && render_frame_created_) On 2015/12/14 22:14:41, dcheng wrote: > Nit: I would personally find this easier to read if this were merged into the > above if (is_active) I asked for this change because the previous comments were hard to follow when they were merged under the same block. (The comments were confusingly referring to the outer condition when placed above an inner condition.) It's possible to rephrase the whole thing and merge them into one block, but I think it's fairly readable as is.
https://codereview.chromium.org/1409693009/diff/360001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1409693009/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:1453: // swapped a RenderFrameProxy with RenderFrame, the proxy need to be On 2015/12/14 22:12:52, Charlie Reis wrote: > nit: with this > nit: needs Done. https://codereview.chromium.org/1409693009/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:1454: // recreated instead of the frame being deleted. On 2015/12/14 22:12:52, Charlie Reis wrote: > Don't we need to delete the frame as well as recreate the proxy? Done.
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1409693009/#ps380001 (title: "Rebased on ToT and couple of more nits fixes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1409693009/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1409693009/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1409693009/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1409693009/380001
Message was sent while issue was closed.
Description was changed from ========== Fix leaking of RenderFrames. This CL adds an explicit IPC from the browser process to the renderer process to delete RenderFrame objects, when the corresponding RenderFrameHost is deleted on the browser side. It fixes two known leaks: * cancelling a navigation in a pending RenderFrameHost * parent frame deleting from DOM a child frame in a remote process BUG=548275 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix leaking of RenderFrames. This CL adds an explicit IPC from the browser process to the renderer process to delete RenderFrame objects, when the corresponding RenderFrameHost is deleted on the browser side. It fixes two known leaks: * cancelling a navigation in a pending RenderFrameHost * parent frame deleting from DOM a child frame in a remote process BUG=548275 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #17 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Fix leaking of RenderFrames. This CL adds an explicit IPC from the browser process to the renderer process to delete RenderFrame objects, when the corresponding RenderFrameHost is deleted on the browser side. It fixes two known leaks: * cancelling a navigation in a pending RenderFrameHost * parent frame deleting from DOM a child frame in a remote process BUG=548275 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix leaking of RenderFrames. This CL adds an explicit IPC from the browser process to the renderer process to delete RenderFrame objects, when the corresponding RenderFrameHost is deleted on the browser side. It fixes two known leaks: * cancelling a navigation in a pending RenderFrameHost * parent frame deleting from DOM a child frame in a remote process BUG=548275 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/eab5c55815be54a878e9a79aa68920a3d6db4a93 Cr-Commit-Position: refs/heads/master@{#365156} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/eab5c55815be54a878e9a79aa68920a3d6db4a93 Cr-Commit-Position: refs/heads/master@{#365156} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
