Allow all non-visible tabs to be discarded. |
|||||||
Issue descriptionCurrently, the selected tab in a window cannot be discarded, even if the window is not visible. That means that if a lot of single-tab windows are opened and a memory pressure signal is received, no tab will be discarded.
,
Jun 13 2017
How will this interact with features like Expose on Mac and Windows+Tab on Windows, which make it possible to quickly get an overview of the open windows? Will the thumbnails of these windows hold the last paint before the discard, or will it degrade (e.g., show a sad tab or force an immediate reload in all the discarded and now-"visible" windows in the overview)?
,
Jun 13 2017
We want a quick solution to allow selected tabs in non-visible windows to be discarded. Initially, that involves a degraded Expose/Windows+Tab/Overview experience (users will see an empty tab with a grey background). We believe that this experience is better than keeping a machine under critical memory pressure [1]. The long-term solution is to take a screenshots of tabs and show that instead of grey backgrounds. This is a non-trivial project (take screenshots ahead of time to avoid increasing memory usage, load screenshots with minimal memory consumption). We want to allow discarding selected tabs in non-visible windows before we implement this long-term solution. [1] The experience is worse on ChromeOS. We could consider enable discarding of selected tabs in non-visible windows only on that platform.
,
Jun 13 2017
I think it might be useful for us to recognize that there're two things mixed into here: 1) there's the short term concerns around CrOS and its woes with memory, 2) there's the "do the right thing for chrome desktop products, like osx" For #1, chromeos, there is a desire for a rapid-if-imperfect solution, so in those caes some breakage and funky behavior might in fact be okay. But, for #2, the other platforms, we clearly have to be more careful and dilligent. We definitely intends to take care of both #1 and #2 concerns. But, because they're both mashed into one bug, I think we can end up accidentally getting confused. Maybe we just are careful to recognize that there're two things happening here. Or maybe we split this bug into CrOS bug and then the other platforms bug....
,
Jun 13 2017
Comments 3-4: Thanks for the context. I'm uncomfortable regressing the tab overview behavior on all platforms if the problem is only being acutely felt on ChromeOS. I'll point out that this will degrade the UX on ChromeOS as well. +estade who I think worked on the new switcher that shows nice screenshots of each window on ChromeOS when you hit Alt+Tab. I agree that critical memory pressure is more important to address, but I also think we don't want to punt the UX degredation for a long time. Maybe it makes sense to proceed with this on ChromeOS only in the short term?
,
Jun 13 2017
as noted this affects alt tab and overview mode. How often would users get grey tabs? I think you should run this by UI review as it seems like it has potential to be a major behavioral regression. Perhaps there's something easier than the long term solution listed above but less ugly than an empty tab.
,
Jun 15 2017
Tab discarding occurs when the machine is under critical memory pressure. Users have a terrible experience already when their machine is under critical memory pressure. Tabs may crash (i.e. be replaced with an "Aw, Snap!" page). The machine may freeze. We've also observed GPU memory corruption. Discarding tabs makes this experience less painful. We kill renderers from less important to most important before the kernel starts intervening and we automatically reload discarded tabs when they are brought to the foreground. I agree that showing a grey background is not the best we can do. I'll explore other solutions share them with UI review. But we still think that a quick fix to this issue would improve UX.
,
Jun 15 2017
Thanks. Is there a bug for the GPU memory corruption issue? That shouldn't be happening solely due to renderer memory use, and we should be fixing it independently. I agree this will give us some more breathing room, but I still think we should consider making this ChromeOS specific (where the problem is most pressing) until the grey background problem is fixed.
,
Jun 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eaace7813ae608e491f7aa502b08da57b32ce0bc commit eaace7813ae608e491f7aa502b08da57b32ce0bc Author: Francois Doray <fdoray@chromium.org> Date: Fri Jun 23 01:59:09 2017 Add |needs_reload| argument to NavigationController::CopyStateFrom(). This argument controls whether the WebContents needs to be reloaded when activated. Not reloading the WebContents as soon as it is activated will allow TabManager to implement more advanced reload rules (e.g. browser window is active, tab has been active for x ms). Note: Today, a WebContents is considered "active" if it is in a selected tab, even if the browser window is not visible. TBR=bauerb@chromium.org,chrisha@chromium.org,bengr@chromium.org Bug: 731145 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I87a16df112752b97bcc369e34a1e1f86d18d9b99 Reviewed-on: https://chromium-review.googlesource.com/531065 Commit-Queue: Francois Doray <fdoray@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Cr-Commit-Position: refs/heads/master@{#481778} [modify] https://crrev.com/eaace7813ae608e491f7aa502b08da57b32ce0bc/chrome/android/javatests/src/org/chromium/chrome/browser/NavigationPopupTest.java [modify] https://crrev.com/eaace7813ae608e491f7aa502b08da57b32ce0bc/chrome/browser/dom_distiller/tab_utils.cc [modify] https://crrev.com/eaace7813ae608e491f7aa502b08da57b32ce0bc/chrome/browser/resource_coordinator/tab_manager.cc [modify] https://crrev.com/eaace7813ae608e491f7aa502b08da57b32ce0bc/content/browser/frame_host/navigation_controller_android.cc [modify] https://crrev.com/eaace7813ae608e491f7aa502b08da57b32ce0bc/content/browser/frame_host/navigation_controller_android.h [modify] https://crrev.com/eaace7813ae608e491f7aa502b08da57b32ce0bc/content/browser/frame_host/navigation_controller_impl.cc [modify] https://crrev.com/eaace7813ae608e491f7aa502b08da57b32ce0bc/content/browser/frame_host/navigation_controller_impl.h [modify] https://crrev.com/eaace7813ae608e491f7aa502b08da57b32ce0bc/content/browser/frame_host/navigation_controller_impl_unittest.cc [modify] https://crrev.com/eaace7813ae608e491f7aa502b08da57b32ce0bc/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/eaace7813ae608e491f7aa502b08da57b32ce0bc/content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java [modify] https://crrev.com/eaace7813ae608e491f7aa502b08da57b32ce0bc/content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java [modify] https://crrev.com/eaace7813ae608e491f7aa502b08da57b32ce0bc/content/public/browser/navigation_controller.h [modify] https://crrev.com/eaace7813ae608e491f7aa502b08da57b32ce0bc/content/test/test_web_contents.cc
,
Jun 23 2017
As pointed out by sky on this review: https://chromium-review.googlesource.com/c/527688/13/chrome/browser/resource_coordinator/tab_manager.cc#120 "I think that code is making a potentially bad assumption. It is assuming last-active order corresponds to reverse z-order. There is no guarantee last active order and reverse z-order are the same. It's entirely possible for a window manager to reorder windows in anyway it seems fit. We only update the BrowserList when the active window changes." Before extending this code to other non-ChromeOS platforms we will need full native window manager information.
,
Jun 23 2017
The same problem exists on ChromeOS too. alt-tab on ChromeOS (and no doubt similar gestures on other OSs) shows the window contents of background windows. If the active tab in one of the background windows has been discarded the representation isn't going to look good.
,
Jun 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/95f28290d4ea9ddcd78f2a71465d15560711a9a8 commit 95f28290d4ea9ddcd78f2a71465d15560711a9a8 Author: Francois Doray <fdoray@chromium.org> Date: Wed Jun 28 22:01:49 2017 Add ui::SortWindowsByZIndex(). This function sorts a list of gfx::NativeWindow by z-index, from topmost to bottommost, and returns the result via an asynchronous callback. The function is currently only implemented on ChromeOS. Bug: 731145 Change-Id: Ie16faeabad2133a00f1013a4968b917cc605ed2d Reviewed-on: https://chromium-review.googlesource.com/550407 Commit-Queue: Francois Doray <fdoray@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#483161} [modify] https://crrev.com/95f28290d4ea9ddcd78f2a71465d15560711a9a8/chrome/browser/ui/BUILD.gn [add] https://crrev.com/95f28290d4ea9ddcd78f2a71465d15560711a9a8/chrome/browser/ui/ash/sort_windows_by_z_index.cc [add] https://crrev.com/95f28290d4ea9ddcd78f2a71465d15560711a9a8/chrome/browser/ui/sort_windows_by_z_index.h [add] https://crrev.com/95f28290d4ea9ddcd78f2a71465d15560711a9a8/chrome/browser/ui/sort_windows_by_z_index_browsertest.cc [modify] https://crrev.com/95f28290d4ea9ddcd78f2a71465d15560711a9a8/chrome/test/BUILD.gn
,
Jun 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bd74fad1ea239ba7784f7a417b97f0276cc52aa1 commit bd74fad1ea239ba7784f7a417b97f0276cc52aa1 Author: Francois Doray <fdoray@chromium.org> Date: Thu Jun 29 17:20:39 2017 Remove code that computes window occlusion from TabManager. With this CL, TabManager only considers a window non-visible if it is minimized. In a separate CL, TabManagerDelegate (ChromeOS) will request a list of windows sorted by z-index and will compute window occlusion when it gets the result. Bug: 731145 Change-Id: I8a95551f30e5e1b95e5d263c252b007bb8c6f3dc Reviewed-on: https://chromium-review.googlesource.com/552876 Reviewed-by: Chris Hamilton <chrisha@chromium.org> Commit-Queue: Francois Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#483406} [modify] https://crrev.com/bd74fad1ea239ba7784f7a417b97f0276cc52aa1/chrome/browser/resource_coordinator/tab_manager.cc [modify] https://crrev.com/bd74fad1ea239ba7784f7a417b97f0276cc52aa1/chrome/browser/resource_coordinator/tab_manager.h [modify] https://crrev.com/bd74fad1ea239ba7784f7a417b97f0276cc52aa1/chrome/browser/resource_coordinator/tab_manager_unittest.cc
,
Jun 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e067fdd3df43af449007fcfccb77e59e8eecf424 commit e067fdd3df43af449007fcfccb77e59e8eecf424 Author: Francois Doray <fdoray@chromium.org> Date: Fri Jun 30 16:30:23 2017 Allow tabs in non-visible windows to be discarded on ChromeOS. For now, a window is considered non-visible only if it is minimized. More advanced visibility checks will be added in a separate CL. Note: Previously, both NavigationController and Browser::ActiveTabChanged reloaded discarded tabs when they became active. With this CL, discarded tabs are reloaded by ReloadWebContentsIfDiscarded(), which is called when the active browser changes or when the active tab in the active browser changes. TBR=tommi@chromium.org Bug: 731145 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I2168a772d876fe8bf9bcee24b923c4ffef1f954b Reviewed-on: https://chromium-review.googlesource.com/527688 Reviewed-by: Francois Doray <fdoray@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Reviewed-by: Chris Hamilton <chrisha@chromium.org> Commit-Queue: Francois Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#483729} [modify] https://crrev.com/e067fdd3df43af449007fcfccb77e59e8eecf424/chrome/browser/resource_coordinator/DEPS [modify] https://crrev.com/e067fdd3df43af449007fcfccb77e59e8eecf424/chrome/browser/resource_coordinator/tab_manager.cc [modify] https://crrev.com/e067fdd3df43af449007fcfccb77e59e8eecf424/chrome/browser/resource_coordinator/tab_manager.h [modify] https://crrev.com/e067fdd3df43af449007fcfccb77e59e8eecf424/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.cc [modify] https://crrev.com/e067fdd3df43af449007fcfccb77e59e8eecf424/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.h [modify] https://crrev.com/e067fdd3df43af449007fcfccb77e59e8eecf424/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos_unittest.cc [modify] https://crrev.com/e067fdd3df43af449007fcfccb77e59e8eecf424/chrome/browser/resource_coordinator/tab_manager_unittest.cc [modify] https://crrev.com/e067fdd3df43af449007fcfccb77e59e8eecf424/chrome/browser/resource_coordinator/tab_stats.h [modify] https://crrev.com/e067fdd3df43af449007fcfccb77e59e8eecf424/chrome/browser/ui/browser.cc
,
Jun 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ef156e2ac9426becb159d0f61ab58c57a8f16c25 commit ef156e2ac9426becb159d0f61ab58c57a8f16c25 Author: Francois Doray <fdoray@chromium.org> Date: Fri Jun 30 19:11:25 2017 Pass vector of ArcProcesses by value to RequestProcessListCallback. Pass the vector of ArcProcesses by value rather than by const& to the RequestProcessListCallback. The vector is moved from the source to the callback, so this CL does not incur additional copies (as proved by the DISALLOW_COPY_AND_ASSIGN in the definition of ArcProcess). This change allows the RequestProcessListCallback to move the vector it receives to another variable. TBR=chrisha@chromium.org Bug: 731145 Change-Id: If91b4107c3b723de38ba80d281f19dafcdada54b Reviewed-on: https://chromium-review.googlesource.com/557909 Commit-Queue: Francois Doray <fdoray@chromium.org> Reviewed-by: Luis Hector Chavez <lhchavez@chromium.org> Reviewed-by: Ahmed Fakhry <afakhry@chromium.org> Cr-Commit-Position: refs/heads/master@{#483784} [modify] https://crrev.com/ef156e2ac9426becb159d0f61ab58c57a8f16c25/chrome/browser/chromeos/arc/process/arc_process_service.h [modify] https://crrev.com/ef156e2ac9426becb159d0f61ab58c57a8f16c25/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.cc [modify] https://crrev.com/ef156e2ac9426becb159d0f61ab58c57a8f16c25/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.h [modify] https://crrev.com/ef156e2ac9426becb159d0f61ab58c57a8f16c25/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos_unittest.cc [modify] https://crrev.com/ef156e2ac9426becb159d0f61ab58c57a8f16c25/chrome/browser/task_manager/providers/arc/arc_process_task_provider.cc [modify] https://crrev.com/ef156e2ac9426becb159d0f61ab58c57a8f16c25/chrome/browser/task_manager/providers/arc/arc_process_task_provider.h
,
Jul 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/26389b471309a5cd75e903ab13f8f37bf5f78942 commit 26389b471309a5cd75e903ab13f8f37bf5f78942 Author: Francois Doray <fdoray@chromium.org> Date: Fri Jul 28 16:41:39 2017 Compute window occlusion in TabManagerDelegate on ChromeOS. This CL adds code to take window occlusion into consideration when determining which windows are visible before discarding tabs on ChromeOS. With this CL, TabManager::LowMemoryKill starts asynchronous requests to get the list of ARC processes and the list of browser windows sorted by z-index (required to compute window occlusion). TabManager::LowMemoryKillImpl is invoked when the result of both requests is available. Bug: 731145 Change-Id: Ic66a55bbbd22b7d7c73dc94e99153c4952811e19 Reviewed-on: https://chromium-review.googlesource.com/557910 Reviewed-by: Chris Hamilton <chrisha@chromium.org> Commit-Queue: Francois Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#490429} [modify] https://crrev.com/26389b471309a5cd75e903ab13f8f37bf5f78942/chrome/browser/resource_coordinator/tab_manager.cc [modify] https://crrev.com/26389b471309a5cd75e903ab13f8f37bf5f78942/chrome/browser/resource_coordinator/tab_manager.h [modify] https://crrev.com/26389b471309a5cd75e903ab13f8f37bf5f78942/chrome/browser/resource_coordinator/tab_manager_browsertest.cc [modify] https://crrev.com/26389b471309a5cd75e903ab13f8f37bf5f78942/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.cc [modify] https://crrev.com/26389b471309a5cd75e903ab13f8f37bf5f78942/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.h [modify] https://crrev.com/26389b471309a5cd75e903ab13f8f37bf5f78942/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos_unittest.cc [modify] https://crrev.com/26389b471309a5cd75e903ab13f8f37bf5f78942/chrome/browser/resource_coordinator/tab_manager_unittest.cc
,
Aug 3 2017
Done on ChromeOS. Implementing this on other platforms is a longer-term project that will require capturing tab screenshots (so that the user can't see discarded tabs when using Windows+Tab on Windows or Expose on Mac). |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by nduca@chromium.org
, Jun 8 2017