Issue 683377  has been merged into this issue.

creis/nasko: thoughts on plan below for depth?

FrameTreeNode maintains depth and probably keep it in RenderFrameHostManager. RenderFrameHostManager pass the depth down to all the RenderFrameHostImpls it creates, which can then talk to the process it belongs to. RenderProcessHostImpl probably need to aggregate over all the frames like it does right now with all the widgets.

actually I think I missed a step in #3. WidgetHost needs to aggregate over its FrameHosts, then ProcessHost aggregates over all WidgetHosts.

Sorry for the delay.  I like the general plan of keeping track of the relative priority of processes based on which ones are used by main frames and subframes, and even tracking the depth to help with pathological cases (such that we'd discard processes for deeper frames first).  Presumably this would affect pending processes as well, so that a newly created nested subframe process won't cause its parent process to be killed.

What does "use" mean specifically (with respect to the timestamp of last use)?

A few thoughts on the proposal in comment 3:

It's ok for FrameTreeNode to track its depth if you don't want to compute it each time by walking up the tree.  RenderFrameHostManager and RenderFrameHostImpl shouldn't have to cache that as well-- it's easy for them to ask their FTN whenever it's needed.

Note that RenderProcessHostImpl doesn't have a way to iterate over its frames, which would be a bit of a layering violation (renderer_host doesn't depend on frame_host in general).  I can see that RenderProcessHost kind of needs to know the values for the "highest priority" frame in its process, but maybe we can do that another way.

Some possibilities:
1) RFHs (and perhaps workers and other things that create processes?) can call a method on RPH to pass along their priorities on creation, notify when they're updated, and let RPH know when they're deleted.  RPH keeps track of the bookkeeping, as well as its current notion of aggregate priority.
2) We tell RenderWidgetHostImpl the depth of its local root frame, and RPH can find it from there when walking over widgets.  I'm not sure I like this quite as much, since it doesn't address the worker case and kind of puts frame info on the widget.

Would workers get something like a depth of 1, acting like an immediate child of some main frame?

[+kenrb for widget thoughts and falken for ServiceWorker thoughts]

I have written the depth part here, though not quite ready for commit yet:
https://chromium-review.googlesource.com/c/chromium/src/+/934311

It's closer to possibility 1) you described.

> What does "use" mean specifically (with respect to the timestamp of last use)?

relative timestamp of when frame/widget became visible, maybe? (android currently relies on the process becoming foreground, which is wrong when widgets are created and destroyed)

> Note that RenderProcessHostImpl doesn't have a way to iterate over its frames, which would be a bit of a layering violation (renderer_host doesn't depend on frame_host in general).  I can see that RenderProcessHost kind of needs to know the values for the "highest priority" frame in its process, but maybe we can do that another way.

As writing that CL, I realized there is no point in having Widget aggregate over all the Frames that belong to it. It just needs the depth of its local root frame. So I think we are ok here.

> Would workers get something like a depth of 1, acting like an immediate child of some main frame?

For workers, I think that makes sense. But it would be more important to get the equivalent of "visibility" correct here. How does that work right now?

One more thing I just realized from typing out the comment above:

Currently as written in the doc, the relative order of "foreground tab subframe" and "background tab main frame" changes depending on whether the chrome app is foreground or background.

Note that "foreground/background" tab is not the same as WebContents::WasShown/WasHidden. The single selected tab in a window is "foreground", and the other tabs in the window are "background". But android (and I assume desktop too) marks the selected tab as hidden if the window is hidden as well. So for this to work, content needs to know whether a WebContents is "foreground/background".

How receptible are owners here of actually introducing this foreground/background concept into content?

Or alternatively, there's an argument to be made that we shouldn't change relative order of things based on whether app is foreground or background? Though this kind of depends more on user experience than any technical argument..


And taking a step back. Anyone know how much of this stuff is useful for chromeos?

All the stakeholders are here, so I think we should also discuss what happens to killed subframes. Added below to doc as well, that affects 

Handling killed renderers
Without OOPIF, if any tab is killed while invisible, the tab is reloaded automatically when it becomes visible. If tab is killed while visible, then a sad tab is shown and user can manually refresh the page.

What should happen to killed subframes when OOPIF is enabled?
Killed in background and upon restore to visible:
1) current behavior is show a sad tab. But user has no way to reload a single subframe. Note also the current sad tab look very well with small iframes.
2) Reload automatically
3) Do not automatically reload, allow user to reload an iframe manually, eg maybe make clicking on sad tab reload the frame?

Killed while visible: probably only 1 and 3 apply here.

2 seems most user friendly, but could lead to churn, eg when switching between two tabs rapidly, and each tab uses more than 50% of the available “capacity” of renderer processes on the device.

3 seems nice to do for desktop as well, and clear better user experience on android

Wrote some other options in doc. But most important question:
It may not technically be possible to restore an iframe. Eg is it possible to reconnect all the things in js and whatnot?

nts: need to be careful with crash metrics, eg right now, android's crash metrics ignores all iframe crashes

Comment 6: Thanks.  I've left comments in the doc (https://docs.google.com/document/d/1la6Ytp4B5V9COsoD9b1Mx3UxvCZ06RIDSHFu26CsQDI/edit).  Note that your CL seems to be implementing option 2 from comment 5, not option 1.  That is, it's telling RenderWidgetHost about the depth, which I wasn't thrilled with because of the extra info on RenderWidgetHost and because it's not a general solution that applies to both frames and workers.  Option 1 would be a more general API on RPH that both frames and workers could use to inform it of priorities, which RPH would keep in kind of a refcount.  (Also, I have no idea how workers track visibility or how that concept applies-- maybe falken@ would know.)

Comment 7: I mentioned in the doc that we probably shouldn't swap priority based on whether Chrome is the foreground or background app.

Comments 8-9: Adding avi@, since he recently dealt with this in  issue 767526 .  I left some comments in the doc about it.

Comment 10: Thanks-- can you file a bug for that?

> Option 1 would be a more general API on RPH that both frames and workers could use to inform it of priorities, which RPH would keep in kind of a refcount.

Ahh I see. But the problem is visibility and depth are not independent. For example:
process A has visible sub frame with depth 2 and invisible main frame (depth 0)
process B has visible sub frame with depth 1

You would expect process B to have higher priority. But doing the independent "compare aggregated visibility, then compare aggregated depth" would result in process A higher. (Unless of course that kind of sharing will *never* happen?)


> (Also, I have no idea how workers track visibility or how that concept applies-- maybe falken@ would know.)

I would really like to get an idea of how workers work, even today, before proceeding further here with implementing depth. falken@ please help?


> Comment 7: I mentioned in the doc that we probably shouldn't swap priority based on whether Chrome is the foreground or background app.

I think that's definitely the easier first step. We can always revisit if it turns out to be a problematic user experience. Will update doc accordingly


> Comment 10: Thanks-- can you file a bug for that?

Histogram is recorded here:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java?rcl=f6107dd175be7849918da34e555b4baaa81df2cd&l=125

Which is based on WebContentsObserver::RenderProcessGone: https://cs.chromium.org/chromium/src/content/public/browser/web_contents_observer.h?rcl=1c75d4a91f50293b8af18b4a40ade73eb237e229&l=114

The comment says RenderProcessGone is called only for main RVH. Although I didn't actually check if comment is correct. I'll double check and file a bug if it really is broken.

"The comment says RenderProcessGone is called only for main RVH. Although I didn't actually check if comment is correct. I'll double check and file a bug if it really is broken."

RenderProcessGone is indeed only called for the main RVH. That probably should be fixed in some way.

Filed  crbug.com/816968  for crash metric.

Got a hacky CL to restore crashed subframes:
https://chromium-review.googlesource.com/c/chromium/src/+/940621 Although haven't had time to play with it too much

As I was playing around with http://csreis.github.io/tests/cross-site-iframe-simple.html and found out that the subframe process has two RenderWidgetHosts. One is coming from RenderFrameProxyHost apparently. Read the documentation on RenderFrameProxyHost, but I don't really understand how that relates to an extra RenderWidgetHost. Anyone here can give a short explanation?

Going to play with the frame restore CL, and investigate how worker process priority work next.

Found  crbug.com/817606  while playing with the restore CL

Looked at workers a bit.

Shared worker is simple. Its lifetime is tied to RenderFrames, so should just implement some form of "transfer priority from frame" kind of thing. Can do this in SharedWorkerHost.

Service worker is a lot more complicated. It has its own independent life time, and not necessarily tied to RenderFrames. So I'm not sure what's a good way policy for priority here. From looking at docs, I think it makes sense to have "transfer priority from frame" when it's used by a frame. But I'm not sure about other cases.

Honestly, I didn't get very far with understanding how all the pieces in c/b/service_worker connect together. But at least found that RenderProcessHost is created or resued from ServiceWorkerProcessManager::AllocateWorkerProcess.


And independent of site isolation, these out of process workers are pretty broken on chrome on android today already. They provide no priority signal to RPH, which means when the last frame of a RPH goes away, the RPH drops to waived binding, meaning any worker still in RPH will very likely to be killed by android. Also once again, android crash renderer crash metric being tied to WebContentsObserver::RenderProcessGone ignores worker-only kills.

I'm very sorry I missed the questions directed to me on this bug. I must have missed them while scanning through emails.

I confirm c#17 is accurate. Service workers are not necessarily tied to any frame. For example, a push notification can wake up a service worker or without any frame open or even without the browser UI open.

In theory, service workers are self-limiting and transient: they can only run when there is something useful for them to do: i.e., they are receiving events. The types of events supported by service workers are carefully designed to maintain the property of transience. The browser terminates a service worker when it hasn't received events for a while. So having service workers always be "high priority" could possibly make sense, and they can be trusted to go away when no longer needed. Unfortunately, there are possibly ways people can abuse service workers to keep them alive indefinitely without a frame open, but we consider those to be bugs and close them when known (e.g., issue  issue 647943  and  issue 648836 ).

If more granularity is desired, granting the service worker the same priority as the highest-priority same-origin frame could make sense, but we'd want to ensure that any new starting service workers have a chance to run (e.g., for push notification events) and perhaps have the priority decay over time.

I'm very interested to know about how out of process workers are broken on Android today. We keep the RPH alive for workers and other things independent of frames via the KeepAliveRefCount in RenderProcessHost. Is this not sufficient to ensure the RPH stays alive?

> I'm very interested to know about how out of process workers are broken on Android today. We keep the RPH alive for workers and other things independent of frames via the KeepAliveRefCount in RenderProcessHost. Is this not sufficient to ensure the RPH stays alive?

Essentially nothing in the background doc is set up worker-only renderer processes:
https://docs.google.com/document/d/1PTI3udZ6TI-R0MlSsl2aflxRcnZ5blnkGGHE5JRI_Z8/edit?usp=sharing

So essentially workers processes (with no RenderFrames) aren't protected from being killed by android to free memory, so in practice, they probably are killed frequently. And afaik we don't really have metrics to show how bad it is really. Although I don't know how common worker process without RenderFrame is.

> The browser terminates a service worker when it hasn't received events for a while.

Do you know how long this is exactly?

> So having service workers always be "high priority" could possibly make sense, and they can be trusted to go away when no longer needed.

Yeah that sounds pretty sensible. Probably use the default binding for android.

Thanks, I'll read through the doc.

>> The browser terminates a service worker when it hasn't received events for a while.

> Do you know how long this is exactly?

If the service worker hasn't gotten an event in 30 seconds, we call it an "idle timeout" and terminate it. The idle timeout check itself runs every 30 seconds. So in practice a service worker is idle for 30 sec - 1 min before termination.

This timeout can likely be adjusted if needed, the downside to more early termination is just the worker might be needed again soon (e.g., user navigates to another page that needs the worker) and would have to startup again which can take hundreds of ms.

Can anyone confirm my understanding of the relationship between RenderFrameProxyHost and RenderView/WidgetHosts?

* FrameTree::render_view_host_map_ is a map from SiteInstance to RVH in this frame tree.
* The "root" RVH in that map is a "real" RVH, ie has the root RenderFrameHost attached, and is not swapped out.
* The "non-root" RVHs in that map are "not real", in the sense that they are only there so other RenderFrameProxyHosts can talk to it, and they are always swapped out and hidden, even when the actually page is visible.
* Non-root RenderFrameHostImpls have their own "real" RVHs that's not in the FrameTree map.


The problem I'm trying to figure out is that, for these swapped out RVHs, there isn't really any depth value that makes sense, or at least I don't see a straightforward way to go from RVH to a RenderFrameHost or FrameTreeNode. So how about make swapped out RVH/RWH not contribute depth its RenderProcessHostImpl? I think that makes sense?

Also out of curiosity, why have these swapped out RVHs instead of using the "real" RVHs? I assume it has something to do with object lifetimes, but I can't figure out the exactly reason.

Swapped out RVHs should not be counted at all for priorities. They are just there for placeholders. RenderWidgetHosts should be the only objects that are considered, though for cases where RWH is associated with RFH, I'm not sure counting both is worthwhile.

There are also cases where RWH is used without RFH. If I'm not mistaken, context menus and select drop-downs are examples of such cases. For those cases we do want to consider that the process is hosting them and take them into account for priority calculations.

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

commit eb218ee0aa3d02f6a14b23895fdc3c2285c1ebc8
Author: Bo Liu <boliu@chromium.org>
Date: Wed Mar 21 20:24:35 2018

Refactor RenderProcessHostImpl widget aggregation

Combine updating visibility and importance of a widget into a single
call on RenderProcessHost to update its priority. RPH keeps track of
the priority of each widget (client) and aggregates over them to
compute the effective priority of the process.

This CL is meant to be a pure refactor with no behavior changes.

There are few reasons for this design for future changes:
* New fields are going to be added, so combining them into a single
  struct will adding new fields easier.
* New fields will not be independent, but have meaning that depends on
  existing priority. Tracking fields independently will lead to
  incorrect result; see  crbug.com/813232#c12 . So they have to be
  updated together.
* Wording has been changed from "widget" to "client" so that workers
  can also contribute priority to RPH in the future.

Bug:  813232 
Change-Id: Iaa6877af5010100562655be6b1aa2951219d67b5
Reviewed-on: https://chromium-review.googlesource.com/933224
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544824}
[modify] https://crrev.com/eb218ee0aa3d02f6a14b23895fdc3c2285c1ebc8/components/visitedlink/browser/visitedlink_event_listener.cc
[modify] https://crrev.com/eb218ee0aa3d02f6a14b23895fdc3c2285c1ebc8/components/visitedlink/test/visitedlink_unittest.cc
[modify] https://crrev.com/eb218ee0aa3d02f6a14b23895fdc3c2285c1ebc8/content/browser/browser_plugin/browser_plugin_guest.h
[modify] https://crrev.com/eb218ee0aa3d02f6a14b23895fdc3c2285c1ebc8/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/eb218ee0aa3d02f6a14b23895fdc3c2285c1ebc8/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/eb218ee0aa3d02f6a14b23895fdc3c2285c1ebc8/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/eb218ee0aa3d02f6a14b23895fdc3c2285c1ebc8/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/eb218ee0aa3d02f6a14b23895fdc3c2285c1ebc8/content/public/browser/render_process_host.h
[modify] https://crrev.com/eb218ee0aa3d02f6a14b23895fdc3c2285c1ebc8/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/eb218ee0aa3d02f6a14b23895fdc3c2285c1ebc8/content/public/test/mock_render_process_host.h

I filed issue 824628 to make sure what boliu@ mentioned in comment is tracked (but could possibly merge it back here if the work on this bug fixes it).

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

commit e0ec68b9d09bda94fc434a6a5118e2fa01147cce
Author: Bo Liu <boliu@chromium.org>
Date: Mon Mar 26 18:24:06 2018

Rename [Compute=>Get]EffectiveImportance

This method just returns a member since r544824.

Bug:  813232 
Change-Id: I7e8ee5e33b559f0fca0c5ee3f2cb7dc9837458a0
Reviewed-on: https://chromium-review.googlesource.com/980825
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545844}
[modify] https://crrev.com/e0ec68b9d09bda94fc434a6a5118e2fa01147cce/android_webview/browser/aw_contents.cc
[modify] https://crrev.com/e0ec68b9d09bda94fc434a6a5118e2fa01147cce/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/e0ec68b9d09bda94fc434a6a5118e2fa01147cce/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/e0ec68b9d09bda94fc434a6a5118e2fa01147cce/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/e0ec68b9d09bda94fc434a6a5118e2fa01147cce/content/public/browser/render_process_host.h
[modify] https://crrev.com/e0ec68b9d09bda94fc434a6a5118e2fa01147cce/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/e0ec68b9d09bda94fc434a6a5118e2fa01147cce/content/public/test/mock_render_process_host.h

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

commit a13e7c09e023e2c4448407f0a47251aa403326b5
Author: Bo Liu <boliu@chromium.org>
Date: Wed Mar 28 22:24:02 2018

Add frame depth to ChildProcessLauncherPriority

Frame depth is the depth of the FrameTreeNode in the frame tree, with
the root being depth 0. This will be used to rank render processes.

Note depth and visibility are not independent when ranking widgets.
A visible widget of higher depth has higher priority than a hidden
widget of lower depth.

Also make inactive widgets stop contributing priority to its
renderer process.

Bug:  813232 
Change-Id: Id9acefc40d6d2f2d94b5d62a592630037766d786
Reviewed-on: https://chromium-review.googlesource.com/934311
Commit-Queue: Bo <boliu@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546622}
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/child_process_launcher.cc
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/child_process_launcher.h
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/child_process_launcher_helper_android.cc
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/frame_host/frame_tree_node.cc
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/frame_host/frame_tree_node.h
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/renderer_host/render_view_host_impl.cc
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/renderer_host/render_view_host_impl.h
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/renderer_host/render_widget_host_owner_delegate.h
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/public/browser/render_process_host.h
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/a13e7c09e023e2c4448407f0a47251aa403326b5/content/shell/android/java/src/org/chromium/content_shell/ShellManager.java

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

commit e77291a73afbac87ad79ba74fa8aafb648a873f3
Author: Bo Liu <boliu@chromium.org>
Date: Mon Apr 30 17:10:35 2018

android: Remove initial binding

Use moderate binding instead which has the exact same flags, and also
supports refcounting.

Bug:  813232 
Change-Id: I22a56345a835c5916e6604bfb7cb2fc5cae6ceb5
Reviewed-on: https://chromium-review.googlesource.com/1033799
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554795}
[modify] https://crrev.com/e77291a73afbac87ad79ba74fa8aafb648a873f3/base/android/java/src/org/chromium/base/process_launcher/ChildProcessConnection.java
[modify] https://crrev.com/e77291a73afbac87ad79ba74fa8aafb648a873f3/base/android/junit/src/org/chromium/base/process_launcher/ChildProcessConnectionTest.java
[modify] https://crrev.com/e77291a73afbac87ad79ba74fa8aafb648a873f3/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java
[modify] https://crrev.com/e77291a73afbac87ad79ba74fa8aafb648a873f3/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherIntegrationTest.java
[modify] https://crrev.com/e77291a73afbac87ad79ba74fa8aafb648a873f3/content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java

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

commit f0ff535560326a04b0bd5315610eba3dd61b92af
Author: Bo Liu <boliu@chromium.org>
Date: Mon Apr 30 17:15:52 2018

android: Connection importance for OOPIF

The effective change is that foreground subframes get moderate
instead of important binding.

Note this is only half of the change that's in the chart at the bottom
of this design doc:
https://docs.google.com/document/d/1PTI3udZ6TI-R0MlSsl2aflxRcnZ5blnkGGHE5JRI_Z8/edit?usp=sharing

Background subframe renderer is not yet lowered to waived.

At the same time refactor ChildProcessLauncherHelper.setPriority to be
easier to read and understand. Compute the effective importance from all
available input, and then update bindings based on the effective
importance.

BindingManager.increaseRecency is now based on gaining effective
"important" importance, which is effectively the same as before for
chrome since ChildProcessImportance.IMPORTANT is not used by src/chrome.

Bug:  813232 
Change-Id: I8969f37edfec8b39b53d3a8ffe740585f3a802a6
Reviewed-on: https://chromium-review.googlesource.com/1033802
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554797}
[modify] https://crrev.com/f0ff535560326a04b0bd5315610eba3dd61b92af/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java
[modify] https://crrev.com/f0ff535560326a04b0bd5315610eba3dd61b92af/content/public/android/javatests/src/org/chromium/content/browser/webcontents/WebContentsTest.java

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

commit 16dd7b832cb36b6b89c074adb827fbacd3aecb83
Author: Bo Liu <boliu@chromium.org>
Date: Wed May 02 21:19:50 2018

android: WebContents.setImportance affect main frame

Rather than affecting all sub frames as well.
There should be no behavior change if OOPIF is disabled.

When OOPIF is enabled:
* on Chrome, marks the foreground tab MODERATE, so that it maintains the
  default binding when the app is in the background. This changes gets
  us the desired behavior where the main frame but not sub frames keeps
  MODERATE.
* Android WebView probably wants the ability to control subframes, but
  that's probably years away from ready, at which point child process
  importance probably needs to be revisited again. So essentially just
  ignoring it for now.

Bug:  813232 
Change-Id: I045dcbc607eb51a7e43c7b1455ff994298e20272
Reviewed-on: https://chromium-review.googlesource.com/1038759
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555546}
[modify] https://crrev.com/16dd7b832cb36b6b89c074adb827fbacd3aecb83/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/16dd7b832cb36b6b89c074adb827fbacd3aecb83/content/browser/web_contents/web_contents_android.cc
[modify] https://crrev.com/16dd7b832cb36b6b89c074adb827fbacd3aecb83/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/16dd7b832cb36b6b89c074adb827fbacd3aecb83/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/16dd7b832cb36b6b89c074adb827fbacd3aecb83/content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
[modify] https://crrev.com/16dd7b832cb36b6b89c074adb827fbacd3aecb83/content/public/android/java/src/org/chromium/content_public/browser/WebContents.java

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

commit 2f75db3919c9989a6a0ced03ff76d7fc47cabbfd
Author: Bo Liu <boliu@chromium.org>
Date: Thu May 03 00:56:21 2018

android: Add crash metrics for OOPIF

Add counts that uses whether a renderer is a subframe process or not.

Dependent changes:
* Rename RenderProcessHost::GetFrameDepthForTesting.
* Add explicit constructor and assignment operator for TerminationInfo.
  This means no longer using list initialization.

Bug:  813232 
Change-Id: I80de8004d344cf012bd906eb01c8a4c63c218bda
Reviewed-on: https://chromium-review.googlesource.com/1040675
Reviewed-by: Scott Graham <scottmg@chromium.org>
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555636}
[modify] https://crrev.com/2f75db3919c9989a6a0ced03ff76d7fc47cabbfd/components/crash/content/browser/crash_dump_manager_android.cc
[modify] https://crrev.com/2f75db3919c9989a6a0ced03ff76d7fc47cabbfd/components/crash/content/browser/crash_dump_manager_android_unittest.cc
[modify] https://crrev.com/2f75db3919c9989a6a0ced03ff76d7fc47cabbfd/components/crash/content/browser/crash_dump_observer_android.cc
[modify] https://crrev.com/2f75db3919c9989a6a0ced03ff76d7fc47cabbfd/components/crash/content/browser/crash_dump_observer_android.h
[modify] https://crrev.com/2f75db3919c9989a6a0ced03ff76d7fc47cabbfd/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/2f75db3919c9989a6a0ced03ff76d7fc47cabbfd/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/2f75db3919c9989a6a0ced03ff76d7fc47cabbfd/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/2f75db3919c9989a6a0ced03ff76d7fc47cabbfd/content/public/browser/render_process_host.h
[modify] https://crrev.com/2f75db3919c9989a6a0ced03ff76d7fc47cabbfd/content/public/test/mock_render_process_host.cc
[modify] https://crrev.com/2f75db3919c9989a6a0ced03ff76d7fc47cabbfd/content/public/test/mock_render_process_host.h
[modify] https://crrev.com/2f75db3919c9989a6a0ced03ff76d7fc47cabbfd/tools/metrics/histograms/enums.xml

 crbug.com/693484  is fixed, and this is missing one last piece to make moderate binding management explicitly rank renderers as well rather than the current imperfect LRU.

Binding management shouldn't have a huge effect on practical usage, so I'd say at least in terms of priority related things, we should be ok to start experimenting site isolation on android.

Ok, so binding management..

Current implementation uses an LRU queue for processes; processes in the queue has moderate binding added by binding management, and those not in the queue do not. "Use" is when a process gains important binding, which right now just means main frame becoming visible; subframes never enter binding management.

The queue is trimmed in LRU order by a constant ratio of its *current* length (ie moderate bindings are removed from processes) when system sends memory pressure signals. The exact constant depends on the pressure signal level, and I think were tuned experimentally?

The queue is emptied when the app is sent into the background, with a delay of 10 seconds. Which is umm... a bit surprising? I guess this is why previously using oom protection binding to approximate visible is actually not too terrible.


The problem with OOPIF is that the LRU queue order don't reflect order of how we'd like to rank the processes (I mean, subframes are never considered right now). Just sorting the queue by the same conditions we sort for intentional kill isn't good enough though, because that doesn't include things not in the queue right now (again subframes).

So I think I'd like to switch to using ChildProcessRanking. Keep an index m so that [0, m) are the processes that have moderate binding. When a process moves from index >= m to index < m, it gains moderate binding (and m increments). And when a process moves index < m to index >= m, m remains unchanged and the corresponding processes gains or loses moderate binding. Then m can shrink by the same constant ratios as before.

I wish it's cleaner, but that's the closest thing to emulating the existing behavior. I think means I have to write my own for loops in ChildProcessRanking..

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

commit 0f8ed56514969817982a21d92fc49a7bca4f44fe
Author: Bo Liu <boliu@chromium.org>
Date: Fri May 11 02:14:29 2018

android: Remove BindingManager interface

and use the implementation type directly.

Bug:  813232 
Change-Id: I53aaaf47a9f05a29930430c764bf87118c8c940e
Reviewed-on: https://chromium-review.googlesource.com/1054640
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557769}
[modify] https://crrev.com/0f8ed56514969817982a21d92fc49a7bca4f44fe/content/public/android/BUILD.gn
[modify] https://crrev.com/0f8ed56514969817982a21d92fc49a7bca4f44fe/content/public/android/java/src/org/chromium/content/browser/BindingManager.java
[delete] https://crrev.com/685e00f8c13b716e700db74476f2fea0d2c54b8d/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
[modify] https://crrev.com/0f8ed56514969817982a21d92fc49a7bca4f44fe/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java
[modify] https://crrev.com/0f8ed56514969817982a21d92fc49a7bca4f44fe/content/public/android/javatests/src/org/chromium/content/browser/OWNERS
[rename] https://crrev.com/0f8ed56514969817982a21d92fc49a7bca4f44fe/content/public/android/junit/src/org/chromium/content/browser/BindingManagerTest.java

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

commit aa3626ec27b96bd37dbeea91f350a60d5ba4fddc
Author: Bo Liu <boliu@chromium.org>
Date: Fri May 11 22:25:38 2018

android: Add crash metrics for OOPIF

Bug:  813232 
Change-Id: Ibdddfa25e97f0c5fbf5e737305d16f6dcd70ecd0
Reviewed-on: https://chromium-review.googlesource.com/1055801
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558041}
[modify] https://crrev.com/aa3626ec27b96bd37dbeea91f350a60d5ba4fddc/components/crash/content/browser/crash_dump_manager_android.cc
[modify] https://crrev.com/aa3626ec27b96bd37dbeea91f350a60d5ba4fddc/tools/metrics/histograms/enums.xml

Just checking: Does r557769 resolve the binding management concern in comments 31-32, or is there more to do before this bug is finished?  (Thanks for all the work here!)

There's one more: https://chromium-review.googlesource.com/1054642

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

commit 656774b92ced0be304393ed9560f946ddde34124
Author: Bo Liu <boliu@chromium.org>
Date: Mon May 21 21:55:10 2018

android: Child frame processes in binding management

Child frame process never becomes IMPORTANT even when visible. So change
the condition for adding to moderate binding for becoming visible
instead.

Note in this implementation, there is no guarantee that root frame is
ordered before sub frames in BindingManager.

Bug:  813232 
Change-Id: Ia8a7e76c557a59054194f921bfdf1b0cedf5e4be
Reviewed-on: https://chromium-review.googlesource.com/1067516
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560359}
[modify] https://crrev.com/656774b92ced0be304393ed9560f946ddde34124/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java

That's everything I intend to do for now. Probably will be more changes before site isolation ships, but those should be driven by metrics.

Found two bugs in this area, and filed  crbug.com/861826  for the fix.