Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1886413002:
Always swap with a replacement proxy in OnSwapOut. (Closed)
|
Created:
4 years, 8 months ago by alexmos Modified:
4 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, mlamouri+watch-blink_chromium.org, nasko+codewatch_chromium.org, jam, dcheng, blink-reviews, darin-cc_chromium.org, ajwong+watch_chromium.org, mkwst+moarreviews-renderer_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. |
DescriptionAlways swap with a replacement proxy in OnSwapOut.
Currently, if a frame is the last active frame in the process and it
gets swapped out, OnSwapOut is not passed a proxy routing ID, and so
it leaves the RenderFrame alone without swapping it or destroying it.
(Later on, closing the RenderView detaches all nodes in the frame
tree, which cleans up that RenderFrame.) This leads to the complexity
of dealing with this special case and has indirectly led to various
races involving reuse of the RenderViewHost for a RFH pending
shutdown. See https://codereview.chromium.org/1835833002 and issues
515302, 581912, 544755, and 591478 for some context.
Additionally, this led to problems of trying to later use the
RenderFrame that was left hanging around by OnSwapOut. For instance,
when this is done for a subframe, there is a race where the subframe's
WebFrameWidget is cleaned up before the RenderView is closed, and the
RenderFrame tried to reference the null widget as part of being
detached when the RenderView is closed (see
https://crbug.com/568836#c15).
This CL changes SwapOut to always swap with a proxy. In the cases
when the last active frame in the process is going away, the proxy
will be short-lived and will be deleted as soon as the RFH pending
shutdown is deleted. To facilitate this, the decrementing of active
frame count moves from RFH::SwapOut to RFH::OnSwappedOut. This also
lets us get rid of the whole concept of RenderViewHosts pending
shutdown, making it fine to reuse a RVH when its RFH is pending
shutdown and just rely on RVH's refcount to keep it alive and
destroy it. The extra short-lived proxy should be well worth
significantly reduced code complexity.
BUG=568836, 515302, 581912, 544755, 591478
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/9aa6123974ab0f85aecf20cfc6e377071f99b386
Cr-Commit-Position: refs/heads/master@{#389904}
Patch Set 1 #Patch Set 2 : Fix test #Patch Set 3 : Fix ExtensionWebRequestApiTest.WebRequestBlocking #Patch Set 4 : Fix test for Android and cleanup #Patch Set 5 : Nits #
Total comments: 10
Patch Set 6 : Try crazy approach: always create proxy in SwapOut #Patch Set 7 : Remove RVH pending deletion logic completely #Patch Set 8 : Undo unintentional test change #Patch Set 9 : Nits #Patch Set 10 : Fix compile #
Total comments: 19
Patch Set 11 : Charlie's comments #
Total comments: 1
Messages
Total messages: 26 (12 generated)
Description was changed from ========== Delete RenderFrame in all cases in OnSwapOut. BUG=568836 ========== to ========== Delete RenderFrame in all cases in OnSwapOut. BUG=568836 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by alexmos@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/1886413002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1886413002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Description was changed from ========== Delete RenderFrame in all cases in OnSwapOut. BUG=568836 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Delete RenderFrame when there is no replacement proxy in OnSwapOut. BUG=568836 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Delete RenderFrame when there is no replacement proxy in OnSwapOut. BUG=568836 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Delete RenderFrame when there is no replacement proxy in OnSwapOut. BUG=568836 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Delete RenderFrame when there is no replacement proxy in OnSwapOut. BUG=568836 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Delete RenderFrame when there is no replacement proxy in OnSwapOut. Currently, OnSwapOut deletes the RenderFrame as part of WebFrame::swap() if there is a replacement proxy, but leaves it alone if there is no proxy. In the latter case, the frame is the last active frame in the SiteInstance and is detached later as part of closing the RenderView, which detaches the main frame and all its children. Unfortunately, for subframes this is prone to a race, where the WebFrameWidget is cleaned up before the RenderView is closed, and the RenderFrame tries to reference the null widget as part of being detached when the RenderView is closed (see https://crbug.com/568836#c15). This CL changes OnSwapOut to also delete subframes when there isn't a replacement proxy, so that we detach the frame earlier, before the browser process sends a ViewMsg_Close to its widget, so that there is no time window when the frame is live without its widget. BUG=568836 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
alexmos@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
Charlie, Nasko - what do you think about this? The full description of the problem this is trying to fix is in https://crbug.com/568836#c15. There are two solutions we were thinking about: adapt localRoot() to deal with the possibility of a null WebFrameWidget (when closing), or make it impossible to get into that situation by reordering cleanup. Daniel and I wanted to try the latter route, which is this CL. Curious about your thoughts. https://codereview.chromium.org/1886413002/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1886413002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:1613: } else if (!is_main_frame) { I initially tried to do this for all frames for consistency, but ran into an issue for main frames: this leaves the view's mainFrame() as null until the view is closed, which should happen soon afterward. That actually caused ExtensionWebRequestApiTest.WebRequestBlocking to fail, where in that time window, there was a zoom change, and RenderViewZoomer::Visit (which walks through all RVs in the process) tried to reference this view's webview->mainFrame() [1]. This zoom code is actually going away in James's https://codereview.chromium.org/1804023002/, so we could try making that change for all frames after that, though I don't know if something else might hit this issue (there are a few other uses of RenderView::ForEach). Essentially we'd be trading a time window where a WebLocalFrame has no frameWidget() for a time window where the webview doesn't have a mainFrame(), and I don't know which one's worse. :) [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
Wow, nice investigation. Definitely a tricky situation. On 2016/04/15 17:05:12, alexmos wrote: > Charlie, Nasko - what do you think about this? > > The full description of the problem this is trying to fix is in > https://crbug.com/568836#c15. There are two solutions we were thinking about: > adapt localRoot() to deal with the possibility of a null WebFrameWidget (when > closing), or make it impossible to get into that situation by reordering > cleanup. Daniel and I wanted to try the latter route, which is this CL. Curious > about your thoughts. I do like the idea of making sure the RenderFrame is always deleted by the end of OnSwapOut, but unfortunately we're not getting that property, since we skip that if it's a main frame and there's no proxy. That said, I agree that simply handling the null case in localRoot() feels like papering over this. Crazy idea: Would it be useful to always swap with a proxy in OnSwapOut, even when we're going away? That's fewer cases to reason about, and we would never have a null WebView::mainFrame(). On the flip side, it does seem like mostly unnecessary work, and I have no idea if it's complicated in practice. It's just appealing to me because it could potentially simplify all this logic. https://codereview.chromium.org/1886413002/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1886413002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:1613: } else if (!is_main_frame) { On 2016/04/15 17:05:12, alexmos wrote: > I initially tried to do this for all frames for consistency, but ran into an > issue for main frames: this leaves the view's mainFrame() as null until the view > is closed, which should happen soon afterward. That actually caused > ExtensionWebRequestApiTest.WebRequestBlocking to fail, where in that time > window, there was a zoom change, and RenderViewZoomer::Visit (which walks > through all RVs in the process) tried to reference this view's > webview->mainFrame() [1]. This zoom code is actually going away in James's > https://codereview.chromium.org/1804023002/, so we could try making that change > for all frames after that, though I don't know if something else might hit this > issue (there are a few other uses of RenderView::ForEach). Essentially we'd be > trading a time window where a WebLocalFrame has no frameWidget() for a time > window where the webview doesn't have a mainFrame(), and I don't know which > one's worse. :) > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... On one hand, there are 400 references to WebView::mainFrame(), so having that return null near shutdown does seem risky. I suppose it's not a problem in the case that we have a proxy during OnSwapOut because WebView::mainFrame() can return a WebRemoteFrame. On the other hand, could we ever be at risk of the current crash for main frames? Suppose we're swapping out the last main frame and not creating a proxy. Could we get to RenderWidget::Close() and clear the Web{Frame}Widget of the main frame, setting up a crash if some code calls GetRenderWidget() from RenderViewImpl::Close()? (I realize we don't have a WebFrameWidget for the main frame yet, but we plan to, and perhaps this applies to the WebWidget for now?) It does seem unfortunate that we can't uniformly guarantee that the frame is gone by the end of OnSwapOut-- it seems like it will create corner cases that lead to more bugs like this in general. https://codereview.chromium.org/1886413002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:1621: OnDeleteFrame(); I'm assuming that using OnDeleteFrame() rather than frame->detach() is intentional, so that we don't notify the browser (due to setting in_browser_initiated_detach_ to true)? If so, we should update the comments on all the appearances of in_browser_initiated_detach_ (and possibly the name itself), since they would be stale after this.
Just some nits on the code itself. Charlie, I like the crazy idea. Tracing through some code, it looks like proxy cleanup will work out nicely if instead of decrementing the active frame count in RFH::SwapOut, we do it in RFH::OnSwappedOut. The proxy will be basically short lived, we will add an extra IPC to delete it, and will keep the memory in the renderer around for short period. I think we should explore what it would actually look like. https://codereview.chromium.org/1886413002/diff/80001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/1886413002/diff/80001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:6451: // RenderWidget and RenderView, in that order. In the bug, the latter crashed nit: "and the RenderView". https://codereview.chromium.org/1886413002/diff/80001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:6494: subframe_process->DecrementWorkerRefCount(); Do we care to wait for process termination at all? I suspect no, since the original bug would've caused a crash by now. https://codereview.chromium.org/1886413002/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1886413002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:1621: OnDeleteFrame(); On 2016/04/15 23:20:05, Charlie Reis wrote: > I'm assuming that using OnDeleteFrame() rather than frame->detach() is > intentional, so that we don't notify the browser (due to setting > in_browser_initiated_detach_ to true)? > > If so, we should update the comments on all the appearances of > in_browser_initiated_detach_ (and possibly the name itself), since they would be > stale after this. This method is called by the browser, so in a sense if we are detaching here, it is browser initiated.
Description was changed from ========== Delete RenderFrame when there is no replacement proxy in OnSwapOut. Currently, OnSwapOut deletes the RenderFrame as part of WebFrame::swap() if there is a replacement proxy, but leaves it alone if there is no proxy. In the latter case, the frame is the last active frame in the SiteInstance and is detached later as part of closing the RenderView, which detaches the main frame and all its children. Unfortunately, for subframes this is prone to a race, where the WebFrameWidget is cleaned up before the RenderView is closed, and the RenderFrame tries to reference the null widget as part of being detached when the RenderView is closed (see https://crbug.com/568836#c15). This CL changes OnSwapOut to also delete subframes when there isn't a replacement proxy, so that we detach the frame earlier, before the browser process sends a ViewMsg_Close to its widget, so that there is no time window when the frame is live without its widget. BUG=568836 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Always swap with a replacement proxy in OnSwapOut. Currently, if a frame is the last active frame in the process and it gets swapped out, OnSwapOut is not passed a proxy routing ID, and so it leaves the RenderFrame alone without swapping it or destroying it. (Later on, closing the RenderView detaches all nodes in the frame tree, which cleans up that RenderFrame.) This leads to the complexity of dealing with this special case and has indirectly led to various races involving reuse of the RenderViewHost for a RFH pending shutdown. See https://codereview.chromium.org/1835833002 and issues 515302, 581912, 544755, and 591478 for some context. Additionally, this led to problems of trying to later use the RenderFrame that was left hanging around by OnSwapOut. For instance, when this is done for a subframe, there is a race where the subframe's WebFrameWidget is cleaned up before the RenderView is closed, and the RenderFrame tried to reference the null widget as part of being detached when the RenderView is closed (see https://crbug.com/568836#c15). This CL changes SwapOut to always swap with a proxy. In the cases when the last active frame in the process is going away, the proxy will be short-lived and will be deleted as soon as the RFH pending shutdown is deleted. To facilitate this, the decrementing of active frame count moves from RFH::SwapOut to RFH::OnSwappedOut. This also lets us get rid of the whole concept of RenderViewHosts pending shutdown, making it fine to reuse a RVH when its RFH is pending shutdown and just rely on RVH's refcount to keep it alive and destroy it. The extra short-lived proxy should be well worth significantly reduced code complexity. BUG=568836 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Always swap with a replacement proxy in OnSwapOut. Currently, if a frame is the last active frame in the process and it gets swapped out, OnSwapOut is not passed a proxy routing ID, and so it leaves the RenderFrame alone without swapping it or destroying it. (Later on, closing the RenderView detaches all nodes in the frame tree, which cleans up that RenderFrame.) This leads to the complexity of dealing with this special case and has indirectly led to various races involving reuse of the RenderViewHost for a RFH pending shutdown. See https://codereview.chromium.org/1835833002 and issues 515302, 581912, 544755, and 591478 for some context. Additionally, this led to problems of trying to later use the RenderFrame that was left hanging around by OnSwapOut. For instance, when this is done for a subframe, there is a race where the subframe's WebFrameWidget is cleaned up before the RenderView is closed, and the RenderFrame tried to reference the null widget as part of being detached when the RenderView is closed (see https://crbug.com/568836#c15). This CL changes SwapOut to always swap with a proxy. In the cases when the last active frame in the process is going away, the proxy will be short-lived and will be deleted as soon as the RFH pending shutdown is deleted. To facilitate this, the decrementing of active frame count moves from RFH::SwapOut to RFH::OnSwappedOut. This also lets us get rid of the whole concept of RenderViewHosts pending shutdown, making it fine to reuse a RVH when its RFH is pending shutdown and just rely on RVH's refcount to keep it alive and destroy it. The extra short-lived proxy should be well worth significantly reduced code complexity. BUG=568836 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Always swap with a replacement proxy in OnSwapOut. Currently, if a frame is the last active frame in the process and it gets swapped out, OnSwapOut is not passed a proxy routing ID, and so it leaves the RenderFrame alone without swapping it or destroying it. (Later on, closing the RenderView detaches all nodes in the frame tree, which cleans up that RenderFrame.) This leads to the complexity of dealing with this special case and has indirectly led to various races involving reuse of the RenderViewHost for a RFH pending shutdown. See https://codereview.chromium.org/1835833002 and issues 515302, 581912, 544755, and 591478 for some context. Additionally, this led to problems of trying to later use the RenderFrame that was left hanging around by OnSwapOut. For instance, when this is done for a subframe, there is a race where the subframe's WebFrameWidget is cleaned up before the RenderView is closed, and the RenderFrame tried to reference the null widget as part of being detached when the RenderView is closed (see https://crbug.com/568836#c15). This CL changes SwapOut to always swap with a proxy. In the cases when the last active frame in the process is going away, the proxy will be short-lived and will be deleted as soon as the RFH pending shutdown is deleted. To facilitate this, the decrementing of active frame count moves from RFH::SwapOut to RFH::OnSwappedOut. This also lets us get rid of the whole concept of RenderViewHosts pending shutdown, making it fine to reuse a RVH when its RFH is pending shutdown and just rely on RVH's refcount to keep it alive and destroy it. The extra short-lived proxy should be well worth significantly reduced code complexity. BUG=568836 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
OK, I think this is ready for another look. The latest PS totally changes direction and implements the crazy always-swap-with-proxy idea (thanks Charlie!) as well as removes all the RVH pending deletion logic. See the updated description as well. The trybots seem happy! I had to tweak expectations/comments in a few tests (mostly from Charlie's https://crrev.com/1835833002) to deal with the RVH pending shutdown checks and to reflect that the RVH can now be reused when the corresponding RFH is pending shutdown. The RFHM tests from that CL actually weren't crashing regardless of the RVH reuse, thanks to Charlie's fix of calling OnSwappedOut() in OnRenderProcessGone. https://codereview.chromium.org/1886413002/diff/80001/content/browser/site_pe... File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/1886413002/diff/80001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:6451: // RenderWidget and RenderView, in that order. In the bug, the latter crashed On 2016/04/18 21:24:26, nasko (slow) wrote: > nit: "and the RenderView". Done. (I used "and to the RenderView".) https://codereview.chromium.org/1886413002/diff/80001/content/browser/site_pe... content/browser/site_per_process_browsertest.cc:6494: subframe_process->DecrementWorkerRefCount(); On 2016/04/18 21:24:26, nasko (slow) wrote: > Do we care to wait for process termination at all? I suspect no, since the > original bug would've caused a crash by now. Correct, we don't need to wait for process termination, as the original bug would've crashed while waiting for the shutdown request above. The DecrementWorkerRefCount isn't actually necessary (everything will die anyway when the test shuts down). I thought it'd be cleaner to have the matching call but could also remove it. https://codereview.chromium.org/1886413002/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1886413002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:1613: } else if (!is_main_frame) { On 2016/04/15 23:20:05, Charlie Reis wrote: > On 2016/04/15 17:05:12, alexmos wrote: > > I initially tried to do this for all frames for consistency, but ran into an > > issue for main frames: this leaves the view's mainFrame() as null until the > view > > is closed, which should happen soon afterward. That actually caused > > ExtensionWebRequestApiTest.WebRequestBlocking to fail, where in that time > > window, there was a zoom change, and RenderViewZoomer::Visit (which walks > > through all RVs in the process) tried to reference this view's > > webview->mainFrame() [1]. This zoom code is actually going away in James's > > https://codereview.chromium.org/1804023002/, so we could try making that > change > > for all frames after that, though I don't know if something else might hit > this > > issue (there are a few other uses of RenderView::ForEach). Essentially we'd > be > > trading a time window where a WebLocalFrame has no frameWidget() for a time > > window where the webview doesn't have a mainFrame(), and I don't know which > > one's worse. :) > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > On one hand, there are 400 references to WebView::mainFrame(), so having that > return null near shutdown does seem risky. I suppose it's not a problem in the > case that we have a proxy during OnSwapOut because WebView::mainFrame() can > return a WebRemoteFrame. > > On the other hand, could we ever be at risk of the current crash for main > frames? Suppose we're swapping out the last main frame and not creating a > proxy. Could we get to RenderWidget::Close() and clear the Web{Frame}Widget of > the main frame, setting up a crash if some code calls GetRenderWidget() from > RenderViewImpl::Close()? (I realize we don't have a WebFrameWidget for the main > frame yet, but we plan to, and perhaps this applies to the WebWidget for now?) > > It does seem unfortunate that we can't uniformly guarantee that the frame is > gone by the end of OnSwapOut-- it seems like it will create corner cases that > lead to more bugs like this in general. Agree with all your concerns, so I'm super-happy that this went away after switching to always swap with a proxy. https://codereview.chromium.org/1886413002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:1621: OnDeleteFrame(); On 2016/04/18 21:24:26, nasko (slow) wrote: > On 2016/04/15 23:20:05, Charlie Reis wrote: > > I'm assuming that using OnDeleteFrame() rather than frame->detach() is > > intentional, so that we don't notify the browser (due to setting > > in_browser_initiated_detach_ to true)? > > > > If so, we should update the comments on all the appearances of > > in_browser_initiated_detach_ (and possibly the name itself), since they would > be > > stale after this. > > This method is called by the browser, so in a sense if we are detaching here, it > is browser initiated. Likewise, this concern is gone in the latest PS. :) https://codereview.chromium.org/1886413002/diff/180001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (left): https://codereview.chromium.org/1886413002/diff/180001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:4182: EXPECT_TRUE(deleted_observer.deleted()); I'm leaving dealing with issue 535246 (race trying to use a process being cleaned up via renderer-initiated shutdown) for a followup CL, as I think the fix is going to be independent from this. See the new comment on process_exit_observer about it, and also issue . I think it's fine to leave this test as-is without simulating a SwapOut ACK even when 535246 is fixed, as it still leads to a path that used to crash in 515302 - hence the removal of this TODO. The 535246 scenario can be tested separately in another test, and I added a note about how to do that on the bottom of the new CloseSubframeWidgetAndViewOnProcessExit test.
This is so nice!! I have two questions below, but I really like how this reduces the complexity. https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/frame_tree.cc:343: CHECK(iter != render_view_host_map_.end()); Would CHECK_NE work? https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... File content/browser/frame_host/frame_tree.h (left): https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/frame_tree.h:242: // their state is already gone away). Sanity check: Is there state that has gone away that we need to be concerned about, or is this stale? For example, what about main_frame_routing_id_? That gets set in the RVH constructor, and updated at commit time. If we didn't reuse a pending delete RVH and created a new one instead, it would have a main_frame_routing_id_ at construction. That impacts RenderViewImpl::Initialize. Now we might have a stale main_frame_routing_id_ until commit time, which could do something wrong in RenderViewImpl::Initialize? (Not sure, since your tests are green.) Do we need to update this value sooner? (I ask because it's related to many of the other crashes we've seen in this area.) https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (left): https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1240: GetSiteInstance()->DecrementActiveFrameCount(); This seems like a real change-- pending delete RFHs will now be considered active frames. I see how it's necessary for this CL, but I'm wondering what the implications are. (Maybe it's an improvement over how things were before? It looks like we might have been able to kill the process before its unload handler finished, for example.) Some questions: 1) What happens if this RFH is destroyed (e.g., by closing the tab or detaching the frame) while it's in the pending delete state? ~RFHI won't call DecrementActiveFrameCount because is_active() returns false. It almost looks like we could unconditionally call DecrementActiveFrameCount in the RFHI destructor and not bother calling in OnSwappedOut, since OnSwappedOut should lead to deletion. 2) What were the implications of it being decremented before swap out before? I see that active_frame_count() was used when deciding whether to create proxies on SwapOut, so that's moot now (i.e., we always create the proxy). We also call active_frame_count() in DiscardUnusedFrame to decide whether to create a proxy after canceling a pending RFH. I'm less clear here-- we will now treat pending delete RFHs as a reason to create a proxy when canceling a pending RFH. To be honest, though, I don't see that as a big problem. Once the pending delete RFH goes away and the count reaches 0, I assume that extra proxy can be cleaned up? (I don't see where that happens at first glance.) If so, this change seems safe to me. https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1220: DCHECK(proxy); Given the trouble we've had here, let's make this a CHECK. :) https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1223: int proxy_routing_id = proxy->GetRoutingID(); nit: Move proxy_routing_id and replication_state into the IsRenderFrameLive block, since that's the only place they're used. https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1392: GetSiteInstance()->DecrementActiveFrameCount(); As noted above, we may just want to do this unconditionally in the destructor. https://codereview.chromium.org/1886413002/diff/180001/content/browser/site_p... File content/browser/site_per_process_browsertest.cc (left): https://codereview.chromium.org/1886413002/diff/180001/content/browser/site_p... content/browser/site_per_process_browsertest.cc:4182: EXPECT_TRUE(deleted_observer.deleted()); On 2016/04/22 05:09:34, alexmos wrote: > I'm leaving dealing with issue 535246 (race trying to use a process being > cleaned up via renderer-initiated shutdown) for a followup CL, as I think the > fix is going to be independent from this. See the new comment on > process_exit_observer about it, and also issue . I think it's fine to leave > this test as-is without simulating a SwapOut ACK even when 535246 is fixed, as > it still leads to a path that used to crash in 515302 - hence the removal of > this TODO. The 535246 scenario can be tested separately in another test, and I > added a note about how to do that on the bottom of the new > CloseSubframeWidgetAndViewOnProcessExit test. Acknowledged. https://codereview.chromium.org/1886413002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1886413002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:1585: // start using the RenderFrameProxy, if one is created. nit: Drop "if one is created"
Thanks for reviewing! https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/frame_tree.cc:343: CHECK(iter != render_view_host_map_.end()); On 2016/04/22 19:11:33, Charlie Reis wrote: > Would CHECK_NE work? I had that originally, but that failed to compile in PS9. https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... File content/browser/frame_host/frame_tree.h (left): https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/frame_tree.h:242: // their state is already gone away). On 2016/04/22 19:11:33, Charlie Reis wrote: > Sanity check: Is there state that has gone away that we need to be concerned > about, or is this stale? > > For example, what about main_frame_routing_id_? That gets set in the RVH > constructor, and updated at commit time. If we didn't reuse a pending delete > RVH and created a new one instead, it would have a main_frame_routing_id_ at > construction. That impacts RenderViewImpl::Initialize. Now we might have a > stale main_frame_routing_id_ until commit time, which could do something wrong > in RenderViewImpl::Initialize? (Not sure, since your tests are green.) > > Do we need to update this value sooner? (I ask because it's related to many of > the other crashes we've seen in this area.) Good question, and the answer is complicated. :) I actually think this might not be specific to pending deletion RVHs -- if we have an A-embed-B and we navigate to B, today that will reuse the old RVH (which was previously swapped out) even without these changes. (I wonder if that's ok and intentional? Maybe we shouldn't reuse a RV for a new main frame in such cases.) For the above scenario, the common case is that we reuse the RVH that is live, so we skip the call to create it, and thus we never get to RVI::Initialize on this path. Instead, we go through RFI::CreateFrame (sent for the main frame). That creates the RF and leaves the RV's main_render_frame_ as null until commit, at which point RenderFrameImpl::didCommitProvisionalLoad updates it to the committed frame. This actually makes sense, since the RV's main frame is technically a proxy until that commit happens, so wouldn't be right to update it earlier. Following the same reasoning, updating main_frame_routing_id_ on the browser process side earlier (before commit) might not be right, since the RVH is still technically swapped out? The alternative to all this is to update main_frame_routing_id_ in FrameTree::CreateRenderViewHost, in the case where that decides to reuse an existing one, but that seems like it won't be consistent with the renderer (I haven't tried this yet, so not sure what other side effects that might have). If the RVH is not live when we decide to reuse it, that seems like it would get us into trouble here. But it actually seems to also work: we get into CreateRenderView with a main_frame_routing_id_ of NONE and proceed to create a new RV which is swapped out, then we make a separate CreateRenderFrame call to create the main frame (this is done because the main frame isn't live yet after creating the view [1]) and then we update the view's main frame at commit time as before. This whole scenario seems fragile though. I'm wondering if Nasko remembers anything about the "part of their state is already gone away" comment. It first appeared in clamy@'s unload handler CL, https://codereview.chromium.org/88503002, which you both reviewed, and I'm not sure what it referred to originally. There are other things I'm curious about that I don't know much about and that might become stale after RVH reuse: enabled_bindings_, sudden_termination_allowed_, is_waiting_for_close_ack_. Since this kind of reuse is already possible today, I wonder if this is the first time we've noticed this might be problem, or if we've already thought about it? Another scenario I'm trying to think through with similar concerns is A1 with a long unload handler navigating to B, and then going to A2. That might now reuse A1's RV (with its RFH pending deletion), whereas it wouldn't reuse it before this CL. But it seems in this case the RV's main_render_frame_id_ will be cleared once B commits, and then we'll go through the flow above when initializing A2. I've tried this manually and it seems to work. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (left): https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1240: GetSiteInstance()->DecrementActiveFrameCount(); On 2016/04/22 19:11:33, Charlie Reis wrote: > This seems like a real change-- pending delete RFHs will now be considered > active frames. I see how it's necessary for this CL, but I'm wondering what the > implications are. (Maybe it's an improvement over how things were before? It > looks like we might have been able to kill the process before its unload handler > finished, for example.) > > Some questions: > > 1) What happens if this RFH is destroyed (e.g., by closing the tab or detaching > the frame) while it's in the pending delete state? ~RFHI won't call > DecrementActiveFrameCount because is_active() returns false. It almost looks > like we could unconditionally call DecrementActiveFrameCount in the RFHI > destructor and not bother calling in OnSwappedOut, since OnSwappedOut should > lead to deletion. You're right, I've verified that we would miss calling DecrementActiveFrameCount in that case. I've moved the decrement to ~RFHI, which indeed makes more sense. > 2) What were the implications of it being decremented before swap out before? I > see that active_frame_count() was used when deciding whether to create proxies > on SwapOut, so that's moot now (i.e., we always create the proxy). We also call > active_frame_count() in DiscardUnusedFrame to decide whether to create a proxy > after canceling a pending RFH. I'm less clear here-- we will now treat pending > delete RFHs as a reason to create a proxy when canceling a pending RFH. > > To be honest, though, I don't see that as a big problem. Once the pending > delete RFH goes away and the count reaches 0, I assume that extra proxy can be > cleaned up? (I don't see where that happens at first glance.) > > If so, this change seems safe to me. That's a great point. Yes, canceling a pending RFH may lead to another extra proxy here if there's also a pending shutdown RFH around. That extra proxy should also get cleaned up in that case: deleting the pending RFH here will decrement active_frame_count to 1, then when the pending delete RFH goes away, it will decrement active_frame_count to 0, SiteInstanceImpl will realize that it has reached 0 and call ActiveFrameCountIsZero on all RFHMs with proxies, and that should delete all of the proxies, including the one created here. I've manually verified that this is indeed what happens. https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1220: DCHECK(proxy); On 2016/04/22 19:11:33, Charlie Reis wrote: > Given the trouble we've had here, let's make this a CHECK. :) Done. https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1223: int proxy_routing_id = proxy->GetRoutingID(); On 2016/04/22 19:11:33, Charlie Reis wrote: > nit: Move proxy_routing_id and replication_state into the IsRenderFrameLive > block, since that's the only place they're used. Done. https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1392: GetSiteInstance()->DecrementActiveFrameCount(); On 2016/04/22 19:11:33, Charlie Reis wrote: > As noted above, we may just want to do this unconditionally in the destructor. Done. https://codereview.chromium.org/1886413002/diff/180001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1886413002/diff/180001/content/renderer/rende... content/renderer/render_frame_impl.cc:1585: // start using the RenderFrameProxy, if one is created. On 2016/04/22 19:11:34, Charlie Reis wrote: > nit: Drop "if one is created" Done.
Description was changed from ========== Always swap with a replacement proxy in OnSwapOut. Currently, if a frame is the last active frame in the process and it gets swapped out, OnSwapOut is not passed a proxy routing ID, and so it leaves the RenderFrame alone without swapping it or destroying it. (Later on, closing the RenderView detaches all nodes in the frame tree, which cleans up that RenderFrame.) This leads to the complexity of dealing with this special case and has indirectly led to various races involving reuse of the RenderViewHost for a RFH pending shutdown. See https://codereview.chromium.org/1835833002 and issues 515302, 581912, 544755, and 591478 for some context. Additionally, this led to problems of trying to later use the RenderFrame that was left hanging around by OnSwapOut. For instance, when this is done for a subframe, there is a race where the subframe's WebFrameWidget is cleaned up before the RenderView is closed, and the RenderFrame tried to reference the null widget as part of being detached when the RenderView is closed (see https://crbug.com/568836#c15). This CL changes SwapOut to always swap with a proxy. In the cases when the last active frame in the process is going away, the proxy will be short-lived and will be deleted as soon as the RFH pending shutdown is deleted. To facilitate this, the decrementing of active frame count moves from RFH::SwapOut to RFH::OnSwappedOut. This also lets us get rid of the whole concept of RenderViewHosts pending shutdown, making it fine to reuse a RVH when its RFH is pending shutdown and just rely on RVH's refcount to keep it alive and destroy it. The extra short-lived proxy should be well worth significantly reduced code complexity. BUG=568836 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Always swap with a replacement proxy in OnSwapOut. Currently, if a frame is the last active frame in the process and it gets swapped out, OnSwapOut is not passed a proxy routing ID, and so it leaves the RenderFrame alone without swapping it or destroying it. (Later on, closing the RenderView detaches all nodes in the frame tree, which cleans up that RenderFrame.) This leads to the complexity of dealing with this special case and has indirectly led to various races involving reuse of the RenderViewHost for a RFH pending shutdown. See https://codereview.chromium.org/1835833002 and issues 515302, 581912, 544755, and 591478 for some context. Additionally, this led to problems of trying to later use the RenderFrame that was left hanging around by OnSwapOut. For instance, when this is done for a subframe, there is a race where the subframe's WebFrameWidget is cleaned up before the RenderView is closed, and the RenderFrame tried to reference the null widget as part of being detached when the RenderView is closed (see https://crbug.com/568836#c15). This CL changes SwapOut to always swap with a proxy. In the cases when the last active frame in the process is going away, the proxy will be short-lived and will be deleted as soon as the RFH pending shutdown is deleted. To facilitate this, the decrementing of active frame count moves from RFH::SwapOut to RFH::OnSwappedOut. This also lets us get rid of the whole concept of RenderViewHosts pending shutdown, making it fine to reuse a RVH when its RFH is pending shutdown and just rely on RVH's refcount to keep it alive and destroy it. The extra short-lived proxy should be well worth significantly reduced code complexity. BUG=568836,515302,581912,544755,591478 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
I think it makes sense to proceed and file a bug to consider any issues with reusing RVHs, since (as you point out) it's already possible today. The rest of this CL is so much cleaner that it seems worth it to proceed. LGTM. https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... File content/browser/frame_host/frame_tree.cc (right): https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/frame_tree.cc:343: CHECK(iter != render_view_host_map_.end()); On 2016/04/25 18:17:07, alexmos wrote: > On 2016/04/22 19:11:33, Charlie Reis wrote: > > Would CHECK_NE work? > > I had that originally, but that failed to compile in PS9. Acknowledged. https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... File content/browser/frame_host/frame_tree.h (left): https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/frame_tree.h:242: // their state is already gone away). On 2016/04/25 18:17:08, alexmos wrote: > On 2016/04/22 19:11:33, Charlie Reis wrote: > > Sanity check: Is there state that has gone away that we need to be concerned > > about, or is this stale? > > > > For example, what about main_frame_routing_id_? That gets set in the RVH > > constructor, and updated at commit time. If we didn't reuse a pending delete > > RVH and created a new one instead, it would have a main_frame_routing_id_ at > > construction. That impacts RenderViewImpl::Initialize. Now we might have a > > stale main_frame_routing_id_ until commit time, which could do something wrong > > in RenderViewImpl::Initialize? (Not sure, since your tests are green.) > > > > Do we need to update this value sooner? (I ask because it's related to many > of > > the other crashes we've seen in this area.) > > Good question, and the answer is complicated. :) I actually think this might > not be specific to pending deletion RVHs -- if we have an A-embed-B and we > navigate to B, today that will reuse the old RVH (which was previously swapped > out) even without these changes. (I wonder if that's ok and intentional? Maybe > we shouldn't reuse a RV for a new main frame in such cases.) > > For the above scenario, the common case is that we reuse the RVH that is live, > so we skip the call to create it, and thus we never get to RVI::Initialize on > this path. Instead, we go through RFI::CreateFrame (sent for the main frame). > That creates the RF and leaves the RV's main_render_frame_ as null until commit, > at which point RenderFrameImpl::didCommitProvisionalLoad updates it to the > committed frame. This actually makes sense, since the RV's main frame is > technically a proxy until that commit happens, so wouldn't be right to update it > earlier. Following the same reasoning, updating main_frame_routing_id_ on the > browser process side earlier (before commit) might not be right, since the RVH > is still technically swapped out? The alternative to all this is to update > main_frame_routing_id_ in FrameTree::CreateRenderViewHost, in the case where > that decides to reuse an existing one, but that seems like it won't be > consistent with the renderer (I haven't tried this yet, so not sure what other > side effects that might have). Agreed-- updating main_frame_routing_id_ does seem like the right thing here. > If the RVH is not live when we decide to reuse it, that seems like it would get > us into trouble here. But it actually seems to also work: we get into > CreateRenderView with a main_frame_routing_id_ of NONE and proceed to create a > new RV which is swapped out, then we make a separate CreateRenderFrame call to > create the main frame (this is done because the main frame isn't live yet after > creating the view [1]) and then we update the view's main frame at commit time > as before. This whole scenario seems fragile though. > > I'm wondering if Nasko remembers anything about the "part of their state is > already gone away" comment. It first appeared in clamy@'s unload handler CL, > https://codereview.chromium.org/88503002, which you both reviewed, and I'm not > sure what it referred to originally. There are other things I'm curious about > that I don't know much about and that might become stale after RVH reuse: > enabled_bindings_, sudden_termination_allowed_, is_waiting_for_close_ack_. > Since this kind of reuse is already possible today, I wonder if this is the > first time we've noticed this might be problem, or if we've already thought > about it? Yeah, I don't think we've thought closely about it. I'm inclined to say that we should leave that for a separate CL/bug and proceed with this one, since there's already a lot of benefit to this one and the issues we're considering are already possible. > > Another scenario I'm trying to think through with similar concerns is A1 with a > long unload handler navigating to B, and then going to A2. That might now reuse > A1's RV (with its RFH pending deletion), whereas it wouldn't reuse it before > this CL. But it seems in this case the RV's main_render_frame_id_ will be > cleared once B commits, and then we'll go through the flow above when > initializing A2. I've tried this manually and it seems to work. Sounds good. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (left): https://codereview.chromium.org/1886413002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1240: GetSiteInstance()->DecrementActiveFrameCount(); On 2016/04/25 18:17:08, alexmos wrote: > On 2016/04/22 19:11:33, Charlie Reis wrote: > > This seems like a real change-- pending delete RFHs will now be considered > > active frames. I see how it's necessary for this CL, but I'm wondering what > the > > implications are. (Maybe it's an improvement over how things were before? It > > looks like we might have been able to kill the process before its unload > handler > > finished, for example.) > > > > Some questions: > > > > 1) What happens if this RFH is destroyed (e.g., by closing the tab or > detaching > > the frame) while it's in the pending delete state? ~RFHI won't call > > DecrementActiveFrameCount because is_active() returns false. It almost looks > > like we could unconditionally call DecrementActiveFrameCount in the RFHI > > destructor and not bother calling in OnSwappedOut, since OnSwappedOut should > > lead to deletion. > > You're right, I've verified that we would miss calling DecrementActiveFrameCount > in that case. I've moved the decrement to ~RFHI, which indeed makes more sense. > > > 2) What were the implications of it being decremented before swap out before? > I > > see that active_frame_count() was used when deciding whether to create proxies > > on SwapOut, so that's moot now (i.e., we always create the proxy). We also > call > > active_frame_count() in DiscardUnusedFrame to decide whether to create a proxy > > after canceling a pending RFH. I'm less clear here-- we will now treat > pending > > delete RFHs as a reason to create a proxy when canceling a pending RFH. > > > > To be honest, though, I don't see that as a big problem. Once the pending > > delete RFH goes away and the count reaches 0, I assume that extra proxy can be > > cleaned up? (I don't see where that happens at first glance.) > > > > If so, this change seems safe to me. > > That's a great point. Yes, canceling a pending RFH may lead to another extra > proxy here if there's also a pending shutdown RFH around. That extra proxy > should also get cleaned up in that case: deleting the pending RFH here will > decrement active_frame_count to 1, then when the pending delete RFH goes away, > it will decrement active_frame_count to 0, SiteInstanceImpl will realize that it > has reached 0 and call ActiveFrameCountIsZero on all RFHMs with proxies, and > that should delete all of the proxies, including the one created here. I've > manually verified that this is indeed what happens. Acknowledged.
LGTM https://codereview.chromium.org/1886413002/diff/200001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1886413002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1222: set_render_frame_proxy_host(proxy); Ugh, this isn't needed, but I'll get rid of it in a cleanup patch. We don't use the render_frame_proxy_host_ member at this time.
The CQ bit was checked by alexmos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1886413002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1886413002/200001
Thanks for the reviews! I've filed https://crbug.com/606926 to track RVH reuse issues, and I'll go ahead with this CL.
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Always swap with a replacement proxy in OnSwapOut. Currently, if a frame is the last active frame in the process and it gets swapped out, OnSwapOut is not passed a proxy routing ID, and so it leaves the RenderFrame alone without swapping it or destroying it. (Later on, closing the RenderView detaches all nodes in the frame tree, which cleans up that RenderFrame.) This leads to the complexity of dealing with this special case and has indirectly led to various races involving reuse of the RenderViewHost for a RFH pending shutdown. See https://codereview.chromium.org/1835833002 and issues 515302, 581912, 544755, and 591478 for some context. Additionally, this led to problems of trying to later use the RenderFrame that was left hanging around by OnSwapOut. For instance, when this is done for a subframe, there is a race where the subframe's WebFrameWidget is cleaned up before the RenderView is closed, and the RenderFrame tried to reference the null widget as part of being detached when the RenderView is closed (see https://crbug.com/568836#c15). This CL changes SwapOut to always swap with a proxy. In the cases when the last active frame in the process is going away, the proxy will be short-lived and will be deleted as soon as the RFH pending shutdown is deleted. To facilitate this, the decrementing of active frame count moves from RFH::SwapOut to RFH::OnSwappedOut. This also lets us get rid of the whole concept of RenderViewHosts pending shutdown, making it fine to reuse a RVH when its RFH is pending shutdown and just rely on RVH's refcount to keep it alive and destroy it. The extra short-lived proxy should be well worth significantly reduced code complexity. BUG=568836,515302,581912,544755,591478 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Always swap with a replacement proxy in OnSwapOut. Currently, if a frame is the last active frame in the process and it gets swapped out, OnSwapOut is not passed a proxy routing ID, and so it leaves the RenderFrame alone without swapping it or destroying it. (Later on, closing the RenderView detaches all nodes in the frame tree, which cleans up that RenderFrame.) This leads to the complexity of dealing with this special case and has indirectly led to various races involving reuse of the RenderViewHost for a RFH pending shutdown. See https://codereview.chromium.org/1835833002 and issues 515302, 581912, 544755, and 591478 for some context. Additionally, this led to problems of trying to later use the RenderFrame that was left hanging around by OnSwapOut. For instance, when this is done for a subframe, there is a race where the subframe's WebFrameWidget is cleaned up before the RenderView is closed, and the RenderFrame tried to reference the null widget as part of being detached when the RenderView is closed (see https://crbug.com/568836#c15). This CL changes SwapOut to always swap with a proxy. In the cases when the last active frame in the process is going away, the proxy will be short-lived and will be deleted as soon as the RFH pending shutdown is deleted. To facilitate this, the decrementing of active frame count moves from RFH::SwapOut to RFH::OnSwappedOut. This also lets us get rid of the whole concept of RenderViewHosts pending shutdown, making it fine to reuse a RVH when its RFH is pending shutdown and just rely on RVH's refcount to keep it alive and destroy it. The extra short-lived proxy should be well worth significantly reduced code complexity. BUG=568836,515302,581912,544755,591478 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/9aa6123974ab0f85aecf20cfc6e377071f99b386 Cr-Commit-Position: refs/heads/master@{#389904} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/9aa6123974ab0f85aecf20cfc6e377071f99b386 Cr-Commit-Position: refs/heads/master@{#389904} |
