OOPIF: iframes loaded from web_accessible_resources cause continual repaints
Reported by
con...@superhuman.com,
Aug 15 2017
|
|||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.101 Safari/537.36 Example URL: https://github.com/superhuman/repaint-extension Steps to reproduce the problem: 1. Install the demo extension, and open index.html 2. 3. What is the expected behavior? It should not use CPU What went wrong? Every frame the top page continually repaints, causing wasted CPU time. Does it occur on multiple sites: Yes Is it a problem with a plugin? No Did this work before? Yes 59 Does this work in other browsers? Yes Chrome version: 60.0.3112.101 Channel: stable OS Version: OS X 10.12.6 Flash Version:
,
Aug 15 2017
Ken, can you take a look to see if this is an OOPIF frame-throttling issue? It's a web page with an extension OOPIF, which eats through more CPU than when loading the same iframe in the parent process.
,
Aug 15 2017
Hmm, I'm not able to repro the bug on Mac Chrome 60.0.3112.101 (Stable). The subframe process stays at 0% for me in Chrome's task manager. I'm also not able to repro the bug on Windows 10 in either 61.0.3163.39 (Beta) or 62.0.3186.0 (Canary). Can you post your list of Variations from chrome://version in case a particular field trial might be involved? It may also help to attach a trace from chrome://tracing, but I'm not sure which categories to recommend recording.
,
Aug 15 2017
Thanks creis. In my experiments it is the parent frame that is using CPU, and you can see in the "Performance" developer tools that this is caused by continual repainting (see attached). This has reproduced on all three laptops we tested variations lists below. If it'd be helpful (and you're in the SF office) happy to bring a laptop over and show you. 98ee9f3e-98ee9f3e 241fff6c-4eda1c57 c68ab9a3-2dd48d0 3095aa95-3f4a17df 6c43306f-ca7d8d80 7c1bc906-f55a7974 47e5d3db-3d47f4f4 d43bf3e5-ad6ab04f ba3f87da-b4a760c3 5ca89f9-3f4a17df f3499283-7711d854 9e201a2b-1359b0af 5b3ed0a1-3f4a17df 68812885-ca7d8d80 332a4d9b-ca7d8d80 9bd94ed7-b1c9f6b0 b791c1b8-ca7d8d80 9773d3bd-ca7d8d80 2e109477-f3b42e62 99144bc3-3cc2175e 9e5c75f1-a59b312f f79cb77b-3d47f4f4 b7786474-d93a0620 27219e67-b2047178 23a898eb-e0e2610f e856c60b-ca7d8d80 4ea303a6-85fb2903 64224f74-5087fa4a 56302f8c-2f882e70 de03e059-e65e20f2 f56e0452-ca7d8d80 b2f0086-93053e47 494d8760-803f8fc4 f47ae82a-86f22ee5 3ac60855-486e2a9c f296190c-9109843f 4442aae2-6e597ede ed1d377-e1cc0f14 75f0f0a0-e1cc0f14 e2b18481-6754d7b7 e7e71889-4ad60575 828a5926-ca7d8d80 a88c475d-3d47f4f4 ----- 98ee9f3e-98ee9f3e ea8deb27-3f4a17df 241fff6c-4eda1c57 c68ab9a3-ca7d8d80 3095aa95-3f4a17df 6c43306f-ca7d8d80 7c1bc906-f55a7974 47e5d3db-3d47f4f4 d43bf3e5-ad6ab04f ba3f87da-b4a760c3 5ca89f9-f23d1dea f3499283-7711d854 9e201a2b-1359b0af 68812885-ca7d8d80 332a4d9b-3f4a17df 9bd94ed7-b1c9f6b0 b791c1b8-ca7d8d80 9773d3bd-ca7d8d80 2e109477-f3b42e62 99144bc3-3cc2175e 165e16d1-3f4a17df 9e5c75f1-40093a1 f79cb77b-3d47f4f4 b7786474-d93a0620 9591f600-d93a0620 be3b5415-ca7d8d80 27219e67-b2047178 23a898eb-e0e2610f d39326b0-d93a0620 e856c60b-ca7d8d80 4ea303a6-ecbb250e 64224f74-5087fa4a 56302f8c-2f882e70 de03e059-e65e20f2 f56e0452-ca7d8d80 b2f0086-93053e47 494d8760-3d47f4f4 f47ae82a-86f22ee5 3ac60855-486e2a9c f296190c-a5822863 4442aae2-e1cc0f14 ed1d377-e1cc0f14 75f0f0a0-d7f6b13c e2b18481-bd104136 e7e71889-e1cc0f14 828a5926-ca7d8d80 a88c475d-3d47f4f4 ----- 98ee9f3e-98ee9f3e ea8deb27-3f4a17df 241fff6c-4eda1c57 c68ab9a3-ca7d8d80 3095aa95-3f4a17df 6c43306f-ca7d8d80 7c1bc906-f55a7974 47e5d3db-3d47f4f4 d43bf3e5-ad6ab04f ba3f87da-45bda656 5ca89f9-ca7d8d80 f3499283-7711d854 9e201a2b-1359b0af 5b3ed0a1-3f4a17df 68812885-ca7d8d80 332a4d9b-3f4a17df b791c1b8-ca7d8d80 9773d3bd-ca7d8d80 2e109477-f3b42e62 99144bc3-3cc2175e 9e5c75f1-2ad3bd2f f79cb77b-3d47f4f4 b7786474-d93a0620 be3b5415-bed9585 27219e67-b2047178 23a898eb-e0e2610f d39326b0-d93a0620 e856c60b-ca7d8d80 4ea303a6-ecbb250e 64224f74-5087fa4a 56302f8c-2f882e70 de03e059-e65e20f2 f56e0452-ca7d8d80 b2f0086-93053e47 494d8760-803f8fc4 f47ae82a-86f22ee5 3ac60855-486e2a9c f296190c-24dd8f80 4442aae2-6bdfffe7 ed1d377-e1cc0f14 75f0f0a0-6bdfffe7 e2b18481-bca011b3 e7e71889-e1cc0f14 828a5926-ca7d8d80 a88c475d-3d47f4f4
,
Aug 16 2017
Ah, confirmed! Thanks for the tip for taking a 1 second trace in the DevTools Performance panel. I can now see the "Composite Layers" and "Update Layer Tree" events happening continuously on both Mac M60 and Windows M61/M62. It's not registering in the Windows task manager for me on my box, but I do see it in the parent process on Mac. We should definitely fix the extra compositing events. Ken, can you take a look?
,
Aug 17 2017
+piman, do you have any thoughts on the problem below? I have an idea what is happening here, it relates to lfg's change last year to allow the hit testing code to skip frames with pointer-events:none. https://codereview.chromium.org/1489913003 It isn't painting repeatedly, but that CL did create a perpetual loop of BeginMainFrames that do no work. In particular, when the compositor calls ProxyMain::BeginMainFrame it notifies its client (LayerTreeHost::WillBeginMainFrame), and as a result of the CL mentioned above, RenderFrameProxy::WillBeginCompositorFrame() calls QueueMessage to deliver hit test data. QueueMessage calls LayerTreeHost::SetNeedsAnimate() which sets up another BeginMainFrame in the compositor. ProxyMain::BeginMainFrame always returns early, because there are no updates, but after it notifies the impl thread of the BeginMainFrame cancellation, the Scheduler immediately posts another BeginMainFrame task to the main thread. Any ideas on working around this? Is it just inherently problematic to try to queue a SwapPromise in response to a BeginMainFrame, or is there a reasonable way to modify RenderWidget::QueueMessage to avoid this scenario? (e.g. keep a flag to indicate when we have gotten a WillBeginMainFrame without a corresponding (Did)BeginMainFrame, and send the message directly in that case)
,
Aug 17 2017
I need to look into the details, but I would think that it would be reasonable to make it so that if SetNeedsAnimate is called from WillBeginMainFrame, then it should not cause a new commit to happen - instead it would use the upcoming commit. That would mean switching the order between where max_requested_pipeline_stage_ = NO_PIPELINE_STAGE and layer_tree_host_->WillBeginMainFrame(); happen in ProxyMain::BeginMainFrame. But I don't know what that would break. It does seem like the right thing to do though. The alternative is to add a different MessageDeliveryPolicy for QueueMessage that wouldn't implicitly force a new commit - just use the existing upcoming one - but otherwise have the same semantics as MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE (which does include sending the message via IPC if the commit is cancelled).
,
Aug 28 2017
Ken pointed out that this bug affects all pages with OOPIFs, so I'll raise the priority. We should get a fix for this into M62.
,
Aug 28 2017
piman, enne: I investigated those suggestions (comment 7), and also looked at a bug where people ran into a similar problem with GetTextInputState ( issue 704763 ). Some of esprehn@'s comments on that bug, along with the fact that the max_requested_pipeline_state_ is explicitly verified during WillBeginMainFrame in cc_unittests, makes the first suggestion in comment#7 look undesirable. (See LayerTreeHostProxyTestSetNeedsUpdateLayersWhileAnimating.) The second alternative is more like what I was thinking above, but would it make more sense just to prevent SetNeedsBeginFrame calls when one is already in progress? That seems like a more reliable way to prevent this from happening in future, since QueueMessage callers in future might not know when they need to use an alternative message delivery policy. A simple CL with this approach is posted here: https://chromium-review.googlesource.com/c/chromium/src/+/639550
,
Aug 28 2017
That CL and approach seems reasonable to me. For what it's worth, there are a number of "SetNeedsFoo" places in cc where we've had to differentiate between "queue another one if in the middle of one" or "ignore this one if in the middle of an existing one" (e.g. SetNeedsOneBeginImplFrame vs SetNeedsRedraw). If we need to add more interfaces for clarity we can do it, but if we can avoid it then that seems better too. This bit of code (especially all the comments and awkwardness in the ProxyMain::SetNeedsFoo functions) has a lot of sharp corners and historical baggage.
,
Aug 28 2017
I think we need to be clear about the contract here. This CL sounds like it's encoding an implementation detail of ProxyMain into RenderWidget, and that seems fragile. Maybe we can bomb it with tests, but RenderWidget is not super testable. Maybe it would be better to have QueueSwapPromise deal with that logic (since it lives closer to the ProxyMain, and can likely be more testable). Though at a high level, I'm also not super sure why we have 3 embedder callbacks (WillBeginMainFrame, BeginMainFrame, UpdateLayerTreeHost) that all correspond to the same pipeline stage (ANIMATE_PIPELINE_STAGE) - and it's not clear that SetNeedsBeginFrame called from WillBeginMainFrame should cause a new one. I don't think the semantics for all these are super clear.
,
Aug 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ce710ebb7d3249db861a4e7e5f3835e8cdd7d40a commit ce710ebb7d3249db861a4e7e5f3835e8cdd7d40a Author: Ken Buchanan <kenrb@chromium.org> Date: Wed Aug 30 23:46:52 2017 Prevent unneeded commit requests when queueing swap promises When a swap promise is queued as a consequence of RenderWidget::WillBeginCompositorFrame, another commit is requested even though the current one has not yet begun. This is unnecessary because current commit will fulfill (or cancel) any swap promises, and further it potentially creates a cycle of endless commit requests even when there is no work to be done. This CL moves the commit request from RenderWidget::QueueMessage to LayerTreeHost::QueueSwapPromise, and declines to request a new commit if a main frame is already in progress. Bug: 755730 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: If35b84d124514892b7d2d6c107158ffc620b8755 Reviewed-on: https://chromium-review.googlesource.com/639550 Commit-Queue: Ken Buchanan <kenrb@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Cr-Commit-Position: refs/heads/master@{#498671} [modify] https://crrev.com/ce710ebb7d3249db861a4e7e5f3835e8cdd7d40a/cc/trees/layer_tree_host.cc [modify] https://crrev.com/ce710ebb7d3249db861a4e7e5f3835e8cdd7d40a/cc/trees/layer_tree_host_unittest.cc [modify] https://crrev.com/ce710ebb7d3249db861a4e7e5f3835e8cdd7d40a/content/renderer/render_widget.cc
,
Aug 31 2017
,
Aug 31 2017
Thanks! Verified on Win Chrome Canary 62.0.3201.0. And thanks for the report, Conrad! |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by dcheng@chromium.org
, Aug 15 2017