Thanks for the report. This is a good point, although we know a few places where we have efficiency problems in subframe renderers. Now that OOPIFs have information on viewport intersection, we should pass that to the compositor and prevent something like this.

What level of degradation are you seeing? For your test page it appears to me that --site-per-process doubles the amount of GPU process memory used, from ~80MB to ~160MB (which is significant).

Try to change iframe height: to 100000px;
I have 60MB vs 700MB (!) and tile tearing problem ( but with another site in iframe with huge news feed )

Data flow:
RenderWidgetHostViewChildFrame::GetViewBounds -> RenderWidgetHostViewBase::GetRequestedRendererSize -> RenderWidgetHostImpl::GetResizeParams -> RenderWidget::Resize -> RenderWidgetCompositor::setViewportSize -> LayerTreeHostImpl::SetViewportSize -> PictureLayerTiling::CreateTile ( massive )

Hm, I try to pass to compositor correct viewport (viewport intersection in RenderWidget), but I get cropping image.
Any ideas?

Deleted: bug.png 697 KB

	bug.png 697 KB View Download

#5: Can you be more specific about what you are doing? Where are you obtaining the value that you are passing to the compositor?

It might be related, but even without --site-per-process the GPU process memory usage over the past week or so on the dev channel has been super high.

I don't run with that flag but seeing the GPU process at 1-2GB ram usage with up to 3GB committed has not been uncommon recently. These figures are up about 10 fold on the figures it was running at 2 weeks ago. Definitely something going on with it.

#7: I think you have another problem. Can you create tracing with memory infra ( https://storage.googleapis.com/chromium-docs.appspot.com/1c6d1886584e7cc6ffed0d377f32023f8da53e02 ) and fill new bug?

#6:
OOPIF doesn't know global viewport.
It rasters all content. And it takes memory and cpu.

Even we pass viewport to OOPIF compositor ( from viewport intersection via RenderWidgetCompositor::setViewportSize ) we have a lot of problems.
1) Viewport is gfx:Size, not Rect. So when we scroll main renderer, OOPIF scrolls as a whole and viewport scrolls too. See bug.png in #5.
2) It is too slow to pass viewport every main renderer scroll.

I have found https://chromiumcodereview.appspot.com/15579002 and have tried to set external_viewport_ and external_transform from viewport intersection.
It works but too slow.

Moreover, if OOPIF is not visible due to browser viewport, it rasters too.

Seems like we have a problem in OOPIF.
Do you have any thouhgts and plans how to fix ix?

We get an actual viewport intersection rect via RenderWidget::OnSetViewportIntersection(), which is used for the IntersectionObserver JavaScript API. That should be all we need to constrain the compositor's tile allocations.

This isn't at the top of our priority list for now, though. It likely will be necessary to fix this before we ship --site-per-process, but isn't critical for more immediate milestones.

+Viz folks, I think draw occlusion might help with performance issues but we are doing too much raster...if necessary a combo of surface sync and raster occlusion might help here too but I'm not sure if it's worth the investment of time in the short term before Salamander.

The idea being we signal a new viewport to a child, allocate a new LocalSurfaceId and synchronize on that ID which is used as a signal for raster occlusion in the child frame.

I agree with fsamuel@ that salamander is the right fix for this and other related issues. Is there need a shorter term remediation?

What is the expected timeline on Salamander?

We don't have numbers on the full impact of this problem (i.e. how often OOPIFs are larger than the viewport area of the window), but we are sensitive to memory usage trade-offs for when we ship Site Isolation. If we are commonly bloating the GPU process, then it would affect how aggressive we can make our isolation policy.

I think piping the intersected viewport is the right thing to do. It goes beyond just allocating GPU memory, we also schedule way too many raster tasks, and those would block activation (since those tiles are considered visible), etc.

Salamander may fix it, but I think it is still very far away - doubly so on non-aura platforms.

The intersected viewport is already available in the OOPIF process (used for IntersectionObserver), but I am clear on how easy it is to use that in the compositor to limit how much gets painted. In this case the visible area is smaller than the iframe's 'viewport' (which the Document is rendering into), and AFAICT this would be a new situation for the compositor to have to handle.

The embedded compositor doesn't really care, it will make CompositorFrames that fit the viewport it's given.
The embedder will need to size its SurfaceLayer/SurfaceInfos appropriately.

I should note, there is some flexibility there, it might make sense to expand the viewport beyond the strict intersection, to allow scrolling without jank/guttering.

Raising the priority on this because it apparently can cause some significant usability issues on the web when a very large iframe is embedded, as we see in issue 807088.

kenrb@ mentioned that he has some work in progress on this.

Adding RB-Beta since one of the goals is to turn on Site Isolation for M66 Beta. 

Brief summary of a meeting yesterday on this bug with fsamuel, samans, kylechar and weiliangc (WIP CL is https://chromium-review.googlesource.com/c/chromium/src/+/932702/):

Fady is concerned that my existing approach could cause problems with surface synchronization because it affects the size of the compositor frames generated by the OOPIF: we're not certain this is true, because I think as written the viewport size only gets changed in response to a Resize message from the browser, it's just that I have added additional constraints on the size at that point). Still, Fady has been working through a lot of crashes related to size changes and thinks there might be corner cases.

Weiliang proposed that instead of changing the viewport size, we plumb the viewport intersection into the visible_rect draw property of the OOPIF's root layer, and that should limit raster. The advantage is that it avoids any questions of negative interaction with surface synchronization, the downside AFAICT is just that it might be a fair bit more work.

There was also a lot of discussion about longer term solutions and what happens with this problem for OOP-R. Ultimately we want to move everything off of the main thread (a large current limitation, for both this and intersection observer on which the solution to this is being built), integrate with surface synchronization (which we might get for free?), and have OOPIF raster during mirror the existing logic for how the compositor currently rasters during scroll to try to prevent unpainted areas from being visible. That's out of scope for the current bug, though, since we need a shorter term solution.

Friendly ping to get an update on this issue as it is marked as release block beta for M66 (As Beta is coming to M66 in ~weeks).

Thanks..!

I am actively working on this.

M66 Beta promotion is coming VERY soon. Your bug is labelled as Beta  ReleaseBlock, pls make sure to land the fix and request a merge into the release branch latest by 1:00 PM PT Friday, 03/12.  Thank you.

I don't think this should be blocking the release of M66 beta, as it is Site Isolation specific, therefore not enabled by default. We can assess whether to merge in M66 once the fix has landed and had some bake time. Removing the beta blocker label for now.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b2c9e2623c8b6a1abdd28acb9e682988c826938c

commit b2c9e2623c8b6a1abdd28acb9e682988c826938c
Author: Ken Buchanan <kenrb@chromium.org>
Date: Sat Mar 10 16:53:31 2018

Limit raster area for OOPIFs

For the purposes of Blink layout, the viewport of an iframe is the
iframe element rect. This is a problem for compositing when an iframe
is very large because it means the OOPIF's compositor will raster a
very large number of pixels.

This CL plumbs a smaller rect based on the current OOPIF's viewport
intersection rect, plus a margin to allow for delay when the parent
frame is scrolling, to be used as a clip for the OOPIF's root layer.

Bug:  690605 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I83e67f0b541fdac54285d89e6de85b040d3fbe3e
Reviewed-on: https://chromium-review.googlesource.com/932702
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Ken Buchanan <kenrb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542376}
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/cc/trees/draw_property_utils.cc
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/cc/trees/layer_tree_host.h
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/cc/trees/layer_tree_impl.h
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/content/browser/frame_host/cross_process_frame_connector.cc
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/content/browser/frame_host/cross_process_frame_connector.h
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/content/browser/renderer_host/frame_connector_delegate.h
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/content/browser/renderer_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/content/browser/renderer_host/render_widget_host_view_child_frame.h
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/content/browser/renderer_host/render_widget_host_view_child_frame_unittest.cc
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/content/common/frame_messages.h
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/content/common/view_messages.h
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/content/renderer/gpu/render_widget_compositor.cc
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/content/renderer/gpu/render_widget_compositor.h
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/content/renderer/render_frame_proxy.h
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/content/renderer/render_widget.cc
[modify] https://crrev.com/b2c9e2623c8b6a1abdd28acb9e682988c826938c/content/renderer/render_widget.h

There appears to be an issue with b2c9e2623c8b when CSS transforms are involved. I noticed while investigating  crbug.com/817392  that after scrolling the div in "iframe-scroll-repro.html", we appear to have unrastered content.

In this example, we have a tall OOPIF in a scrollable div, and the div is inside another div which has CSS transforms. Without these transforms (see "iframe-scroll-no-repro.html"), we don't have this issue.

I've tried locally reverting b2c9e2623c8b and the issue goes away.

Deleted: oopif-blank-content.png 41.0 KB

	oopif-blank-content.png 41.0 KB View Download

 Issue 823433  is now tracking the problem from comment 32.