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

Issue 731145 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Allow all non-visible tabs to be discarded.

Project Member Reported by fdoray@chromium.org, Jun 8 2017

Issue description

Currently, 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.
 

Comment 1 by nduca@chromium.org, Jun 8 2017

Labels: Hotlist-GRC

Comment 2 by creis@chromium.org, Jun 13 2017

Cc: creis@chromium.org
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)?

Comment 3 by fdoray@chromium.org, 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.

Comment 4 by nduca@chromium.org, 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....

Comment 5 by creis@chromium.org, Jun 13 2017

Cc: est...@chromium.org
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?

Comment 6 by est...@chromium.org, Jun 13 2017

Cc: sgabr...@chromium.org tbuck...@chromium.org
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.

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

Comment 8 by creis@chromium.org, Jun 15 2017

Components: UI>Browser>Navigation Internals>GPU
Labels: Stability-Memory Performance-Memory
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.
Project Member

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

Cc: sky@chromium.org
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. 

Comment 11 by sky@chromium.org, 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.
Project Member

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

Project Member

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

Project Member

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

Project Member

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

Project Member

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

Comment 17 by fdoray@google.com, Aug 3 2017

Status: Fixed (was: Started)
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