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

Issue 731264 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocked on:
issue 731240



Sign in to add a comment

Skip Unload/BeforeUnload on Critical Memory Pressure on desktop [to match Android behavior]

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

Issue description

On desktop, tab manager has to do a full unload of a tab (including running its unload handle) on desktop platforms, if that tab has an unload handler. A quick glance at histograms suggests this means that about 80% of tabs have to do a full unload. Put another way, right now, 80% of all tab kills have to run substantial amounts of blink/v8/js code in order to shut down a tab in response to memory pressure. Best practices for critical memory pressure are to rapidly respond to the memory pressure, as fast as possible.We should modify our behavior 


On Android, we already skip unload logic during critical memory pressure situations. But, this is accidental rather than intentional: on Android, the OS just fast kills processes directly when it reaches a critical state, without routing through the tab manager. Whereas, on other platforms, our code is involved, and our current handling errs in favor of not breaking content.

The change this bug proposes is, on critical memory pressure, on all platforms, to skip running unload/beforeunload handlers.
 

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

Cc: brettw@chromium.org
Labels: -Pri-3 M-61 Pri-1
P1 due to ChromeOS memory push

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

Blockedon: 731240
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d9680cb31e92b038b14f41c6f462341d16c9c321

commit d9680cb31e92b038b14f41c6f462341d16c9c321
Author: Fadi Meawad <fmeawad@chromium.org>
Date: Wed Jul 19 00:15:38 2017

[TabDiscarding] Differentiate between beforeunload and unload handlers

To enable fast killing of tabs in more situations, the TabManager need
to be aware of the cause of the sudden termination disabler. This CL
propagates the disabler type to the RenderFrameHostImpl where it can be
stored and queried.

This step is required to enable the TabManager to fastkill in more
situations.

This CL is extracted from oysteine's CL:
https://chromium-review.googlesource.com/c/537072 which I took over
with his permission.

Bug:  chromium:731264 , chromium:123049 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Ibd3a96df76793d4808bf84edecf4353093d81620
Reviewed-on: https://chromium-review.googlesource.com/562616
Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487680}
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/content/browser/DEPS
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/content/browser/web_contents/web_contents_impl_browsertest.cc
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/content/common/DEPS
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/content/common/frame_messages.h
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/content/public/browser/render_frame_host.h
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/content/renderer/render_frame_impl.h
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/third_party/WebKit/Source/core/frame/LocalFrameClient.h
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/third_party/WebKit/Source/web/AssertMatchingEnums.cpp
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/third_party/WebKit/Source/web/LocalFrameClientImpl.cpp
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/third_party/WebKit/Source/web/LocalFrameClientImpl.h
[add] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/third_party/WebKit/public/platform/WebSuddenTerminationDisablerType.h
[modify] https://crrev.com/d9680cb31e92b038b14f41c6f462341d16c9c321/third_party/WebKit/public/web/WebFrameClient.h

Project Member

Comment 4 by bugdroid1@chromium.org, Jul 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/deafcdf91e3015bd675fb0c088df245e99c70e30

commit deafcdf91e3015bd675fb0c088df245e99c70e30
Author: Fadi Meawad <fmeawad@chromium.org>
Date: Thu Jul 20 16:58:39 2017

[Tab discarding] Ignore unload handlers under critical memory pressure

This happens only when able to fastkilling renderers due to only running
the tab being discarded.

This also sorts tabs with beforeunload handlers before other tabs, as
that state tends to signify tabs with unsaved user state.
(ChromeOS only, for now)

This is an ownership transfer of the CL:
https://chromium-review.googlesource.com/c/537072/

based on oysteine's request since he is OOO. First patch is his CL +
Rebase

R=chrisha
CC=oysteine

BUG:  chromium:731264 , chromium:123049 
Change-Id: If736141c2bb925453b7fa2c48179c1e481591032
Reviewed-on: https://chromium-review.googlesource.com/558007
Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488273}
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/android/tab_android.cc
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/memory/chrome_memory_coordinator_delegate.cc
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/memory/chrome_memory_coordinator_delegate.h
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/resource_coordinator/DEPS
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/resource_coordinator/tab_manager.cc
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/resource_coordinator/tab_manager.h
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/resource_coordinator/tab_manager_browsertest.cc
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.cc
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.h
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos_unittest.cc
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/resource_coordinator/tab_manager_observer_browsertest.cc
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/resource_coordinator/tab_manager_unittest.cc
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/resource_coordinator/tab_stats.h
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/ui/tabs/tab_strip_model.cc
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/browser/ui/webui/about_ui.cc
[add] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/chrome/test/data/unload.html
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/content/browser/memory/memory_coordinator_default_policy.cc
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/content/browser/memory/memory_coordinator_impl.cc
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/content/browser/memory/memory_coordinator_impl.h
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/content/browser/memory/memory_coordinator_impl_unittest.cc
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/content/public/browser/memory_coordinator_delegate.h
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/content/public/browser/render_process_host.h
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/deafcdf91e3015bd675fb0c088df245e99c70e30/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Assigned)

Sign in to add a comment