New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 802809 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Number of missing surfaces increased 1000x with Site Isolation

Project Member Reported by fsam...@chromium.org, Jan 16 2018

Issue description

https://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.
 
Ahh I might have a repro:

If you kill the GPU process while an extension popup is open, then you get tons of missing surfaces.
Components: Internals>Sandbox>SiteIsolation
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.
I've confirmed that we get missing surfaces if we kill an OOPIF and then change the device scale factor.
I have a fix in progress.

Comment 6 by creis@chromium.org, Jan 30 2018

Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
Status: Started (was: Assigned)
Glad to hear!  Any updates on the fix?  And does it look like the bug only affects the crashed/killed OOPIF case?
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! 
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Labels: Merge-Request-65
Project Member

Comment 10 by sheriffbot@chromium.org, Feb 8 2018

Labels: -Merge-Request-65 Hotlist-Merge-Approved Merge-Approved-65
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
Pls merge your change to M65 branch 3325 ASAP so we can pick it up for next M65 Beta release. Thank you.
Labels: -Hotlist-Merge-Approved -Merge-Approved-65
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.
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).
Status: Fixed (was: Started)
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