Avoid freezing visible pages in page scheduler |
||||||||
Issue descriptionOn the embedder page freezing path, the page and frame schedulers fire DCHECKS and NOTREACHED if attempting to freeze a visible page. On release builds, the freeze will succeed, leaving the scheduler in a possibly bad state. Furthermore, on Chrome OS, NOTREACHED will result in error log messages. The page scheduler should be defensive and reject the freeze in cases where the page is visible.
,
Aug 23
,
Aug 23
,
Aug 23
,
Aug 23
,
Aug 23
Before we approve merge to M69, please answer followings: * Is this M69 regression? Is it critical? * Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M69? * Any other important details to justify the merge. Please note M69 is going to stable VERY soon, so we're only taking absolutely critical and safe merges in at this point. Thank you.
,
Aug 23
This bug requires manual review: We are only 11 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 23
* Is this M69 regression? Is it critical? Yes. It is a stability improvement, will remove lot of log errors (see crbug//873214) and the risk of freezing pages that are visible to the user. * Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M69? Yes, it will well tested, verified in canary, it fixes the issue in crbug/873214 * Any other important details to justify the merge. Freezing & Discarding is an important new feature in the birthday release of Chrome. This fixes an important bug in freezing - where visible pages could be frozen -- which would be a very bad user experience.
,
Aug 23
crbug//873214 is for Chrome OS. Is this merge request is for Chrome OS only?
,
Aug 23
No, we would want this for all desktop platforms. Because of how Chrome OS compiles NOTREACHED on release bugs (it logs them), we were able to figure out that this was happening. We changed a similar check to a dump w/o crashing and found this is an issue on multiple platforms (see crbug/873214 here also).
,
Aug 23
^ release builds, not bugs
,
Aug 23
Approving merge to M69 branch 3497 based on comment #8. Please merge ASAP, Thank you.
,
Aug 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eec902abb5a888786d3bd0c83ef23e9914d6c12d commit eec902abb5a888786d3bd0c83ef23e9914d6c12d Author: Scott Haseley <shaseley@google.com> Date: Fri Aug 24 00:22:59 2018 [scheduler] Do not allow freezing visible pages in PageSchedulerImpl We have a DCHECK and a NOTREACHED that fire when freezing a visible page, but the freeze will still succeed on release builds. This CL adds a check to ensure visible pages cannot be frozen by the embedder. Bug: 875995 Change-Id: I933c5e9377e0fa9670c60efa6deb2ced7ef1601b Reviewed-on: https://chromium-review.googlesource.com/1180503 Commit-Queue: Scott Haseley <shaseley@google.com> Reviewed-by: Alexander Timin <altimin@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#584597}(cherry picked from commit 6f67719f5ea0360a9099e114b26c8d18c8a3fd3d) Reviewed-on: https://chromium-review.googlesource.com/1187097 Reviewed-by: Shubhie Panicker <panicker@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#796} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/eec902abb5a888786d3bd0c83ef23e9914d6c12d/third_party/blink/renderer/platform/scheduler/main_thread/page_scheduler_impl.cc |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by bugdroid1@chromium.org
, Aug 21