Number of missing surfaces increased 1000x with Site Isolation |
|||||||
Issue descriptionhttps://uma.googleplex.com/p/chrome/variations/?sid=c0542d649faa8fe4147515269b825d9f The number of missing surfaces increased by 1000x but the number of valid surfaces increased from 2 to 2.6x. This suggests there's a bug here. I'm not sure what the bug is yet, though.
,
Jan 18 2018
,
Jan 22 2018
The number of missing surfaces has dropped by an order of magnitude. There's probably still a bug there somewhere but it's less serious. I think it might have to do with when an OOPIF crashes and then a device scale factor changes.
,
Jan 23 2018
I've confirmed that we get missing surfaces if we kill an OOPIF and then change the device scale factor.
,
Jan 23 2018
I have a fix in progress.
,
Jan 30 2018
Glad to hear! Any updates on the fix? And does it look like the bug only affects the crashed/killed OOPIF case?
,
Jan 30 2018
Yea, I think this bug only affects crashed/killed OOPIFs. At least that's the only way I've been able to get missing surfaces reported so far. This is the fix: https://chromium-review.googlesource.com/c/chromium/src/+/881831 I'll fix some things and send it out for review tomorrow. Sorry for the delay! Triaging/fixing a bunch of surface sync and site isolation issues at the moment!
,
Feb 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4e717f58729bab4ce08b32688fe8a60fb2048852 commit 4e717f58729bab4ce08b32688fe8a60fb2048852 Author: Fady Samuel <fsamuel@chromium.org> Date: Wed Feb 07 05:51:50 2018 Surface synchronization: Fix missing surfaces Prior to this CL, when an OOPIF crashed, we kept the fallback surface around. This caused a massive stream of missing surfaces to be reported when the primary surface was set on ScreenInfo change or FrameRects change. This CL addresses the problem in multiple ways: 1. We clear the fallback_surface_id_ in ChildFrameCompositingHelper when we report ChildFrameGone. This ensures that ChildFrameCompositingHelper does not re-embed the fallback surface that is now gone. 2. We track the crashed state of a child frame, and prevent updating the primary surface ID in ChildFrameCompositingHelper. 3. Instead (this is a tweak), we position the sad page icon as a function frame size and device scale factor. Note, neither RenderFrameProxy nor ChildFrameCompositingHelper have unit tests at the moment so no unit test is included yet. I'm going to try to write a unit test for this change. Change-Id: I1f25ed5e446612a7015c2717e6d2005241241942 Bug: 802809 , 672962 Reviewed-on: https://chromium-review.googlesource.com/881831 Reviewed-by: Antoine Labour <piman@chromium.org> Reviewed-by: Saman Sami <samans@chromium.org> Commit-Queue: Fady Samuel <fsamuel@chromium.org> Cr-Commit-Position: refs/heads/master@{#534925} [modify] https://crrev.com/4e717f58729bab4ce08b32688fe8a60fb2048852/content/renderer/BUILD.gn [modify] https://crrev.com/4e717f58729bab4ce08b32688fe8a60fb2048852/content/renderer/browser_plugin/browser_plugin.cc [modify] https://crrev.com/4e717f58729bab4ce08b32688fe8a60fb2048852/content/renderer/browser_plugin/browser_plugin.h [modify] https://crrev.com/4e717f58729bab4ce08b32688fe8a60fb2048852/content/renderer/child_frame_compositing_helper.cc [modify] https://crrev.com/4e717f58729bab4ce08b32688fe8a60fb2048852/content/renderer/child_frame_compositing_helper.h [add] https://crrev.com/4e717f58729bab4ce08b32688fe8a60fb2048852/content/renderer/child_frame_compositing_helper_unittest.cc [add] https://crrev.com/4e717f58729bab4ce08b32688fe8a60fb2048852/content/renderer/child_frame_compositor.h [modify] https://crrev.com/4e717f58729bab4ce08b32688fe8a60fb2048852/content/renderer/render_frame_proxy.cc [modify] https://crrev.com/4e717f58729bab4ce08b32688fe8a60fb2048852/content/renderer/render_frame_proxy.h [modify] https://crrev.com/4e717f58729bab4ce08b32688fe8a60fb2048852/content/test/BUILD.gn
,
Feb 7 2018
,
Feb 8 2018
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 8 2018
Pls merge your change to M65 branch 3325 ASAP so we can pick it up for next M65 Beta release. Thank you.
,
Feb 9 2018
This patch doesn't apply cleanly and there are too many conflicts for me to be confident about this more. Since this problem only affects the OOPIF crash case, and the missing surfaces metric, I'll skip the merge for M65.
,
Feb 9 2018
I will say that looking at the latest UMA numbers, it seems like missing surfaces are now only up 9-10x versus the control group. This is inline with my expectations. I basically consider this bug fixed in canary. I will follow up with the site isolation team first as to whether they want this fix merged into M65 and the procedure to go about writing an M65 specific fix (since this fix doesn't merge cleanly at all).
,
Feb 9 2018
Site isolation folks feel we can punt on this fix until M66. I'm going to mark this as Fixed. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by fsam...@chromium.org
, Jan 16 2018