Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2415973002:
Do not hide the old RenderWidgetHostView when commiting a navigation. (Closed)
|
Created:
4 years, 2 months ago by lfg Modified:
4 years, 2 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not hide the old RenderWidgetHostView when commiting a navigation.
This fixes an issue where the old RenderView is reused by a new remote
subframe. We shouldn't be leaking resources, because now we are already
hiding the unused RenderView in the renderer, when the local main frame
gets detached and the WebViewFrameWidget is closed.
BUG=638375
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/7c3d7bbd510eda2bcfb971f6d166da26bcfe4222
Cr-Commit-Position: refs/heads/master@{#426913}
Patch Set 1 #Patch Set 2 : add test #
Messages
Total messages: 25 (14 generated)
Description was changed from ========== Do not hide the old RenderView when commiting a navigation. This fixes an issue where the old RenderView is reused by a new remote subframe. We shouldn't be leaking resources, because now we are already hiding the unused RenderView in the renderer, when the local main frame gets detached and the WebViewFrameWidget is closed. BUG=638375 ========== to ========== Do not hide the old RenderView when commiting a navigation. This fixes an issue where the old RenderView is reused by a new remote subframe. We shouldn't be leaking resources, because now we are already hiding the unused RenderView in the renderer, when the local main frame gets detached and the WebViewFrameWidget is closed. BUG=638375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lfg@chromium.org changed reviewers: + creis@chromium.org
Charlie, please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yeah, I could see that causing the bug. Seems like we're using the wrong criteria to hide it, now that RenderViewHosts can be "visible" when the main frame is remote. Thanks for tracking it down! What about cases where there really are no active frames in the RenderViewHost (e.g., window.open to site B with no iframes), though? The intent of the Hide was to prevent doing extra paint work in that case. Maybe somewhere else we should be telling RenderWidgetHostView to hide if it has no active frames? Or do we already delete the RWHV in that case? (I vaguely seem to remember that we don't create RWHVs if a RVH is created for a proxy, at least until they're needed.) Maybe this isn't an issue? nit: Please update CL subject and description, since it looks like we were hiding the RWHV, not the RV.
Description was changed from ========== Do not hide the old RenderView when commiting a navigation. This fixes an issue where the old RenderView is reused by a new remote subframe. We shouldn't be leaking resources, because now we are already hiding the unused RenderView in the renderer, when the local main frame gets detached and the WebViewFrameWidget is closed. BUG=638375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Do not hide the old RenderWidgetHostView when commiting a navigation. This fixes an issue where the old RenderView is reused by a new remote subframe. We shouldn't be leaking resources, because now we are already hiding the unused RenderView in the renderer, when the local main frame gets detached and the WebViewFrameWidget is closed. BUG=638375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
On 2016/10/14 18:17:46, Charlie Reis (Away till 10-14) wrote: > Yeah, I could see that causing the bug. Seems like we're using the wrong > criteria to hide it, now that RenderViewHosts can be "visible" when the main > frame is remote. Thanks for tracking it down! > > What about cases where there really are no active frames in the RenderViewHost > (e.g., window.open to site B with no iframes), though? The intent of the Hide > was to prevent doing extra paint work in that case. This isn't an issue anymore, because when the main frame gets detached (in RenderFrameImpl::frameDetached) we call RenderViewImpl::CloseForFrame (when called on the main frame), which calls WebViewFrameWidget::Close, which calls WebViewImpl::setCompositorVisibility(false). So we avoid the extra paint work already. > Maybe somewhere else we should be telling RenderWidgetHostView to hide if it has > no active frames? Or do we already delete the RWHV in that case? (I vaguely > seem to remember that we don't create RWHVs if a RVH is created for a proxy, at > least until they're needed.) Maybe this isn't an issue? Right, this is now already done in the renderer, when a local main frame is detached. > nit: Please update CL subject and description, since it looks like we were > hiding the RWHV, not the RV. Done.
On 2016/10/14 20:32:03, lfg wrote: > > Maybe somewhere else we should be telling RenderWidgetHostView to hide if it > has > > no active frames? Or do we already delete the RWHV in that case? (I vaguely > > seem to remember that we don't create RWHVs if a RVH is created for a proxy, > at > > least until they're needed.) Maybe this isn't an issue? > > Right, this is now already done in the renderer, when a local main frame is > detached. Oh, one more thing, I don't think we delete the RWHV, it hangs around in the browser process. We could look into cleaning that up, but when I look at some of those things, it's not very easy to do a proper cleanup as those classes are not meant to be reused that way. It would be a lot easier if we could just delete the RenderWidgetHost instead, but I'm not sure how far we would have to be in the view/widget split to achieve that.
Thanks, LGTM. Is there a way to add a IsShowing() check to a (possible existing) test to prevent regression here?
On 2016/10/14 20:44:11, Charlie Reis (OOO until 10-19) wrote: > Thanks, LGTM. > > Is there a way to add a IsShowing() check to a (possible existing) test to > prevent regression here? Sorry for the delay. Done, please take another look. This test becomes obsolete once RenderView/RenderWidget is split, but it's worth having since that's not in the near future.
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/19 21:03:16, lfg wrote: > On 2016/10/14 20:44:11, Charlie Reis (OOO until 10-19) wrote: > > Thanks, LGTM. > > > > Is there a way to add a IsShowing() check to a (possible existing) test to > > prevent regression here? > > Sorry for the delay. Done, please take another look. > > This test becomes obsolete once RenderView/RenderWidget is split, but it's worth > having since that's not in the near future. Thanks, and sorry for the delay! LGTM.
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Do not hide the old RenderWidgetHostView when commiting a navigation. This fixes an issue where the old RenderView is reused by a new remote subframe. We shouldn't be leaking resources, because now we are already hiding the unused RenderView in the renderer, when the local main frame gets detached and the WebViewFrameWidget is closed. BUG=638375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Do not hide the old RenderWidgetHostView when commiting a navigation. This fixes an issue where the old RenderView is reused by a new remote subframe. We shouldn't be leaking resources, because now we are already hiding the unused RenderView in the renderer, when the local main frame gets detached and the WebViewFrameWidget is closed. BUG=638375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Do not hide the old RenderWidgetHostView when commiting a navigation. This fixes an issue where the old RenderView is reused by a new remote subframe. We shouldn't be leaking resources, because now we are already hiding the unused RenderView in the renderer, when the local main frame gets detached and the WebViewFrameWidget is closed. BUG=638375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Do not hide the old RenderWidgetHostView when commiting a navigation. This fixes an issue where the old RenderView is reused by a new remote subframe. We shouldn't be leaking resources, because now we are already hiding the unused RenderView in the renderer, when the local main frame gets detached and the WebViewFrameWidget is closed. BUG=638375 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/7c3d7bbd510eda2bcfb971f6d166da26bcfe4222 Cr-Commit-Position: refs/heads/master@{#426913} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7c3d7bbd510eda2bcfb971f6d166da26bcfe4222 Cr-Commit-Position: refs/heads/master@{#426913}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2448503002/ by lfg@chromium.org. The reason for reverting is: Caused a regression on Mac. BUG=658688. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
