Refactor child process launcher |
||||||
Issue descriptionI think this is really needed after reviewing and modifying this code a bit. Every change can potentially lead to hard to debug crashes that show up on UMA.. Goals: * make it more functional, so it's easier to test, then add moar tests * add more comments, clarify ownership and thread access * oneway aidl interfaces, right now all binder calls are synchronous, can be made async with "oneway" keyword * make CreationParams per-connection, which is required for webapk I'm sure no one will object to the goals, other than maybe tying CreationParams change to this, since webapk kinda needs it yesterday.. I can work on this as a low priority project when I don't have code reviews or release blockers. I do have a question as I read the existing code: Why do we need a SpawnQueue at all? Shouldn't client avoid creating launching more child than supported? That arbitrarily high latency in the spawn queue feels really scary to me..
,
Feb 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/22b31729529579fea43962f4bd0d20a760d027f5 commit 22b31729529579fea43962f4bd0d20a760d027f5 Author: boliu <boliu@chromium.org> Date: Tue Feb 14 02:18:13 2017 android: Move PendingSpawnQueue into ChildConnectionAllocator This simplifies locking between allocator and queue: {allocate or enqueue} and {free and dequeue} need to be atomic operations, and this was previously achieved by exposing the lock in PendingSpawnQueue outside of its class. See https://codereview.chromium.org/1725643003 By moving PendingSpawnQueue, can get rid of the lock in Queue entirely and just use the Allocator lock instead. Everything else is more or less fallout from this change: * Pass entire SpawnData to allocate call so it may be enqueued * Add isForWarmup to SpawnData; warmup allocations are never queued * Rename PendingSpawnData to SpawnData since it's no longer just for pending allocations * Add comments * clang-format made some questionable format choices BUG= 689758 Review-Url: https://codereview.chromium.org/2689673007 Cr-Commit-Position: refs/heads/master@{#450201} [modify] https://crrev.com/22b31729529579fea43962f4bd0d20a760d027f5/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
,
Feb 14 2017
We have 20 service slots, but RenderProcessHost::GetMaxRendererProcessCount returns infinity. I think the reasoning at the time was that 20 is way more than the number of renderers that devices can keep alive. That most certainly isn't the case anymore, with android devices with 6GB of ram around.. fwiw, we also have a spawn queue, which means the launch latency can grow unboundedly as client code actually requests more renderers. Yay
,
Feb 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/687c39deed13217382e70376304e664ea0d124b9 commit 687c39deed13217382e70376304e664ea0d124b9 Author: boliu <boliu@chromium.org> Date: Wed Feb 15 00:35:33 2017 android: Add test for child connection warmup BUG= 689758 Review-Url: https://codereview.chromium.org/2693893006 Cr-Commit-Position: refs/heads/master@{#450527} [modify] https://crrev.com/687c39deed13217382e70376304e664ea0d124b9/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
,
Feb 17 2017
Actually tried creating 20+ tabs on a pixel, which is the highest ram device I can get my hands on. pixel has enough memory to keep 19 renderers (with about:blank loaded) all running without issuing a single trim signal to the browser. This means all 19 renderers have moderate bindings. Then after allocating the last slot, code in allocateBoundConnection kicks in and drops *all* moderate bindings. This always leads to android killing 2-3 renderers for me. So I've never managed to have an allocation get queued up. This still makes me wonder if say 6GB devices can keep everything running without moderate bindings..
,
Feb 17 2017
Another question, in case anyone knows. Why is it safe to call Context.bindService on a background thread?
,
Feb 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3e7384263148e6719c894b1457ee1cf92c01ef21 commit 3e7384263148e6719c894b1457ee1cf92c01ef21 Author: boliu <boliu@chromium.org> Date: Fri Feb 17 21:23:01 2017 android: Remove command line from bind The command line argument is no longer sent in the intent. This is a tiny clean up following tohttps://codereview.chromium.org/2560403002 BUG= 689758 Review-Url: https://codereview.chromium.org/2701893002 Cr-Commit-Position: refs/heads/master@{#451383} [modify] https://crrev.com/3e7384263148e6719c894b1457ee1cf92c01ef21/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java [modify] https://crrev.com/3e7384263148e6719c894b1457ee1cf92c01ef21/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java [modify] https://crrev.com/3e7384263148e6719c894b1457ee1cf92c01ef21/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [modify] https://crrev.com/3e7384263148e6719c894b1457ee1cf92c01ef21/content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java
,
Mar 9 2017
,
Mar 14 2017
I want to kill background thread usage. There is code that uses AsyncTask, there's code that starts an one-off thread (!!!). These should just all be posts to the launcher thread. However since some of this code runs before native side is loaded, it's not safe to always reach the native message loop. I'm not sure if it's easier to change launcher thread to be backed by java looper, or plumb a signal that native side is loaded..
,
Mar 14 2017
,
Mar 14 2017
Agreed that it would be ideal to just use process launcher thread. :) jaekyun@ had a CL at one point to split things to just child process launcher thread and ui thread (I think - it certainly cleaned up some threading violations), but it was a bit messy and he was leaving the project so we didn't want to just dump it. There's probably a cleaner path but requires some thought as to which objects live on which thread. I'm referring to things like ModerateBindingManager and the like.
,
Mar 14 2017
how did he deal with warmup? all threading simplifications in my head gets messed up by warmup, which can't be on the UI thread, but may happen before native library is loaded
,
Mar 14 2017
asynctask: https://codereview.chromium.org/1307793003/ it wasn't the full clean-up you're looking to do and was focused primarily on eliminating existing races
,
Mar 14 2017
yaron, what's the outcome of ModerateBindingOnBackgroundTabCreation? the experiment has long expired, but the code to read the experiment is still there
,
Mar 15 2017
,
Mar 15 2017
Notes for myself for understanding logic in BindingManagerImpl. Read at your leisure.. A child connection is a set of bindings. Binding "importance" levels include: * strong, for foreground tabs, gpu * moderate, for recent back ground tabs, these bindings are dropped on memory pressure * waived, are always there so renderer don't immediately suicide, and does not impact android oom calculations. BindingManagerImpl doesn't care about this * initial, this is same level as strong, hopefully removed soon ( crbug.com/537671 ) Ignoring initial, a connection can be in 3 states: strong: has strong and waived binding moderate: has moderate and waived binding none: only has waived binding Connections does roughly form a "queue" in the order of binding strength, although only the "moderate" portion of the queue is kept in order in ModerateBindingPool. There are a bunch of what I would consider hacks for mIsLowMemoryDevice: * moderate state does not exist (ModerateBindingPool isn't even instantiated) * BindingManagerImpl enforces there's exactly one connection in strong state. Making a second connection strong will force drop the first one into none. * Don't delay dropping strong bindings. Otherwise 1s delay, I guess in case tab is switched back again. The whole thing is supposed to be thread safe, although there are 3 lock objects in the class, so checking that it's thread safe is not very trivial. Haven't found anything wrong yet though..
,
Mar 16 2017
Thinking of refactoring BindingManagerImpl like this, to support maintaining connections in LRU order for issue 693484 . maintain separate queues (probably ArrayList) of connections for the strong, moderate, etc; together they are in LRU order. Bindings change when connection move between queues, or they change for an entire queue; so invariant is all connections in a queue have the same bindings. For thread safety, I think I'll just use one giant lock, but avoid holding that lock when calling into things outside of BindingManager (ie Connection methods) This all kinda makes sense in my head. I don't suppose anyone still understands this enough to have an opinion. So I think I'll try it..
,
Mar 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c3458a35d1cbf723a6ddc4a75cce9148eeafed82 commit c3458a35d1cbf723a6ddc4a75cce9148eeafed82 Author: boliu <boliu@chromium.org> Date: Thu Mar 16 06:20:09 2017 android: AtomicReference over ModerateBindingPoolLock Does the exact same thing, but imo more readable. BUG= 689758 Review-Url: https://codereview.chromium.org/2750123002 Cr-Commit-Position: refs/heads/master@{#457364} [modify] https://crrev.com/c3458a35d1cbf723a6ddc4a75cce9148eeafed82/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
,
Mar 16 2017
#16 sounds pretty accurate. Re: "* Don't delay dropping strong bindings. Otherwise 1s delay, I guess in case tab is switched back again." That's exactly right. THere's also the concern that you open a tab which then dismisses itself (e.g. download/pdf) and don't want to lose the tab. On low-end we can't risk the ram though. Re: #17 - don't remember the edge cases well enough to confidently comment on them. I would hope that binding no longer happens on UI thread though
,
Mar 16 2017
yaron: there's also that question in #14 about outcome of ModerateBindingOnBackgroundTabCreation? peter directed me to you :p
,
Mar 16 2017
I thought the actual change launched - it got green light
,
Mar 16 2017
The code is still checking for the ModerateBindingOnBackgroundTabCreation experiment, and I can't find any active experiment that enables it on stable; also the dev and beta experiments have expired, which I assume means it'll default to false. This is my first experience with experiments though, so could have got things wrong.. So I can remove the experiment, and default it true/on now?
,
Mar 17 2017
Ha, nothing ever gets removed from "SparseArray<ManagedConnection> mManagedConnections", so just leaks and leaks. The actual connections are cleared, so it's just the empty ManagedConnection objects that are leaked. But still..
,
Mar 20 2017
#22: sgtm #23: sigh
,
Mar 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c7a0947f9803da749b8f498252a5118e9b08d9ad commit c7a0947f9803da749b8f498252a5118e9b08d9ad Author: boliu <boliu@chromium.org> Date: Mon Mar 20 16:59:35 2017 Remove ModerateBindingOnBackgroundTabCreation Unconditionally enable code behind this experiment, and remove the experiment code. BUG= 689758 , 595175 Review-Url: https://codereview.chromium.org/2753713002 Cr-Commit-Position: refs/heads/master@{#458099} [modify] https://crrev.com/c7a0947f9803da749b8f498252a5118e9b08d9ad/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java [modify] https://crrev.com/c7a0947f9803da749b8f498252a5118e9b08d9ad/chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java [modify] https://crrev.com/c7a0947f9803da749b8f498252a5118e9b08d9ad/chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java [modify] https://crrev.com/c7a0947f9803da749b8f498252a5118e9b08d9ad/content/public/android/java/src/org/chromium/content/browser/BindingManager.java [modify] https://crrev.com/c7a0947f9803da749b8f498252a5118e9b08d9ad/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java [modify] https://crrev.com/c7a0947f9803da749b8f498252a5118e9b08d9ad/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [modify] https://crrev.com/c7a0947f9803da749b8f498252a5118e9b08d9ad/content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java [modify] https://crrev.com/c7a0947f9803da749b8f498252a5118e9b08d9ad/testing/variations/fieldtrial_testing_config.json
,
Mar 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4ac59a56d54826dbdfc5b17b2e8875863065b0b8 commit 4ac59a56d54826dbdfc5b17b2e8875863065b0b8 Author: boliu <boliu@chromium.org> Date: Mon Mar 20 17:45:32 2017 android: Move launcher_android.cc to helper The ultimate goal is to fix leaking mManagedConnections in BindingManagerImpl, which is leaking to support getting isOomProtected after a service has been disconnected. That requires saving that status in native when service is disconnected, which probably belongs on LauncherHelper. That implies Helper should talk directly java, and this is the first step. This is meant to to be a straight-forward move with no changes, even if the existing code makes no sense once they live in the same file. Order of function is rearranged a bit though. BUG= 689758 Review-Url: https://codereview.chromium.org/2753383002 Cr-Commit-Position: refs/heads/master@{#458114} [modify] https://crrev.com/4ac59a56d54826dbdfc5b17b2e8875863065b0b8/content/browser/BUILD.gn [modify] https://crrev.com/4ac59a56d54826dbdfc5b17b2e8875863065b0b8/content/browser/android/browser_jni_registrar.cc [delete] https://crrev.com/4832979ab80769a47bcd70b3a5510d9b523f2b06/content/browser/android/child_process_launcher_android.cc [delete] https://crrev.com/4832979ab80769a47bcd70b3a5510d9b523f2b06/content/browser/android/child_process_launcher_android.h [modify] https://crrev.com/4ac59a56d54826dbdfc5b17b2e8875863065b0b8/content/browser/child_process_launcher_helper_android.cc [add] https://crrev.com/4ac59a56d54826dbdfc5b17b2e8875863065b0b8/content/browser/child_process_launcher_helper_android.h
,
Mar 23 2017
So I'm trying to fix the ManagedConnection leak. Going to write the explanation reason for needing "hold a lock and call into outside code". This is a semi-rant, as well as motivation for fixing crbug.com/701442 , ie exposing launcher thread looper to java. * ManagedConnection are leaked in order to save the isOomProtected state, which is super important for crash reporting * The ownership/lifetime relationship between native ChildProcessLauncher(Helper) and java ChildProcessLauncher is complex. Java ChildProcessLauncher and native only calls into static methods. Ownership happens with java holding a opaque native pointer, that native then deletes in a callback. Calls can happen on different threads * To fix the leak, the solution is to save the isOomProtected state in native after the connection is disconnected. This needs another callback into native, let's call it "close", in addition to "onConnect". The native object will have to live through both callbacks, and they happen on different treads. It's kinda fun trying to explore what options there are to correctly synchronize this so that there are no leaks in this native object, and also no dangling pointers. My solution: * native side is RefCountedThreadSafe, and java keeps a reference * native code is all posted to launcher thread, so only refcounting need to be thread safe * java need to guarantee the order between callbacks, ie close is always the last thing being called, so native can be deleted in that call. This turns out to require holding a lock in java while calling these callbacks, so that the posted tasks in native is guaranteed a certain order; AtomicReference is not good enough. This would all be easier if * Launcher thread is exposed to java, then can post in java code instead, which is crbug.com/701442 * There's saner (ie more common pattern of) ownership/lifetime between native and java here. Guess one more thing to refactor..
,
Mar 23 2017
> Java ChildProcessLauncher and native only calls into static methods Java ChildProcessLauncher *is not instantiated* and native only calls into static methods
,
Mar 23 2017
And continuing the conversation to myself.. why don't we return the isOomProtected state in the stop call? because crashes don't involve stop. Also turns out native ChildProcessLauncher is making this hard too, because it asks for termination status synchronously on the client thread (UI for renderer, IO for gpu). So caching things on launcher thread will inherently be racy. So need more complexity to deal with that. Yay...
,
Mar 23 2017
I'm beginning to think this callback to native is the wrong way to deal with java-native lifetime. Should just add a java object *instance* that has the same lifetime as native ChildProcessLauncher, and cache the isOomProtect state there. I guess that's somewhat obvious with benefit of hindsight. meh :/
,
Mar 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fcf25e5abf9f679d8717049e2d48f6d38bdd24ae commit fcf25e5abf9f679d8717049e2d48f6d38bdd24ae Author: boliu <boliu@chromium.org> Date: Thu Mar 23 23:52:27 2017 android: Remove nativeIsSingleProcess It's only used to guard a warning log message, which shouldn't be logged in the first place. BUG= 689758 Review-Url: https://codereview.chromium.org/2769313002 Cr-Commit-Position: refs/heads/master@{#459284} [modify] https://crrev.com/fcf25e5abf9f679d8717049e2d48f6d38bdd24ae/content/browser/child_process_launcher_helper_android.cc [modify] https://crrev.com/fcf25e5abf9f679d8717049e2d48f6d38bdd24ae/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
,
Mar 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d604de96c960021b33f929e9cdf9eca8079ad92b commit d604de96c960021b33f929e9cdf9eca8079ad92b Author: boliu <boliu@chromium.org> Date: Fri Mar 24 00:49:46 2017 android: ChildProcessLauncherAndroid follow up cleanups This is a folow up for to https://chromium.googlesource.com/chromium/src/+/4ac59a56d54826dbdfc5b17b2e8875863065b0b8 Move jni namespace to content::internal to match native code. Inline some Java_ChildProcessLauncer_* calls that has only a single caller. BUG= 689758 Review-Url: https://codereview.chromium.org/2772793003 Cr-Commit-Position: refs/heads/master@{#459307} [modify] https://crrev.com/d604de96c960021b33f929e9cdf9eca8079ad92b/content/browser/child_process_launcher_helper_android.cc [modify] https://crrev.com/d604de96c960021b33f929e9cdf9eca8079ad92b/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
,
Mar 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4ff655334a321362a3611d0e1d0f1c1c123dac08 commit 4ff655334a321362a3611d0e1d0f1c1c123dac08 Author: boliu <boliu@chromium.org> Date: Mon Mar 27 22:49:27 2017 android: Java ChildProcessLauncherHelper instance This is mostly meant to be a no-op refactor. There is a long term need to have ChildProcessLauncher(Helper) to have a java _instance_ to save state, as an alternative to putting things into global maps keyed by the pid. See bug for specific use. So create a java ChildProcessLauncherHelper, which is instantiated and owned by native ChildProcessLauncherHelper. Then move all jni calls from ChildProcessLauncher (which only had static methods) to ChildProcessLauncherHelper. Also convert the jni calls from static calls to instance calls where possible; the major except right is Terminate/Stop, because it's used as a static method. Replace the opaque "clientContext" native pointer in ChildProcessLauncher with a java callback implemented by Helper. Helper should hold a pointer directly to native peer, but due to threading/ownership issues, it is set back to 0 before native side is actually deleted. BUG= 689758 Review-Url: https://codereview.chromium.org/2774163002 Cr-Commit-Position: refs/heads/master@{#459916} [modify] https://crrev.com/4ff655334a321362a3611d0e1d0f1c1c123dac08/android_webview/java/src/org/chromium/android_webview/AwRendererPriorityManager.java [modify] https://crrev.com/4ff655334a321362a3611d0e1d0f1c1c123dac08/content/browser/child_process_launcher.cc [modify] https://crrev.com/4ff655334a321362a3611d0e1d0f1c1c123dac08/content/browser/child_process_launcher_helper.h [modify] https://crrev.com/4ff655334a321362a3611d0e1d0f1c1c123dac08/content/browser/child_process_launcher_helper_android.cc [modify] https://crrev.com/4ff655334a321362a3611d0e1d0f1c1c123dac08/content/browser/child_process_launcher_helper_linux.cc [modify] https://crrev.com/4ff655334a321362a3611d0e1d0f1c1c123dac08/content/browser/child_process_launcher_helper_mac.cc [modify] https://crrev.com/4ff655334a321362a3611d0e1d0f1c1c123dac08/content/browser/child_process_launcher_helper_win.cc [modify] https://crrev.com/4ff655334a321362a3611d0e1d0f1c1c123dac08/content/public/android/BUILD.gn [modify] https://crrev.com/4ff655334a321362a3611d0e1d0f1c1c123dac08/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [add] https://crrev.com/4ff655334a321362a3611d0e1d0f1c1c123dac08/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java [modify] https://crrev.com/4ff655334a321362a3611d0e1d0f1c1c123dac08/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
,
Apr 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2b2e80acce1c253615137ccdd101e6c390458d23 commit 2b2e80acce1c253615137ccdd101e6c390458d23 Author: boliu <boliu@chromium.org> Date: Mon Apr 03 21:00:39 2017 base: CommandLineFlags.Add need to parse out value This is probably source of flakes where tests use @CommandLineFlags.Add("foo=bar"). Before this CL, the entire "foo=bar" string is used as the switch, and a value of null, but only if the java-only CommandLine instance is in use. So if any test tries to read the switch during start up, it's racy. Fix by parsing the command line in CommandLineFlags. BUG= 689758 Review-Url: https://codereview.chromium.org/2793903003 Cr-Commit-Position: refs/heads/master@{#461534} [modify] https://crrev.com/2b2e80acce1c253615137ccdd101e6c390458d23/base/test/android/javatests/src/org/chromium/base/test/util/CommandLineFlags.java
,
Apr 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/52a60f52642d164a8f83cdf2a8c37beb8e26c898 commit 52a60f52642d164a8f83cdf2a8c37beb8e26c898 Author: boliu <boliu@chromium.org> Date: Wed Apr 05 18:45:54 2017 android: Post warmup to launcher thread Then simplify all callsites to just call warmup on UI thread. Test got a bit more complicated with the new restriction that warmUp must be called on the UI thread. BUG= 689758 Review-Url: https://codereview.chromium.org/2792873003 Cr-Commit-Position: refs/heads/master@{#462158} [modify] https://crrev.com/52a60f52642d164a8f83cdf2a8c37beb8e26c898/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java [modify] https://crrev.com/52a60f52642d164a8f83cdf2a8c37beb8e26c898/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java [modify] https://crrev.com/52a60f52642d164a8f83cdf2a8c37beb8e26c898/chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java [modify] https://crrev.com/52a60f52642d164a8f83cdf2a8c37beb8e26c898/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [modify] https://crrev.com/52a60f52642d164a8f83cdf2a8c37beb8e26c898/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
,
Apr 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/846fef8c771a4987220e3572c984793cb6162d7e commit 846fef8c771a4987220e3572c984793cb6162d7e Author: boliu <boliu@chromium.org> Date: Fri Apr 07 19:20:31 2017 android: Mark two binder calls oneway These are the only two (important) calls that do not have a return value, so can be made oneway. "oneway" is roughly equivalent to being asynchronous, since binder calls are synchronous by default. This is like an early test to make sure oneway doesn't cause issues. BUG= 689758 Review-Url: https://codereview.chromium.org/2799683007 Cr-Commit-Position: refs/heads/master@{#462955} [modify] https://crrev.com/846fef8c771a4987220e3572c984793cb6162d7e/base/android/java/src/org/chromium/base/process_launcher/IChildProcessService.aidl [modify] https://crrev.com/846fef8c771a4987220e3572c984793cb6162d7e/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java [modify] https://crrev.com/846fef8c771a4987220e3572c984793cb6162d7e/content/public/android/java/src/org/chromium/content/common/IGpuProcessCallback.aidl [modify] https://crrev.com/846fef8c771a4987220e3572c984793cb6162d7e/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
,
Apr 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/39c7bd71dc2f9c9f692dd346e4e4e525af25ede1 commit 39c7bd71dc2f9c9f692dd346e4e4e525af25ede1 Author: boliu <boliu@chromium.org> Date: Mon Apr 10 18:23:03 2017 android: Async setupConnection binder call Make setupConnection oneway/async. Add a ICallbackInt interface to return the PID of the child process. The reply is then posted back to launcher thread. Connection setup is already mostly async so only involves moving related code to the reply callback. One caveat is that getCallingPid no longer works and just returns 0 for oneway calls. The pid check is not strictly necessary, so dropped the from setupConnection instead. BUG= 689758 , 690118 Review-Url: https://codereview.chromium.org/2810433002 Cr-Commit-Position: refs/heads/master@{#463332} [modify] https://crrev.com/39c7bd71dc2f9c9f692dd346e4e4e525af25ede1/base/BUILD.gn [add] https://crrev.com/39c7bd71dc2f9c9f692dd346e4e4e525af25ede1/base/android/java/src/org/chromium/base/process_launcher/ICallbackInt.aidl [modify] https://crrev.com/39c7bd71dc2f9c9f692dd346e4e4e525af25ede1/base/android/java/src/org/chromium/base/process_launcher/IChildProcessService.aidl [modify] https://crrev.com/39c7bd71dc2f9c9f692dd346e4e4e525af25ede1/content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java [modify] https://crrev.com/39c7bd71dc2f9c9f692dd346e4e4e525af25ede1/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java
,
Apr 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2cc9e495637d506c65ae9802408043f7fbaf27d2 commit 2cc9e495637d506c65ae9802408043f7fbaf27d2 Author: rouslan <rouslan@chromium.org> Date: Mon Apr 10 20:52:34 2017 Revert of android: Async setupConnection binder call (patchset #4 id:60001 of https://codereview.chromium.org/2810433002/ ) Reason for revert: Appears to have broken ChildProcessLauncherTest_testBindServiceFromMultipleProcesses/0 on https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%29/builds/41605 junit.framework.AssertionFailedError at org.chromium.content.browser.ChildProcessLauncherTest.testBindServiceFromMultipleProcesses(ChildProcessLauncherTest.java:448) at java.lang.reflect.Method.invokeNative(Native Method) at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:129) at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) Original issue's description: > android: Async setupConnection binder call > > Make setupConnection oneway/async. Add a ICallbackInt interface to > return the PID of the child process. The reply is then posted back to > launcher thread. Connection setup is already mostly async so only > involves moving related code to the reply callback. > > One caveat is that getCallingPid no longer works and just returns 0 > for oneway calls. The pid check is not strictly necessary, so dropped > the from setupConnection instead. > > BUG= 689758 , 690118 > > Review-Url: https://codereview.chromium.org/2810433002 > Cr-Commit-Position: refs/heads/master@{#463332} > Committed: https://chromium.googlesource.com/chromium/src/+/39c7bd71dc2f9c9f692dd346e4e4e525af25ede1 TBR=rsesek@chromium.org,jcivelli@chromium.org,torne@chromium.org,boliu@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 689758 , 690118 Review-Url: https://codereview.chromium.org/2806343003 Cr-Commit-Position: refs/heads/master@{#463387} [modify] https://crrev.com/2cc9e495637d506c65ae9802408043f7fbaf27d2/base/BUILD.gn [delete] https://crrev.com/b87f01f54ba47ace1ddf35476a9166ef67341ea8/base/android/java/src/org/chromium/base/process_launcher/ICallbackInt.aidl [modify] https://crrev.com/2cc9e495637d506c65ae9802408043f7fbaf27d2/base/android/java/src/org/chromium/base/process_launcher/IChildProcessService.aidl [modify] https://crrev.com/2cc9e495637d506c65ae9802408043f7fbaf27d2/content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java [modify] https://crrev.com/2cc9e495637d506c65ae9802408043f7fbaf27d2/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java
,
Apr 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/03ee275e219a1dd9ab281452507aeb7357c6f4c7 commit 03ee275e219a1dd9ab281452507aeb7357c6f4c7 Author: boliu <boliu@chromium.org> Date: Mon Apr 10 22:21:32 2017 android: Async setupConnection binder call Make setupConnection oneway/async. Add a ICallbackInt interface to return the PID of the child process. The reply is then posted back to launcher thread. Connection setup is already mostly async so only involves moving related code to the reply callback. One caveat is that getCallingPid no longer works and just returns 0 for oneway calls. The pid check is not strictly necessary, so dropped the from setupConnection instead. BUG= 689758 , 690118 Review-Url: https://codereview.chromium.org/2810433002 Cr-Commit-Position: refs/heads/master@{#463332} Committed: https://chromium.googlesource.com/chromium/src/+/39c7bd71dc2f9c9f692dd346e4e4e525af25ede1 Reland contains fix for flaky test. The pid is received asynchronously now, so wait for it after allocating a connection. Review-Url: https://codereview.chromium.org/2810433002 Cr-Commit-Position: refs/heads/master@{#463419} [modify] https://crrev.com/03ee275e219a1dd9ab281452507aeb7357c6f4c7/base/BUILD.gn [add] https://crrev.com/03ee275e219a1dd9ab281452507aeb7357c6f4c7/base/android/java/src/org/chromium/base/process_launcher/ICallbackInt.aidl [modify] https://crrev.com/03ee275e219a1dd9ab281452507aeb7357c6f4c7/base/android/java/src/org/chromium/base/process_launcher/IChildProcessService.aidl [modify] https://crrev.com/03ee275e219a1dd9ab281452507aeb7357c6f4c7/content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java [modify] https://crrev.com/03ee275e219a1dd9ab281452507aeb7357c6f4c7/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java [modify] https://crrev.com/03ee275e219a1dd9ab281452507aeb7357c6f4c7/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
,
Apr 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/06114d78b56a1f73eeede3514c06bcb0aa33a942 commit 06114d78b56a1f73eeede3514c06bcb0aa33a942 Author: boliu <boliu@chromium.org> Date: Thu Apr 13 19:21:34 2017 android: assert runningOnLauncherThread This change is meant to have no production impact. Add assert to methods that already only running on launcher thread in production. This involves a lot of changes in tests though since test often assume things are thread safe. This includes moving some helper methods from ChildProcessLauncherTest to ChildProcessLauncherTestHelperService so they can be shared. Also take this opportunity to move a bunch of ForTesting methods from ChildProcessLauncher to actual test file. This does involve exposing other things and marking them as @VisibleForTesting, but imo removing stuff from ChildProcessLauncher is a good thing. BUG= 689758 Review-Url: https://codereview.chromium.org/2809293005 Cr-Commit-Position: refs/heads/master@{#464498} [modify] https://crrev.com/06114d78b56a1f73eeede3514c06bcb0aa33a942/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [modify] https://crrev.com/06114d78b56a1f73eeede3514c06bcb0aa33a942/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java [modify] https://crrev.com/06114d78b56a1f73eeede3514c06bcb0aa33a942/content/public/android/java/src/org/chromium/content/browser/LauncherThread.java [modify] https://crrev.com/06114d78b56a1f73eeede3514c06bcb0aa33a942/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java [modify] https://crrev.com/06114d78b56a1f73eeede3514c06bcb0aa33a942/content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestHelperService.java
,
Apr 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/da4f4f98353bc5fd8f88300721260355bb994b3b commit da4f4f98353bc5fd8f88300721260355bb994b3b Author: boliu <boliu@chromium.org> Date: Tue Apr 18 04:40:21 2017 android: Post stop/onServiceDisconnected to launcher From native, stop used to be called on both the client (UI/IO) thread and on launcher thread, depending only which ChildProcessLauncher method client calls. Now post both to launcher thread. onServiceDisconnected to launcher thread as well. onServiceDisconnected also calls stop, so these two changes are bundled together. This allows all synchronization to be removed from Allocator since everything is accessed on the Launcher thread only. But once again, tests needed to be fixed here as well. Note onServiceConnected is unchanged because it has some interaction with the warm up connection. Potential risks: I think delaying native calls are fine. stop was always asynchronous anyway. There is a bit of a risk delaying onServiceDisconnected because dropping bindings will prevent android from restarting the service on our behalf. I think we just have to swallow this latency and one day unify everything to run on the launcher thread. Of course there could be other unforeseen issues. BUG= 689758 Review-Url: https://codereview.chromium.org/2821583002 Cr-Commit-Position: refs/heads/master@{#465139} [modify] https://crrev.com/da4f4f98353bc5fd8f88300721260355bb994b3b/content/browser/child_process_launcher_helper_android.cc [modify] https://crrev.com/da4f4f98353bc5fd8f88300721260355bb994b3b/content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java [modify] https://crrev.com/da4f4f98353bc5fd8f88300721260355bb994b3b/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java [modify] https://crrev.com/da4f4f98353bc5fd8f88300721260355bb994b3b/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java [modify] https://crrev.com/da4f4f98353bc5fd8f88300721260355bb994b3b/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [modify] https://crrev.com/da4f4f98353bc5fd8f88300721260355bb994b3b/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java [modify] https://crrev.com/da4f4f98353bc5fd8f88300721260355bb994b3b/content/public/android/java/src/org/chromium/content/browser/LauncherThread.java [modify] https://crrev.com/da4f4f98353bc5fd8f88300721260355bb994b3b/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
,
Apr 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/223e40da117757e1ddc9d4e5eb71d563f50254c9 commit 223e40da117757e1ddc9d4e5eb71d563f50254c9 Author: boliu <boliu@chromium.org> Date: Tue Apr 18 06:25:01 2017 android: Post onServiceConnected to launcher thread This involves refactoring warm up connection. Previously, start used to wait on launcher thread for the warm up connection to finish StartCallbacks, which happens in onServiceConnected on the UI thread. This isn't possible anymore if onServiceConnected is posted to launcher thread. Upside is everything is on launcher thread now, so it's a "simple" matter of keeping more state. Add |sSpareConnectionStartCallback| to save the StartCallback of an allocation if StartCallback of the warm up has not finished StartCallback yet. Note it's not ok to skip the warm up connection when it is not ready, because that defeats the point warm up and also will allow webview to launch more than one service. This change allows removal of sSpareConnectionLock. BUG= 689758 Review-Url: https://codereview.chromium.org/2822803002 Cr-Commit-Position: refs/heads/master@{#465155} [modify] https://crrev.com/223e40da117757e1ddc9d4e5eb71d563f50254c9/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnectionImpl.java [modify] https://crrev.com/223e40da117757e1ddc9d4e5eb71d563f50254c9/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
,
Apr 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/faaae321d190728d93ff5b51c961c3d28c016ae5 commit faaae321d190728d93ff5b51c961c3d28c016ae5 Author: jcivelli <jcivelli@chromium.org> Date: Tue Apr 25 23:55:48 2017 Refactoring ChildProcessConnection. Splitting the ChildProcessConnectionImpl class in 2 and changing it to an abstract class instead of an interface. ChildProcessConnections can now be either: - ImportantChildProcessConnection: bound BIND_IMPORTANT and used for the GPU process. This is equivalent to the previous alwaysInForeground ChildProcessConnectionImpl. - ManagedChildProcessConnection: managed by the BindingManager. With that split, only ManagedChildProcessConnection are now passed to the BindingManager. Also changing the ChildProcessLauncherHelper to keep a reference to the ChildProcessConnection so that it can determine if it is OOM protected without the help of the BindingManager. As a result, the BindingManager does not keep ChildProcessConnections around when they are cleared. Also changed BindingManagerImplTest to exercise the actual connection code (instead of mocking some of the logic in the test). BUG= 689758 Review-Url: https://codereview.chromium.org/2828793002 Cr-Commit-Position: refs/heads/master@{#467166} [modify] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java [modify] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/build/android/lint/suppressions.xml [modify] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java [modify] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/content/public/android/BUILD.gn [rename] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java [modify] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/content/public/android/java/src/org/chromium/content/browser/BindingManager.java [modify] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java [modify] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java [delete] https://crrev.com/212d4283c7de2e8120cdf8c986dda3fcade13048/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java [modify] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [modify] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java [add] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/content/public/android/java/src/org/chromium/content/browser/ImportantChildProcessConnection.java [modify] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/content/public/android/java/src/org/chromium/content/browser/LauncherThread.java [add] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java [modify] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java [modify] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java [modify] https://crrev.com/faaae321d190728d93ff5b51c961c3d28c016ae5/content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestHelperService.java
,
Apr 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/213830d7d68eefddedc6199bf699dfac3d5481e8 commit 213830d7d68eefddedc6199bf699dfac3d5481e8 Author: boliu <boliu@chromium.org> Date: Wed Apr 26 21:26:20 2017 Remove post_launch_on_client_thread_called This is a follow up to refs/heads/master@{#465155}. Android now always calls OnChildProcessStarted on the launcher thread, which means the complexity to deal with the call directly happening on the client thread can be removed. BUG= 689758 Review-Url: https://codereview.chromium.org/2843063002 Cr-Commit-Position: refs/heads/master@{#467463} [modify] https://crrev.com/213830d7d68eefddedc6199bf699dfac3d5481e8/content/browser/child_process_launcher_helper.cc [modify] https://crrev.com/213830d7d68eefddedc6199bf699dfac3d5481e8/content/browser/child_process_launcher_helper.h [modify] https://crrev.com/213830d7d68eefddedc6199bf699dfac3d5481e8/content/browser/child_process_launcher_helper_android.cc
,
Apr 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2582adf8aedbc7c9538aec544f80bf6379b11cc5 commit 2582adf8aedbc7c9538aec544f80bf6379b11cc5 Author: sashab <sashab@chromium.org> Date: Wed Apr 26 23:18:31 2017 Revert of Remove post_launch_on_client_thread_called (patchset #2 id:20001 of https://codereview.chromium.org/2843063002/ ) Reason for revert: Caused failure on WebKit Win10 builder: webkit_python_tests webkit_python_tests webkitpy/layout_tests/run_webkit_tests_unittest/RunTest/test_child_processes_2 https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win10/builds/21699 Original issue's description: > Remove post_launch_on_client_thread_called > > This is a follow up to refs/heads/master@{#465155}. Android now always > calls OnChildProcessStarted on the launcher thread, which means the > complexity to deal with the call directly happening on the client thread > can be removed. > > BUG= 689758 > > Review-Url: https://codereview.chromium.org/2843063002 > Cr-Commit-Position: refs/heads/master@{#467463} > Committed: https://chromium.googlesource.com/chromium/src/+/213830d7d68eefddedc6199bf699dfac3d5481e8 TBR=jcivelli@chromium.org,boliu@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 689758 Review-Url: https://codereview.chromium.org/2839353002 Cr-Commit-Position: refs/heads/master@{#467502} [modify] https://crrev.com/2582adf8aedbc7c9538aec544f80bf6379b11cc5/content/browser/child_process_launcher_helper.cc [modify] https://crrev.com/2582adf8aedbc7c9538aec544f80bf6379b11cc5/content/browser/child_process_launcher_helper.h [modify] https://crrev.com/2582adf8aedbc7c9538aec544f80bf6379b11cc5/content/browser/child_process_launcher_helper_android.cc
,
Apr 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5674fee66841f27be43267127bd8586b89042b82 commit 5674fee66841f27be43267127bd8586b89042b82 Author: boliu <boliu@chromium.org> Date: Wed Apr 26 23:41:59 2017 Remove post_launch_on_client_thread_called This is a follow up to refs/heads/master@{#465155}. Android now always calls OnChildProcessStarted on the launcher thread, which means the complexity to deal with the call directly happening on the client thread can be removed. BUG= 689758 Review-Url: https://codereview.chromium.org/2843063002 Cr-Original-Commit-Position: refs/heads/master@{#467463} Committed: https://chromium.googlesource.com/chromium/src/+/213830d7d68eefddedc6199bf699dfac3d5481e8 Review-Url: https://codereview.chromium.org/2843063002 Cr-Commit-Position: refs/heads/master@{#467514} [modify] https://crrev.com/5674fee66841f27be43267127bd8586b89042b82/content/browser/child_process_launcher_helper.cc [modify] https://crrev.com/5674fee66841f27be43267127bd8586b89042b82/content/browser/child_process_launcher_helper.h [modify] https://crrev.com/5674fee66841f27be43267127bd8586b89042b82/content/browser/child_process_launcher_helper_android.cc
,
Apr 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/43d9f387a0febd1c7b5ea74b121a6e485638cbdf commit 43d9f387a0febd1c7b5ea74b121a6e485638cbdf Author: boliu <boliu@chromium.org> Date: Fri Apr 28 23:11:24 2017 android: Remove sBindingManagerLock Everything calling getBindingManager is already on launcher thread. This removes the last lock in android's ChildProcessLauncher. BUG= 689758 Review-Url: https://codereview.chromium.org/2849943002 Cr-Commit-Position: refs/heads/master@{#468173} [modify] https://crrev.com/43d9f387a0febd1c7b5ea74b121a6e485638cbdf/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
,
Apr 28 2017
all da locks are gone \o/
,
Apr 28 2017
🙌
,
May 3 2017
griping about BindingManagerImpl.. oh so many problems * there are cases where the transition from important to moderate that drops important binding first, and the add the moderate binding, so it's momentarily dropped down to waived * LruCache uses the service number as key. But those can be duplicated with webapks (if it uses its own declares services), so we'd have key collisions * no one ever thought about how utility process should work when it was added * no one really thought about how webapk services should work (if it uses its own services), since pool right now has a fixed size of 20, based on number of declared sandboxed services in chrome
,
May 4 2017
Yikes! I'll admit that WebApk stuff was pretty hack and slash. We did it for the prototype (which I'll argue was fine for that purpose) but failed to do it properly when integrating into chromium. I'll take the blame - sorry :( Thank you so much for your help on this!
,
May 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/643ad22bbc6337e430f0108db6f4677499d897e8 commit 643ad22bbc6337e430f0108db6f4677499d897e8 Author: jcivelli <jcivelli@chromium.org> Date: Thu May 04 20:33:59 2017 Removed the service number member from BaseChildProcessConnection. In preparation for making the ChildProcessConnection simpler, removing the service number member from BaseChildProcessConnection. It is pertaining to the allocator, not the connection. Also replaced the LruCache used for the moderate binding pool in BindingManagerImpl (which was relying on the service number from the connection) with a LinkedList. It makes the code clearer since we were only using the LruCache to evict old connections, not the cache part. BUG= 689758 Review-Url: https://codereview.chromium.org/2855323003 Cr-Commit-Position: refs/heads/master@{#469452} [modify] https://crrev.com/643ad22bbc6337e430f0108db6f4677499d897e8/content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java [modify] https://crrev.com/643ad22bbc6337e430f0108db6f4677499d897e8/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java [modify] https://crrev.com/643ad22bbc6337e430f0108db6f4677499d897e8/content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java [modify] https://crrev.com/643ad22bbc6337e430f0108db6f4677499d897e8/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [modify] https://crrev.com/643ad22bbc6337e430f0108db6f4677499d897e8/content/public/android/java/src/org/chromium/content/browser/ImportantChildProcessConnection.java [modify] https://crrev.com/643ad22bbc6337e430f0108db6f4677499d897e8/content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java [modify] https://crrev.com/643ad22bbc6337e430f0108db6f4677499d897e8/content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java [modify] https://crrev.com/643ad22bbc6337e430f0108db6f4677499d897e8/content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestUtils.java
,
May 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3caa6e14cb856d09bc62c2bbbcae32e2aeadf95b commit 3caa6e14cb856d09bc62c2bbbcae32e2aeadf95b Author: jcivelli <jcivelli@chromium.org> Date: Fri May 05 22:53:43 2017 Clean-up in BindingmanagerImpl. Now that BindingManagerImpl is running on 1 thread, we can remove ManagedConnection.cleanUp() and some null check over connections. Also ensuring we don't hold on to connections (in the moderate pool or other member variables) when removed, so that we don't modify their bindings from that point. Also some minor clean-ups and renaming of variables. BUG= 689758 Review-Url: https://codereview.chromium.org/2860283002 Cr-Commit-Position: refs/heads/master@{#469788} [modify] https://crrev.com/3caa6e14cb856d09bc62c2bbbcae32e2aeadf95b/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
,
May 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/56de014ae2e5f80d5a009401d8d513314b3b5a6d commit 56de014ae2e5f80d5a009401d8d513314b3b5a6d Author: boliu <boliu@chromium.org> Date: Sat May 06 00:49:50 2017 android: Remove ChildProcessServiceImpl.sContext getContext is never called so the only use case left is checking that create is not called more than once in this process. It's an AutomicReference, but is not used atomically (ie it's not really calling compareAndSet). So it can just replace it with a bool instead. BUG= 689758 Review-Url: https://codereview.chromium.org/2861673009 Cr-Commit-Position: refs/heads/master@{#469832} [modify] https://crrev.com/56de014ae2e5f80d5a009401d8d513314b3b5a6d/content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
,
May 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3aa41d205d3efc87258a59e4db771ba9cabee9c2 commit 3aa41d205d3efc87258a59e4db771ba9cabee9c2 Author: jcivelli <jcivelli@chromium.org> Date: Mon May 08 22:08:11 2017 Making ChildConnectionAllocator simpler. Removing the sandboxed state from ChildConnectionAllocator and also removing the default service class names: it is now always retrieved from the AndroidManifest.xml. BaseChildProcessConnection does not have a sandboxed state anymore either now. The allocators are now held and managed by the ChildProcessLauncher. BUG= 689758 Review-Url: https://codereview.chromium.org/2863063002 Cr-Commit-Position: refs/heads/master@{#470146} [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/android_webview/apk/java/AndroidManifest.xml [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/android_webview/test/shell/AndroidManifest.xml [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/chrome/android/java/AndroidManifest.xml [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/chromecast/browser/android/apk/AndroidManifest.xml.jinja2 [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/components/test/android/browsertests_apk/AndroidManifest.xml.jinja2 [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/content/public/android/java/src/org/chromium/content/browser/BaseChildProcessConnection.java [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/content/public/android/java/src/org/chromium/content/browser/ImportantChildProcessConnection.java [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/content/public/test/android/javatests/src/org/chromium/content/browser/test/ChildProcessAllocatorSettingsHook.java [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/content/shell/android/browsertests_apk/AndroidManifest.xml.jinja2 [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/content/shell/android/linker_test_apk/AndroidManifest.xml.jinja2 [modify] https://crrev.com/3aa41d205d3efc87258a59e4db771ba9cabee9c2/content/shell/android/shell_apk/AndroidManifest.xml.jinja2
,
May 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1cbf49d95de1db9249589a14344f95da79110ac0 commit 1cbf49d95de1db9249589a14344f95da79110ac0 Author: jcivelli <jcivelli@chromium.org> Date: Tue May 09 04:06:16 2017 Removing the unused child process ID parameter in ChildProcessLauncher. ChildProcessLauncher has a child process ID parameter which is not used. Removing it. BUG= 689758 Review-Url: https://codereview.chromium.org/2865173002 Cr-Commit-Position: refs/heads/master@{#470180} [modify] https://crrev.com/1cbf49d95de1db9249589a14344f95da79110ac0/content/browser/child_process_launcher_helper_android.cc [modify] https://crrev.com/1cbf49d95de1db9249589a14344f95da79110ac0/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [modify] https://crrev.com/1cbf49d95de1db9249589a14344f95da79110ac0/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java [modify] https://crrev.com/1cbf49d95de1db9249589a14344f95da79110ac0/content/public/android/java/src/org/chromium/content/browser/ChildSpawnData.java [modify] https://crrev.com/1cbf49d95de1db9249589a14344f95da79110ac0/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java [modify] https://crrev.com/1cbf49d95de1db9249589a14344f95da79110ac0/content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestUtils.java
,
May 12 2017
,
May 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e8aabd539bd139049d3890562a45339428e7ece1 commit e8aabd539bd139049d3890562a45339428e7ece1 Author: jcivelli <jcivelli@chromium.org> Date: Mon May 15 17:38:24 2017 Merging Managed and Important connection together again. As part of the effort to move ChildProcessConnection to base, merging ImportantChildProcessConnection and ManagedChildProcessConnection into a unique class again. The ChildProcessLauncher adds a strong binding to privileged processes and add sanboxed processes to the BindingManager. Also changing the OOM protected state to waived bound only and making the ChildProcessLauncherHelper policy clearer. BUG= 689758 Review-Url: https://codereview.chromium.org/2874643002 Cr-Commit-Position: refs/heads/master@{#471819} [modify] https://crrev.com/e8aabd539bd139049d3890562a45339428e7ece1/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java [modify] https://crrev.com/e8aabd539bd139049d3890562a45339428e7ece1/chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java [modify] https://crrev.com/e8aabd539bd139049d3890562a45339428e7ece1/content/public/android/BUILD.gn [modify] https://crrev.com/e8aabd539bd139049d3890562a45339428e7ece1/content/public/android/java/src/org/chromium/content/browser/BindingManager.java [modify] https://crrev.com/e8aabd539bd139049d3890562a45339428e7ece1/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java [modify] https://crrev.com/e8aabd539bd139049d3890562a45339428e7ece1/content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java [rename] https://crrev.com/e8aabd539bd139049d3890562a45339428e7ece1/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java [modify] https://crrev.com/e8aabd539bd139049d3890562a45339428e7ece1/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [modify] https://crrev.com/e8aabd539bd139049d3890562a45339428e7ece1/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java [delete] https://crrev.com/ea05207e94618c286f12bbf97f5f65ecc552d663/content/public/android/java/src/org/chromium/content/browser/ImportantChildProcessConnection.java [delete] https://crrev.com/ea05207e94618c286f12bbf97f5f65ecc552d663/content/public/android/java/src/org/chromium/content/browser/ManagedChildProcessConnection.java [modify] https://crrev.com/e8aabd539bd139049d3890562a45339428e7ece1/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherIntegrationTest.java [modify] https://crrev.com/e8aabd539bd139049d3890562a45339428e7ece1/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java [modify] https://crrev.com/e8aabd539bd139049d3890562a45339428e7ece1/content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java [modify] https://crrev.com/e8aabd539bd139049d3890562a45339428e7ece1/content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestHelperService.java [modify] https://crrev.com/e8aabd539bd139049d3890562a45339428e7ece1/content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestUtils.java
,
May 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9a50e07bda300db2b09f4caf8bb52943bee15b4d commit 9a50e07bda300db2b09f4caf8bb52943bee15b4d Author: Jay Civelli <jcivelli@google.com> Date: Thu May 18 03:27:59 2017 Factoring out spare connection in its own class. Factoring out spare connection managerment out of ChildProcessLauncher and adding unit-tests. BUG= 689758 Change-Id: Ie5337c052133be87cee21a8f7aeab8fd19a0354e Reviewed-on: https://chromium-review.googlesource.com/506890 Reviewed-by: Bo Liu <boliu@chromium.org> Commit-Queue: Jay Civelli <jcivelli@chromium.org> Cr-Commit-Position: refs/heads/master@{#472633} [modify] https://crrev.com/9a50e07bda300db2b09f4caf8bb52943bee15b4d/content/public/android/BUILD.gn [modify] https://crrev.com/9a50e07bda300db2b09f4caf8bb52943bee15b4d/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java [modify] https://crrev.com/9a50e07bda300db2b09f4caf8bb52943bee15b4d/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [add] https://crrev.com/9a50e07bda300db2b09f4caf8bb52943bee15b4d/content/public/android/java/src/org/chromium/content/browser/SpareChildConnection.java [add] https://crrev.com/9a50e07bda300db2b09f4caf8bb52943bee15b4d/content/public/android/junit/src/org/chromium/content/browser/SpareChildConnectionTest.java
,
May 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8128a03ebe80194af98245cae93009e846cac283 commit 8128a03ebe80194af98245cae93009e846cac283 Author: Bo Liu <boliu@chromium.org> Date: Mon May 22 21:52:01 2017 android: Fix var name from warmup to queueIfNoneAvailable BUG= 689758 Change-Id: Iab841c65ab1a5acdd81b7e3e41015b16edd7ef30 Reviewed-on: https://chromium-review.googlesource.com/511302 Reviewed-by: Jay Civelli <jcivelli@chromium.org> Commit-Queue: Bo Liu <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#473713} [modify] https://crrev.com/8128a03ebe80194af98245cae93009e846cac283/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
,
May 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9f35c62e5cdcef375cf9113ea4202bbe697006e commit a9f35c62e5cdcef375cf9113ea4202bbe697006e Author: Bo Liu <boliu@chromium.org> Date: Wed May 24 21:35:02 2017 android: Remove ConnectionAllocator from global map when all connections are freed. This fixes a TODO. BUG= 689758 Change-Id: I93fd1701b49a64d5635565761a61147e6d44f152 Reviewed-on: https://chromium-review.googlesource.com/513587 Reviewed-by: Jay Civelli <jcivelli@chromium.org> Commit-Queue: Bo Liu <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#474431} [modify] https://crrev.com/a9f35c62e5cdcef375cf9113ea4202bbe697006e/content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java [modify] https://crrev.com/a9f35c62e5cdcef375cf9113ea4202bbe697006e/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [modify] https://crrev.com/a9f35c62e5cdcef375cf9113ea4202bbe697006e/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
,
May 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8b2191fa170cbf333ccd4b01f6504a3885703a00 commit 8b2191fa170cbf333ccd4b01f6504a3885703a00 Author: Jay Civelli <jcivelli@google.com> Date: Sun May 28 15:58:22 2017 Moving queuing of pending process creation to CPL. In preparation for the move to base of part of the ChildProcessLauncher functionality, moving the queueing of ChildProcessConnection creation from ChildConnectionAllocator to ChildProcessLauncher so that the queueing can be done at the content layer. With that we can remove the ChildSpawnData class and simplify SpareConnection so it only requires the connection allocator and the extra creation params. Bug: 689758 Change-Id: Ia0bed45e2abd3f81f140a69a86173c5ef1039daa Reviewed-on: https://chromium-review.googlesource.com/508443 Commit-Queue: Jay Civelli <jcivelli@chromium.org> Reviewed-by: Bo Liu <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#475263} [modify] https://crrev.com/8b2191fa170cbf333ccd4b01f6504a3885703a00/content/public/android/BUILD.gn [modify] https://crrev.com/8b2191fa170cbf333ccd4b01f6504a3885703a00/content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java [modify] https://crrev.com/8b2191fa170cbf333ccd4b01f6504a3885703a00/content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java [modify] https://crrev.com/8b2191fa170cbf333ccd4b01f6504a3885703a00/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [modify] https://crrev.com/8b2191fa170cbf333ccd4b01f6504a3885703a00/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java [delete] https://crrev.com/484bedec5922468b681054f150c28504bdd144e1/content/public/android/java/src/org/chromium/content/browser/ChildSpawnData.java [modify] https://crrev.com/8b2191fa170cbf333ccd4b01f6504a3885703a00/content/public/android/java/src/org/chromium/content/browser/SpareChildConnection.java [modify] https://crrev.com/8b2191fa170cbf333ccd4b01f6504a3885703a00/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherIntegrationTest.java [modify] https://crrev.com/8b2191fa170cbf333ccd4b01f6504a3885703a00/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java [modify] https://crrev.com/8b2191fa170cbf333ccd4b01f6504a3885703a00/content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java [modify] https://crrev.com/8b2191fa170cbf333ccd4b01f6504a3885703a00/content/public/android/junit/src/org/chromium/content/browser/SpareChildConnectionTest.java [modify] https://crrev.com/8b2191fa170cbf333ccd4b01f6504a3885703a00/content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestUtils.java
,
Jun 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6b7030d491f6a37fda078a059f71b9e55b0c3528 commit 6b7030d491f6a37fda078a059f71b9e55b0c3528 Author: Jay Civelli <jcivelli@google.com> Date: Wed Jun 07 01:04:37 2017 Merging ChildProcessLauncher into ChildProcessLauncherHelper. Moving the logic in ChildProcessLauncher into ChildProcessLauncherHelper. This is in preparation for moving the core process launching code to base. Also cleaned-up the ChildProcessLauncherTest tests. Bug: 689758 Change-Id: I347a74f7c53daacb7dfb5c11ae276c12e768b277 Reviewed-on: https://chromium-review.googlesource.com/519567 Commit-Queue: Jay Civelli <jcivelli@chromium.org> Reviewed-by: Bo Liu <boliu@chromium.org> Reviewed-by: Ted Choc <tedchoc@chromium.org> Cr-Commit-Position: refs/heads/master@{#477501} [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/android_webview/java/src/org/chromium/android_webview/AwRendererPriorityManager.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivitySessionTracker.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/content/public/android/BUILD.gn [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/content/public/android/java/src/org/chromium/content/app/ChildProcessService.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/content/public/android/java/src/org/chromium/content/browser/ChildConnectionAllocator.java [delete] https://crrev.com/178f2c6485a8e541d554eb1d14803f0d366bc715/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherIntegrationTest.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/content/public/test/android/javatests/src/org/chromium/content/browser/test/ChildProcessAllocatorSettingsHook.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestHelperService.java [modify] https://crrev.com/6b7030d491f6a37fda078a059f71b9e55b0c3528/content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestUtils.java
,
Nov 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/80943ada381be0b05562a02af9db38d6d426fe74 commit 80943ada381be0b05562a02af9db38d6d426fe74 Author: Bo Liu <boliu@chromium.org> Date: Fri Nov 17 15:34:23 2017 android: Remove 1s delay to remove strong binding This is needed to avoid dropping the strong binding when there were two racy signals to drop and add strong binding. Now the the "last foreground connection" has long been removed, it should be safe to just remove this delay. Bug: 689758 Change-Id: I874b7762f35c208f497189f142797200de307cd9 Reviewed-on: https://chromium-review.googlesource.com/767651 Reviewed-by: Yaron Friedman <yfriedman@chromium.org> Commit-Queue: Bo <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#517394} [modify] https://crrev.com/80943ada381be0b05562a02af9db38d6d426fe74/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java [modify] https://crrev.com/80943ada381be0b05562a02af9db38d6d426fe74/content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java
,
Nov 29 2017
We've started getting flaky SIGABRT failures in at least one of the VR instrumentation tests (VrShellNavigationTest#testNativeNavigationAndInteraction) with the following Java stderr: java.lang.AssertionError at org.chromium.content.browser.BindingManagerImpl$ModerateBindingPool.addConnection(BindingManagerImpl.java:99) at org.chromium.content.browser.BindingManagerImpl$ManagedConnection.addConnectionToModerateBindingPool(BindingManagerImpl.java:212) at org.chromium.content.browser.BindingManagerImpl$ManagedConnection.setPriority(BindingManagerImpl.java:247) at org.chromium.content.browser.BindingManagerImpl.setPriority(BindingManagerImpl.java:304) at org.chromium.content.browser.ChildProcessLauncherHelper.setPriority(ChildProcessLauncherHelper.java:494) at org.chromium.base.SystemMessageHandler.nativeDoRunLoopOnce(Native Method) at org.chromium.base.SystemMessageHandler.handleMessage(SystemMessageHandler.java:51) at android.os.Handler.dispatchMessage(Handler.java:102) at android.os.Looper.loop(Looper.java:154) at android.os.HandlerThread.run(HandlerThread.java:61) Is it possible your most recent patch for this could have caused it? It's the only recent change to BindingManagerImpl.java
,
Nov 29 2017
hmm, seems likely. I don't think there were any guarantee this used to hold, but that change probably make it more likely to fail (on non low ram devices)
,
Nov 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8275fa4711f93c2bcb517efabf8826625ba1609b commit 8275fa4711f93c2bcb517efabf8826625ba1609b Author: Bo Liu <boliu@chromium.org> Date: Wed Nov 29 15:48:18 2017 android: Fix add moderate binding assert Two independent signals cause a binding to be added to the moderate binding pool. * view becomes invisible, ie when it drops strong binding * view no longer has pending views, ie when it drops the initial binding. (Previously, this was the onDeterminedVisibility signal) So the assert that a connection is only added once does not actually hold. The assert was added to make sure the moderate pool only adds a single refcount on the moderate binding. So turn the assert into a runtime check instead. Also add back the old comment explaining why the second signal exists. Bug: 689758 Change-Id: I50471eee598dbf57108b91fc478d34b0bbee0e6b Reviewed-on: https://chromium-review.googlesource.com/795094 Reviewed-by: Yaron Friedman <yfriedman@chromium.org> Commit-Queue: Bo <boliu@chromium.org> Cr-Commit-Position: refs/heads/master@{#520125} [modify] https://crrev.com/8275fa4711f93c2bcb517efabf8826625ba1609b/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java
,
Apr 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0ac39eeeb4b2e6092f1ecbfd8cf960a46a449169 commit 0ac39eeeb4b2e6092f1ecbfd8cf960a46a449169 Author: Bo Liu <boliu@chromium.org> Date: Tue Apr 10 22:37:06 2018 android: Refactor BindingManagerImpl Make BindingManagerImpl only be responsible for the LRU queue of connections and the corresponding moderate binding ref count. Lift up controls of all other bindings to ChildProcessLauncherHelper. More concretely, remove ManagedConnection and merged ModerateBindingPool directly into BindingManagerImpl. Moved the logic of whether moderate binding management is enabled to ChildProcessLauncherHelper as well. This is not a totally no-op refactor: * Visibility signals now work on "always foreground", ie non-sandboxed, connections. This makes non-sandboxed connections less special. Note this has no practical difference since non-renderer processes do not have priorities set currently. * Connection with strong binding can remain in LRU queue. This is just a consequence of making these two systems independent. * The "use" signal, in the LRU sense, is now when a connection gains strong binding only. Previously a connection is marked as most recent when strong or initial binding is dropped, which doesn't make much sense. For tests, essentially deleted a lot of tests that explicitly check binding state, and at the same time breaks layering. One clean up not yet done to make review easier is that the BindingManager interface can now be removed. Bug: 689758 Change-Id: I32489a809bee944fbe26f34ca1de06b4e65c310e Reviewed-on: https://chromium-review.googlesource.com/642058 Commit-Queue: Bo <boliu@chromium.org> Reviewed-by: Yaron Friedman <yfriedman@chromium.org> Reviewed-by: Jay Civelli <jcivelli@chromium.org> Cr-Commit-Position: refs/heads/master@{#549665} [modify] https://crrev.com/0ac39eeeb4b2e6092f1ecbfd8cf960a46a449169/chrome/android/java_sources.gni [delete] https://crrev.com/f74f39e43de8c481d621820074946dffa5d2f0aa/chrome/android/javatests/src/org/chromium/chrome/browser/BindingManagerIntegrationTest.java [modify] https://crrev.com/0ac39eeeb4b2e6092f1ecbfd8cf960a46a449169/content/public/android/java/src/org/chromium/content/browser/BindingManager.java [modify] https://crrev.com/0ac39eeeb4b2e6092f1ecbfd8cf960a46a449169/content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java [modify] https://crrev.com/0ac39eeeb4b2e6092f1ecbfd8cf960a46a449169/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelper.java [modify] https://crrev.com/0ac39eeeb4b2e6092f1ecbfd8cf960a46a449169/content/public/android/javatests/src/org/chromium/content/browser/webcontents/WebContentsTest.java [modify] https://crrev.com/0ac39eeeb4b2e6092f1ecbfd8cf960a46a449169/content/public/android/junit/src/org/chromium/content/browser/BindingManagerImplTest.java
,
Apr 10 2018
I'll claim victory here |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by boliu@chromium.org
, Feb 9 2017