Ensure foreground tab is not dropped to waive binding when chrome is backgrounded |
|||
Issue descriptionDesired behavior: the renderer process hosting the "foreground" tab should always have the strong binding, regardless of whether chrome activity is foreground/paused/stoppped/etc. (ie it should behave similarly to the gpu process) Current implementation: Connection has addStrongBinding/removeStrongBinding to control strong binding. They are currently controlled by two mechanisms: 1) RenderProcessHostImpl::UpdateProcessPriority will treat visible web contents (and a few other signals) as important 2) BindingManagerImpl keeps mLastConnectionInForeground, and tries to add strong binding if the app is backgrounded These two mechanisms are independent. When app is backgrounded, 1) will remove strong binding, and 2) will add it. There is no guaranteed order which one will trigger first, so there is no guarantee strong binding won't be momentarily dropped. Also since ~m59 (didn't check exact CL), connection no longer keeps a ref count of strong bindings, so strong binding is guaranteed to be either dropped momentarily, or permanently while chrome is in the background. Also mLastConnectionInForeground's is just wrong. Since multiwindow in N, there can be multiple visible tabs. It also doesn't work for cases where there are multiple high priority tabs (eg one that's playing music) Desired implementation: Rip out mLastConnectionInForeground logic. Add a code path parallel to web contents visibility that explicitly controls the strong binding, and use that to control strong binding. Most this will go through cross-platform code, but it will only be used on android. Picking a good name is important. This will also be useful for android webview to control process priorities, which right now just have a giant (although correct) hack to duplicate this behavior. Not sure yet how a invisible tab that's playing music should behave.
,
Jul 27 2017
And update title to reflect new plan
,
Jul 28 2017
I could be wrong, but... I don't think changing the binding while in the background will make any difference. I think strong/default binding are the same when you're in the background. As far as I could tell, then *only* way to order your processes while in the background is to use BIND_ABOVE_CLIENT. I had the idea of using this flag between renderers, but as I think you suspected somewhere else, it's not allowed: Caused by: java.lang.SecurityException: Isolated process not allowed to call bindService
,
Jul 28 2017
It all depends on the android implementation I guess. When chrome is in the background, Android can ignore these signals (and basically does, on older versions), or it can actually use them to make decisions. But we should send the right signals regardless.
,
Jul 31 2017
I think we want the current tab to have important binding when it is visible. When it is not visible, it can drop to default binding. This should fix the original issue. For the invisible media tab, it will use default binding. So if the browser is visible, current tab has higher priority. If the browser is not visible, the current tab and media tab have the same priority.
,
Aug 11 2017
What are the next steps? Is there anything blocking progress? When is a good time to check to see if this work is complete?
,
Aug 11 2017
CLs are under review. Targeting m62 so shouldn't be too much rush? https://chromium-review.googlesource.com/c/602892 https://chromium-review.googlesource.com/c/608824
,
Aug 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7c6779e9aab929a38b5f4633f97ed67fc6804644 commit 7c6779e9aab929a38b5f4633f97ed67fc6804644 Author: Bo Liu <boliu@chromium.org> Date: Wed Aug 16 02:02:28 2017 Add ChildProcessImportance and implement for Android Android requires controls of renderer process importance bindings that's independent of visibility. The current hacky implementation on android tracks the "last made visible" process, which is not completely correct so very error-prone. See bug for details. This is an attempt to properly implement controls for importance bindings on Android. It's a new enum that's plumbed (similar to visibility) from WebContents all the way to ChildProcessLauncher. Note however since the plan is that this will only be used on Android, so the enum and new API is only exposed through java. The path of information flow is: WebContentsImpl -> RenderWidgetHostImpl -> RenderProcessHostImpl -> ChildProcessLauncher. RenderProcessHostImpl aggregates importance of all views and uses the highest importance view as the effective importance. In Android code, DEFAULT corresponds to default binding (without any special bind flags), and IMPORTANT corresponds to important binding (Context#BIND_IMPORTANT). The default binding is now controlled independently by Importance setting path as well as BindingManager so need to add a ref-count for it in ChildProcessConnection. Nothing uses this code path yet in production. Adding an Android demonstrate code path at least works. Bug: 749347 Change-Id: I57a44e7efce89aea96fabb40def252a2dd52ed03 Reviewed-on: https://chromium-review.googlesource.com/602892 Reviewed-by: Charlie Reis (OOO Aug 17-24) <creis@chromium.org> Reviewed-by: Andrew Grieve <agrieve@chromium.org> Commit-Queue: Bo <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#494665} [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/base/android/java/src/org/chromium/base/process_launcher/ChildProcessConnection.java [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/BUILD.gn [add] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/child_process_importance.h [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/child_process_launcher.cc [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/child_process_launcher.h [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/child_process_launcher_helper.h [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/child_process_launcher_helper_android.cc [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/child_process_launcher_helper_linux.cc [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/child_process_launcher_helper_mac.cc [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/child_process_launcher_helper_win.cc [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/renderer_host/render_process_host_impl.cc [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/renderer_host/render_process_host_impl.h [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/renderer_host/render_widget_host_impl.cc [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/renderer_host/render_widget_host_impl.h [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/site_per_process_browsertest.cc [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/web_contents/web_contents_android.cc [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/web_contents/web_contents_android.h [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/browser/web_contents/web_contents_impl.h [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/public/android/BUILD.gn [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/public/android/java/src/org/chromium/content_public/browser/WebContents.java [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/public/android/javatests/src/org/chromium/content/browser/webcontents/WebContentsTest.java [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/public/browser/render_process_host.h [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/public/test/mock_render_process_host.cc [modify] https://crrev.com/7c6779e9aab929a38b5f4633f97ed67fc6804644/content/public/test/mock_render_process_host.h
,
Aug 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d9cb1ee6d4dfa7ca41f723119c4e2d0cb2bf4ec7 commit d9cb1ee6d4dfa7ca41f723119c4e2d0cb2bf4ec7 Author: Bo Liu <boliu@chromium.org> Date: Wed Aug 16 15:07:32 2017 android: Set importance for WebContents Basic idea is the currently "foreground" Tab/WebContents is raised to moderate importance. Note the key different from visibility is that the WebContents still retains moderate importance even if the app is sent to the background. Moderate is used instead of Important because while chrome is in the background, we would want any "user visible" tab, ie those playing music, to have higher importance than the foreground tab. Bug: 749347 Change-Id: I46320e84f09f80114b144dc228218e9d3b1bf5e0 Reviewed-on: https://chromium-review.googlesource.com/608824 Reviewed-by: Jay Civelli <jcivelli@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Commit-Queue: Bo <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#494786} [modify] https://crrev.com/d9cb1ee6d4dfa7ca41f723119c4e2d0cb2bf4ec7/base/android/java/src/org/chromium/base/process_launcher/ChildProcessConnection.java [modify] https://crrev.com/d9cb1ee6d4dfa7ca41f723119c4e2d0cb2bf4ec7/chrome/android/java/src/org/chromium/chrome/browser/SingleTabActivity.java [modify] https://crrev.com/d9cb1ee6d4dfa7ca41f723119c4e2d0cb2bf4ec7/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java [modify] https://crrev.com/d9cb1ee6d4dfa7ca41f723119c4e2d0cb2bf4ec7/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java [modify] https://crrev.com/d9cb1ee6d4dfa7ca41f723119c4e2d0cb2bf4ec7/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java [modify] https://crrev.com/d9cb1ee6d4dfa7ca41f723119c4e2d0cb2bf4ec7/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherIntegrationTest.java [modify] https://crrev.com/d9cb1ee6d4dfa7ca41f723119c4e2d0cb2bf4ec7/content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java
,
Aug 16 2017
visible media tab is still important, but other than that minor thing, all done for chrome
,
Aug 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9c115c17670824d1d6035f766439bd9f9052cdc commit a9c115c17670824d1d6035f766439bd9f9052cdc Author: Bo Liu <boliu@chromium.org> Date: Tue Aug 22 18:27:15 2017 android: Raise importance before lowering In the edge case of switching between two tabs of hosted by the same process, if importance is lowered on the old tab before raised on the new tab, then that process could have its effective importance temporarily dropped unexpectedly. On low ram devices, this could mean the process is killed by the oom killer, leading to a very bad user experience. Bug: 749347 Change-Id: I9f6fe4d737808d5a36c3f0f9d7eb41af08898c7f Reviewed-on: https://chromium-review.googlesource.com/624719 Commit-Queue: Bo <boliu@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Cr-Commit-Position: refs/heads/master@{#496374} [modify] https://crrev.com/a9c115c17670824d1d6035f766439bd9f9052cdc/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java |
|||
►
Sign in to add a comment |
|||
Comment 1 by boliu@chromium.org
, Jul 27 2017