RemoteFrameView should only propagate viewport intersection when lifecycle is kPaintClean |
|||||||
Issue descriptionRemoteFrameView::UpdateViewportIntersectionsForSubtree does the job of propagating the visible rect of an OOPIF in the embedding frame down to the iframe's process, for use by IntersectionObserver. In the current code, that method gets called on every lifecycle update, including forced layouts (target_state = kLayoutClean). It should only propagate its visible rect when we're actually generating a frame (kPaintClean).
,
May 14 2018
,
May 14 2018
szager@: Thanks for the fix! Does this need to be merged to M67 for the Site Isolation launch for functional reasons, or is it just an optimization that can wait for M68?
,
May 14 2018
This has the potential to cause IntersectionObserver to behave weirdly (incorrectly) in ways that would be hard for a developer to diagnose. That said, I haven't seen any bugs filed, so I don't know how likely it is to cause real problems. It is a very safe fix, so I would feel pretty good about merging it.
,
May 14 2018
Ah, that sounds more important than I thought! Given the potential for incorrect behavior and the safety of the CL, I think we should request merge to M67. If we get it in tomorrow morning, it could be part of this week's beta release to have time to bake. Can you double check there weren't crashes or bugs introduced in 68.0.3429.0 from this just to be sure? Thanks!
,
May 14 2018
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 15 2018
There are crashes in RemoteFrameView::UpdateViewportIntersectionsForSubtree going back to M64, but it's not clear to me that this issue is the root cause of those crashes. The more common failure (I would expect) is simply incorrect information being reported by IntersectionObserver.
,
May 15 2018
Thanks. I just meant to sanity check that the CL didn't introduce new crashes or bugs. :)
,
May 15 2018
Ah -- no, I don't see any crashes or bugs that would have been caused by this.
,
May 15 2018
Approving merge to M67 branch 3396 based on comment #4, #5 and #9. Please merge. Thank you.
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a3688bf2019707594731ce6a6090d4a8abdefde9 commit a3688bf2019707594731ce6a6090d4a8abdefde9 Author: Stefan Zager <szager@chromium.org> Date: Tue May 15 15:49:21 2018 Only update remote frame viewport intersection when paint is clean. Cherry-picked from: https://chromium-review.googlesource.com/1056491 BUG= 842368 TBR=kenrb@chromium.org Change-Id: Iaa8f60bc76d98bf1e12a21f0179f8243a0f0cff7 Reviewed-on: https://chromium-review.googlesource.com/1059934 Reviewed-by: Stefan Zager <szager@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#603} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/a3688bf2019707594731ce6a6090d4a8abdefde9/third_party/blink/renderer/core/frame/remote_frame_view.cc |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by bugdroid1@chromium.org
, May 12 2018