Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
704573002:
Android: Use a spawning queue for child processes while we are at capacity. (Closed)
DescriptionAndroid: Use a spawning queue for child processes while we are at capacity.
- Put child process spawning requests in a queue while we are at capacity and
spawn when a service is freed.
- Delay reusing freed services to avoid immediately reusing.
BUG=429657
Committed: https://crrev.com/44f33c01325ccbe4bce804309636b102a74cc179
Cr-Commit-Position: refs/heads/master@{#309199}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fix. #
Total comments: 1
Patch Set 3 : Fix. #
Total comments: 16
Patch Set 4 : Fix. #Patch Set 5 : Rebased on master. #
Total comments: 2
Patch Set 6 : Fix. #Patch Set 7 : Added test. #
Total comments: 7
Patch Set 8 : Fix. #Patch Set 9 : Fix. #Patch Set 10 : Reverte last commit. #Patch Set 11 : Removed assert. #Patch Set 12 : Fix. #Patch Set 13 : Rebased on master. #
Total comments: 2
Patch Set 14 : Fix. #
Total comments: 2
Patch Set 15 : Revert previous commit. #Patch Set 16 : Fix for assert message. #
Messages
Total messages: 45 (6 generated)
auygun@opera.com changed reviewers: + ppi@chromium.org
@reviewer: How does this patch look to you?
Thanks! Please find some remarks below. https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:266: private static final Handler sHandler = new Handler(Looper.getMainLooper()) { We have the needed support in ThreadUtils - please take a look at ThreadUtils.postOnUiThreadDelayed(). This will also make us use a Runnable instead of Message which I think makes sense here. https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:274: start(pendingSpawn.context(), pendingSpawn.commandLine(), We used to call this only from the process spawning thread. I wonder if we should be worried about running this on the main thread now - I imagine it can cause some jankiness... Maybe it's acceptable for this quite special case? Let's ask other reviewers for opinion once we're done with the other remarks. https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:283: sHandler.sendMessageDelayed(sHandler.obtainMessage(0, connection), 1); Please extract the constant into a private static final field, see DETACH_AS_ACTIVE_HIGH_END_DELAY_MILLIS in BindingManagerImpl.
https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:266: private static final Handler sHandler = new Handler(Looper.getMainLooper()) { On 2014/11/04 16:51:53, ppi wrote: > We have the needed support in ThreadUtils - please take a look at > ThreadUtils.postOnUiThreadDelayed(). This will also make us use a Runnable > instead of Message which I think makes sense here. Done. https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:274: start(pendingSpawn.context(), pendingSpawn.commandLine(), On 2014/11/04 16:51:53, ppi wrote: > We used to call this only from the process spawning thread. I wonder if we > should be worried about running this on the main thread now - I imagine it can > cause some jankiness... Maybe it's acceptable for this quite special case? Let's > ask other reviewers for opinion once we're done with the other remarks. It works for me. I don't know if we should be worried about this. Would be good to get more opinions. https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:283: sHandler.sendMessageDelayed(sHandler.obtainMessage(0, connection), 1); On 2014/11/04 16:51:53, ppi wrote: > Please extract the constant into a private static final field, see > DETACH_AS_ACTIVE_HIGH_END_DELAY_MILLIS in BindingManagerImpl. Done.
Thanks! I think we should wait until we get the resolution of the spawn queue vs killing the excessive processes ourselves discussion on the crbug. I hope that won't take long.. please don't hesitate to let me know if you have any concerns. https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:274: start(pendingSpawn.context(), pendingSpawn.commandLine(), On 2014/11/05 11:23:18, auygun wrote: > On 2014/11/04 16:51:53, ppi wrote: > > We used to call this only from the process spawning thread. I wonder if we > > should be worried about running this on the main thread now - I imagine it can > > cause some jankiness... Maybe it's acceptable for this quite special case? > Let's > > ask other reviewers for opinion once we're done with the other remarks. > > It works for me. I don't know if we should be worried about this. Would be good > to get more opinions. I guess we might want to do the spawn on a background thread - ie. "new Thread(new Runnable() {" sort of thing.
https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:274: start(pendingSpawn.context(), pendingSpawn.commandLine(), On 2014/11/05 18:45:35, ppi wrote: > On 2014/11/05 11:23:18, auygun wrote: > > On 2014/11/04 16:51:53, ppi wrote: > > > We used to call this only from the process spawning thread. I wonder if we > > > should be worried about running this on the main thread now - I imagine it > can > > > cause some jankiness... Maybe it's acceptable for this quite special case? > > Let's > > > ask other reviewers for opinion once we're done with the other remarks. > > > > It works for me. I don't know if we should be worried about this. Would be > good > > to get more opinions. > > I guess we might want to do the spawn on a background thread - ie. "new > Thread(new Runnable() {" sort of thing. In the comment it says that start maybe called on any thread. Why would it be necessary to create another thread here?
ohrn@opera.com changed reviewers: + ohrn@opera.com
https://codereview.chromium.org/704573002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/704573002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:264: // is bound at that point, the process is reused and bad things happen (mostly static Note that I have proposed a fix for this, alleviating the need for a delay, but it's stuck in limbo. https://codereview.chromium.org/146693011/
https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:274: start(pendingSpawn.context(), pendingSpawn.commandLine(), On 2014/11/06 11:55:58, auygun wrote: > On 2014/11/05 18:45:35, ppi wrote: > > On 2014/11/05 11:23:18, auygun wrote: > > > On 2014/11/04 16:51:53, ppi wrote: > > > > We used to call this only from the process spawning thread. I wonder if we > > > > should be worried about running this on the main thread now - I imagine it > > can > > > > cause some jankiness... Maybe it's acceptable for this quite special case? > > > Let's > > > > ask other reviewers for opinion once we're done with the other remarks. > > > > > > It works for me. I don't know if we should be worried about this. Would be > > good > > > to get more opinions. > > > > I guess we might want to do the spawn on a background thread - ie. "new > > Thread(new Runnable() {" sort of thing. > > In the comment it says that start maybe called on any thread. Why would it be > necessary to create another thread here? Yes, it's safe to do this on the main thread. It's just for performance reasons (avoiding blocking the main thread) that we may want to do this in background. Iiirc, it can take a good couple of 10s of ms before the spawning call returns.
https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/704573002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:274: start(pendingSpawn.context(), pendingSpawn.commandLine(), On 2014/11/07 14:19:08, ppi wrote: > On 2014/11/06 11:55:58, auygun wrote: > > On 2014/11/05 18:45:35, ppi wrote: > > > On 2014/11/05 11:23:18, auygun wrote: > > > > On 2014/11/04 16:51:53, ppi wrote: > > > > > We used to call this only from the process spawning thread. I wonder if > we > > > > > should be worried about running this on the main thread now - I imagine > it > > > can > > > > > cause some jankiness... Maybe it's acceptable for this quite special > case? > > > > Let's > > > > > ask other reviewers for opinion once we're done with the other remarks. > > > > > > > > It works for me. I don't know if we should be worried about this. Would be > > > good > > > > to get more opinions. > > > > > > I guess we might want to do the spawn on a background thread - ie. "new > > > Thread(new Runnable() {" sort of thing. > > > > In the comment it says that start maybe called on any thread. Why would it be > > necessary to create another thread here? > > Yes, it's safe to do this on the main thread. It's just for performance reasons > (avoiding blocking the main thread) that we may want to do this in background. > Iiirc, it can take a good couple of 10s of ms before the spawning call returns. Done.
ppi@chromium.org changed reviewers: + mariakhomenko@chromium.org
Apologies for letting this slip off my radar. The CL looks good modulo the nits below. We're one month before the branch point, and I think it would be a good idea to land this and to see how it goes. Adding Maria who is the new owner of the process management on our side, as I move from Chrome Android to other things. Maria, could you take a look here? More details / context is at http://crbug.com/429657 . https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:180: private static List<PendingSpawnData> sPendingSpawnDataList = nit: I would probably just call it "sPendingSpawns" https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:183: public synchronized void enqueue(final PendingSpawnData pendingSpawn) { Could you add private static final Object mPendingSpawnsLock = new Object(); and explicitly lock on it? We tend to prefer explicit locks to synchronized methods.
https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:183: public synchronized void enqueue(final PendingSpawnData pendingSpawn) { javadoc for public methods https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:188: return !sPendingSpawnDataList.isEmpty() ? sPendingSpawnDataList.remove(0) : null; this can be return sPendingSpawnDataList.poll() instead https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:298: } Would it be possible to add a test for the new behaviour?
https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:180: private static List<PendingSpawnData> sPendingSpawnDataList = On 2014/12/12 19:01:51, ppi wrote: > nit: I would probably just call it "sPendingSpawns" Done. https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:183: public synchronized void enqueue(final PendingSpawnData pendingSpawn) { On 2014/12/12 19:01:51, ppi wrote: > Could you add private static final Object mPendingSpawnsLock = new Object(); and > explicitly lock on it? We tend to prefer explicit locks to synchronized methods. Done. https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:183: public synchronized void enqueue(final PendingSpawnData pendingSpawn) { On 2014/12/12 21:02:47, Maria wrote: > javadoc for public methods Done. https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:188: return !sPendingSpawnDataList.isEmpty() ? sPendingSpawnDataList.remove(0) : null; On 2014/12/12 21:02:47, Maria wrote: > this can be > return sPendingSpawnDataList.poll() > instead It's an ArrayList. Should I change it to LinkedList instead? https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:298: } On 2014/12/12 21:02:47, Maria wrote: > Would it be possible to add a test for the new behaviour? I can't get the tests to work. I'm following steps in https://code.google.com/p/chromium/wiki/AndroidTestInstructions but can't run any test on my Nexus 5. I'll add a test for the new behaviour as soon as I get the tests to work.
https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:180: private static List<PendingSpawnData> sPendingSpawnDataList = On 2014/12/16 09:28:26, auygun wrote: > On 2014/12/12 19:01:51, ppi wrote: > > nit: I would probably just call it "sPendingSpawns" > > Done. I am not sure this change makes sense because now we have two sPendingSpawns -- one inside the class and one outside the class referring to the Queue. I think we should rename one of them to be different to avoid confusion. I don't care which one. https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:188: return !sPendingSpawnDataList.isEmpty() ? sPendingSpawnDataList.remove(0) : null; On 2014/12/16 09:28:26, auygun wrote: > On 2014/12/12 21:02:47, Maria wrote: > > this can be > > return sPendingSpawnDataList.poll() > > instead > > It's an ArrayList. Should I change it to LinkedList instead? Sorry, not sure why I thought LinkedList is implementing Queue interface. Thinking about it, given that we only remove from the beginning and append at the end, a LinkedList will probably work well for this. https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:298: } What's the problem you are seeing with running the tests? On 2014/12/16 09:28:26, auygun wrote: > On 2014/12/12 21:02:47, Maria wrote: > > Would it be possible to add a test for the new behaviour? > > I can't get the tests to work. I'm following steps in > https://code.google.com/p/chromium/wiki/AndroidTestInstructions but can't run > any test on my Nexus 5. I'll add a test for the new behaviour as soon as I get > the tests to work. https://codereview.chromium.org/704573002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/704573002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:171: static final Object mPendingSpawnsLock = new Object(); static object should be named "sPendingSpawnsLock"
https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:180: private static List<PendingSpawnData> sPendingSpawnDataList = On 2014/12/16 19:10:02, Maria wrote: > On 2014/12/16 09:28:26, auygun wrote: > > On 2014/12/12 19:01:51, ppi wrote: > > > nit: I would probably just call it "sPendingSpawns" > > > > Done. > > I am not sure this change makes sense because now we have two sPendingSpawns -- > one inside the class and one outside the class referring to the Queue. I think > we should rename one of them to be different to avoid confusion. I don't care > which one. I agree. I'll change the name of one of them. https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:188: return !sPendingSpawnDataList.isEmpty() ? sPendingSpawnDataList.remove(0) : null; On 2014/12/16 19:10:02, Maria wrote: > On 2014/12/16 09:28:26, auygun wrote: > > On 2014/12/12 21:02:47, Maria wrote: > > > this can be > > > return sPendingSpawnDataList.poll() > > > instead > > > > It's an ArrayList. Should I change it to LinkedList instead? > > Sorry, not sure why I thought LinkedList is implementing Queue interface. > Thinking about it, given that we only remove from the beginning and append at > the end, a LinkedList will probably work well for this. Done. https://codereview.chromium.org/704573002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:298: } On 2014/12/16 19:10:02, Maria wrote: > What's the problem you are seeing with running the tests? > > On 2014/12/16 09:28:26, auygun wrote: > > On 2014/12/12 21:02:47, Maria wrote: > > > Would it be possible to add a test for the new behaviour? > > > > I can't get the tests to work. I'm following steps in > > https://code.google.com/p/chromium/wiki/AndroidTestInstructions but can't run > > any test on my Nexus 5. I'll add a test for the new behaviour as soon as I get > > the tests to work. > When I run a test host_forwarder fails with a timeout error. I can't get "git try" to work either. "apply patch" fails. Only thing that works for me is "git cl try". https://codereview.chromium.org/704573002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/704573002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:171: static final Object mPendingSpawnsLock = new Object(); On 2014/12/16 19:10:02, Maria wrote: > static object should be named "sPendingSpawnsLock" Done.
lgtm https://codereview.chromium.org/704573002/diff/120001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/704573002/diff/120001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:38: Thread.sleep(100); We prefer to use CriteriaHelper in cases where we need to wait for something to happen to reduce flakiness in tests. Not sure what was the issue here, but could we use a criteria helper?
On 2014/12/17 19:33:04, Maria wrote: > lgtm > > https://codereview.chromium.org/704573002/diff/120001/content/public/android/... > File > content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java > (right): > > https://codereview.chromium.org/704573002/diff/120001/content/public/android/... > content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:38: > Thread.sleep(100); > We prefer to use CriteriaHelper in cases where we need to wait for something to > happen to reduce flakiness in tests. Not sure what was the issue here, but could > we use a criteria helper? Btw, are you trying to run tests locally on Lollipop or KitKat? I believe there's a bug with Lollipop -- so you may want to try on a KitKat device.
https://codereview.chromium.org/704573002/diff/120001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/704573002/diff/120001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:38: Thread.sleep(100); On 2014/12/17 19:33:04, Maria wrote: > We prefer to use CriteriaHelper in cases where we need to wait for something to > happen to reduce flakiness in tests. Not sure what was the issue here, but could > we use a criteria helper? Yes, we could and should wait for allocatedConnectionsCountForTesting() or connectedServicesCountForTesting() to become non-zero. We would assert that this did *not* happen, ie. that pollForCriteria returned false. https://codereview.chromium.org/704573002/diff/120001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:192: assertEquals(1, ChildProcessLauncher.allocatedConnectionsCountForTesting(appContext)); This assertion failed on the bot. I think the "allocatedConnectionsCountForTesting" is 0 immediately after the criterion you poll above is satisfied. It takes some time for the spawn / allocation to take place after dequeuing the spawn request. Maybe it would be better to pollForCriteria here?
https://codereview.chromium.org/704573002/diff/120001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/704573002/diff/120001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:38: Thread.sleep(100); On 2014/12/17 19:33:04, Maria wrote: > We prefer to use CriteriaHelper in cases where we need to wait for something to > happen to reduce flakiness in tests. Not sure what was the issue here, but could > we use a criteria helper? Sure. I'll change this. The issue was that the allocated connection count was not 0 after a failed connection attempt. It's because we now delay freeing a service in freeConnection(). https://codereview.chromium.org/704573002/diff/120001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:38: Thread.sleep(100); On 2014/12/18 09:48:10, ppi wrote: > On 2014/12/17 19:33:04, Maria wrote: > > We prefer to use CriteriaHelper in cases where we need to wait for something > to > > happen to reduce flakiness in tests. Not sure what was the issue here, but > could > > we use a criteria helper? > > Yes, we could and should wait for allocatedConnectionsCountForTesting() or > connectedServicesCountForTesting() to become non-zero. We would assert that this > did *not* happen, ie. that pollForCriteria returned false. Done. https://codereview.chromium.org/704573002/diff/120001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:192: assertEquals(1, ChildProcessLauncher.allocatedConnectionsCountForTesting(appContext)); On 2014/12/18 09:48:10, ppi wrote: > This assertion failed on the bot. I think the > "allocatedConnectionsCountForTesting" is 0 immediately after the criterion you > poll above is satisfied. It takes some time for the spawn / allocation to take > place after dequeuing the spawn request. > > Maybe it would be better to pollForCriteria here? Sure. I'll do pollForCriteria instead. Where did you see that this assertion was failed? I don't see it in the log. It says that the test failed but there is no detailed information.
On 2014/12/18 02:00:32, Maria wrote: > On 2014/12/17 19:33:04, Maria wrote: > > lgtm > > > > > https://codereview.chromium.org/704573002/diff/120001/content/public/android/... > > File > > > content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java > > (right): > > > > > https://codereview.chromium.org/704573002/diff/120001/content/public/android/... > > > content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:38: > > Thread.sleep(100); > > We prefer to use CriteriaHelper in cases where we need to wait for something > to > > happen to reduce flakiness in tests. Not sure what was the issue here, but > could > > we use a criteria helper? > > Btw, are you trying to run tests locally on Lollipop or KitKat? I believe > there's a bug with Lollipop -- so you may want to try on a KitKat device. My Nexus 5 has Lollipop. I've also tried to run the test on a Nexus 7 with Android 4.4.2. I'll try it on Nexus 4 as well.
https://codereview.chromium.org/704573002/diff/120001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/704573002/diff/120001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:192: assertEquals(1, ChildProcessLauncher.allocatedConnectionsCountForTesting(appContext)); On 2014/12/18 10:10:11, auygun wrote: > On 2014/12/18 09:48:10, ppi wrote: > > This assertion failed on the bot. I think the > > "allocatedConnectionsCountForTesting" is 0 immediately after the criterion you > > poll above is satisfied. It takes some time for the spawn / allocation to take > > place after dequeuing the spawn request. > > > > Maybe it would be better to pollForCriteria here? > > Sure. I'll do pollForCriteria instead. > > Where did you see that this assertion was failed? I don't see it in the log. It > says that the test failed but there is no detailed information. Yes, this is a bit confusing. On this page: http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes... there is a link to "logcat dump" in step 34. I think this is full record of the logcat during the execution of all steps. There I found the bit I think was relevant: 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: junit.framework.AssertionFailedError: expected:<1> but was:<0> 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at junit.framework.Assert.fail(Assert.java:50) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at junit.framework.Assert.failNotEquals(Assert.java:287) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at junit.framework.Assert.assertEquals(Assert.java:67) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at junit.framework.Assert.assertEquals(Assert.java:199) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at junit.framework.Assert.assertEquals(Assert.java:205) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at org.chromium.content.browser.ChildProcessLauncherTest.testPendingSpawnQueue(ChildProcessLauncherTest.java:192) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at java.lang.reflect.Method.invokeNative(Native Method) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at java.lang.reflect.Method.invoke(Method.java:515) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at junit.framework.TestCase.runBare(TestCase.java:134) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at junit.framework.TestResult$1.protect(TestResult.java:115) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at junit.framework.TestResult.runProtected(TestResult.java:133) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at junit.framework.TestResult.run(TestResult.java:118) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at junit.framework.TestCase.run(TestCase.java:124) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701)
On 2014/12/18 10:13:51, ppi wrote: > https://codereview.chromium.org/704573002/diff/120001/content/public/android/... > File > content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java > (right): > > https://codereview.chromium.org/704573002/diff/120001/content/public/android/... > content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:192: > assertEquals(1, > ChildProcessLauncher.allocatedConnectionsCountForTesting(appContext)); > On 2014/12/18 10:10:11, auygun wrote: > > On 2014/12/18 09:48:10, ppi wrote: > > > This assertion failed on the bot. I think the > > > "allocatedConnectionsCountForTesting" is 0 immediately after the criterion > you > > > poll above is satisfied. It takes some time for the spawn / allocation to > take > > > place after dequeuing the spawn request. > > > > > > Maybe it would be better to pollForCriteria here? > > > > Sure. I'll do pollForCriteria instead. > > > > Where did you see that this assertion was failed? I don't see it in the log. > It > > says that the test failed but there is no detailed information. > > Yes, this is a bit confusing. On this page: > http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes... > there is a link to "logcat dump" in step 34. I think this is full record of the > logcat during the execution of all steps. > > There I found the bit I think was relevant: > > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: > junit.framework.AssertionFailedError: expected:<1> but was:<0> > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > junit.framework.Assert.fail(Assert.java:50) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > junit.framework.Assert.failNotEquals(Assert.java:287) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > junit.framework.Assert.assertEquals(Assert.java:67) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > junit.framework.Assert.assertEquals(Assert.java:199) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > junit.framework.Assert.assertEquals(Assert.java:205) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > org.chromium.content.browser.ChildProcessLauncherTest.testPendingSpawnQueue(ChildProcessLauncherTest.java:192) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > java.lang.reflect.Method.invokeNative(Native Method) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > java.lang.reflect.Method.invoke(Method.java:515) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > junit.framework.TestCase.runBare(TestCase.java:134) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > junit.framework.TestResult$1.protect(TestResult.java:115) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > junit.framework.TestResult.runProtected(TestResult.java:133) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > junit.framework.TestResult.run(TestResult.java:118) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > junit.framework.TestCase.run(TestCase.java:124) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) > 0b8bb: 12-17 17:27:09.669 14985 14998 I TestRunner: at > android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) I get an "Access denied" for that logcat link. I've tried to run the test on a Nexus 4 with KitKat as well. Still failing. Something must be wrong with my system. Maybe I have an incompatible version of lighttpd. I'll try a newer version. Anyway, hopefully try bot will pass this time.
Now it's failing the delayed spawn:
0aebb: 12-18 10:47:11.975 15598 15626 E AndroidRuntime: FATAL EXCEPTION:
Thread-991
0aebb: 12-18 10:47:11.975 15598 15626 E AndroidRuntime: Process:
org.chromium.content_shell_apk, PID: 15598
0aebb: 12-18 10:47:11.975 15598 15626 E AndroidRuntime:
java.lang.AssertionError
0aebb: 12-18 10:47:11.975 15598 15626 E AndroidRuntime: at
org.chromium.content.browser.ChildProcessLauncher.start(ChildProcessLauncher.java:504)
0aebb: 12-18 10:47:11.975 15598 15626 E AndroidRuntime: at
org.chromium.content.browser.ChildProcessLauncher$2$1.run(ChildProcessLauncher.java:320)
0aebb: 12-18 10:47:11.975 15598 15626 E AndroidRuntime: at
java.lang.Thread.run(Thread.java:841)
0aebb: 12-18 10:47:11.975 626 739 W ActivityManager: Error in app
org.chromium.content_shell_apk running instrumentation
ComponentInfo{org.chromium.content_shell_apk.tests/android.test.InstrumentationTestRunner}:
0aebb: 12-18 10:47:11.975 626 739 W ActivityManager:
java.lang.AssertionError
0aebb: 12-18 10:47:11.975 626 739 W ActivityManager:
java.lang.AssertionError
It seems that we need to pass a non-zero clientContext for the test spawn?
On 2014/12/18 13:35:38, ppi wrote:
> Now it's failing the delayed spawn:
>
> 0aebb: 12-18 10:47:11.975 15598 15626 E AndroidRuntime: FATAL EXCEPTION:
> Thread-991
> 0aebb: 12-18 10:47:11.975 15598 15626 E AndroidRuntime: Process:
> org.chromium.content_shell_apk, PID: 15598
> 0aebb: 12-18 10:47:11.975 15598 15626 E AndroidRuntime:
> java.lang.AssertionError
> 0aebb: 12-18 10:47:11.975 15598 15626 E AndroidRuntime: at
>
org.chromium.content.browser.ChildProcessLauncher.start(ChildProcessLauncher.java:504)
> 0aebb: 12-18 10:47:11.975 15598 15626 E AndroidRuntime: at
>
org.chromium.content.browser.ChildProcessLauncher$2$1.run(ChildProcessLauncher.java:320)
> 0aebb: 12-18 10:47:11.975 15598 15626 E AndroidRuntime: at
> java.lang.Thread.run(Thread.java:841)
> 0aebb: 12-18 10:47:11.975 626 739 W ActivityManager: Error in app
> org.chromium.content_shell_apk running instrumentation
>
ComponentInfo{org.chromium.content_shell_apk.tests/android.test.InstrumentationTestRunner}:
> 0aebb: 12-18 10:47:11.975 626 739 W ActivityManager:
> java.lang.AssertionError
> 0aebb: 12-18 10:47:11.975 626 739 W ActivityManager:
> java.lang.AssertionError
>
>
> It seems that we need to pass a non-zero clientContext for the test spawn?
If we pass a non-zero clientContext, nativeOnChildProcessStarted will be called
and it'll crash as clientContext is the pointer to a callback it calls.
if (clientContext != 0) { // Will be 0 in Java
instrumentation tests.
nativeOnChildProcessStarted(clientContext, pid);
I think we need to remove the assert and let start() to take zero clientContext.
Sounds reasonable.
On 2014/12/18 13:50:20, ppi wrote: > Sounds reasonable. Or we can move part of start() into startInternal() and call that one instead. This way we can keep the assert in start().
Either works. I don't think the assert is that useful. If we want to make sure that the native side passes a non-zero clientContext, we can add a DCHECK at the call site in the native code, and handle nicely 0 on the Java side (ie. remove the assert) so that we can run our tests.
New error:
9b8bb: 12-18 15:40:17.055 15684 15712 E AndroidRuntime:
java.lang.AssertionError
9b8bb: 12-18 15:40:17.055 15684 15712 E AndroidRuntime: at
org.chromium.content.browser.ChildProcessLauncher.start(ChildProcessLauncher.java:517)
9b8bb: 12-18 15:40:17.055 15684 15712 E AndroidRuntime: at
org.chromium.content.browser.ChildProcessLauncher$2$1.run(ChildProcessLauncher.java:320)
9b8bb: 12-18 15:40:17.055 15684 15712 E AndroidRuntime: at
java.lang.Thread.run(Thread.java:841)
9b8bb: 12-18 15:40:17.055 618 867 W ActivityManager: Error in app
org.chromium.content_shell_apk running instrumentation
ComponentInfo{org.chromium.content_shell_apk.tests/android.test.InstrumentationTestRunner}:
9b8bb: 12-18 15:40:17.055 618 867 W ActivityManager:
java.lang.AssertionError
9b8bb: 12-18 15:40:17.055 618 867 W ActivityManager:
java.lang.AssertionError
BTW could you paste how you run the tests locally (what command) and what is the
error you see?
On 2014/12/18 16:14:49, ppi wrote:
> New error:
>
> 9b8bb: 12-18 15:40:17.055 15684 15712 E AndroidRuntime:
> java.lang.AssertionError
> 9b8bb: 12-18 15:40:17.055 15684 15712 E AndroidRuntime: at
>
org.chromium.content.browser.ChildProcessLauncher.start(ChildProcessLauncher.java:517)
> 9b8bb: 12-18 15:40:17.055 15684 15712 E AndroidRuntime: at
>
org.chromium.content.browser.ChildProcessLauncher$2$1.run(ChildProcessLauncher.java:320)
> 9b8bb: 12-18 15:40:17.055 15684 15712 E AndroidRuntime: at
> java.lang.Thread.run(Thread.java:841)
> 9b8bb: 12-18 15:40:17.055 618 867 W ActivityManager: Error in app
> org.chromium.content_shell_apk running instrumentation
>
ComponentInfo{org.chromium.content_shell_apk.tests/android.test.InstrumentationTestRunner}:
> 9b8bb: 12-18 15:40:17.055 618 867 W ActivityManager:
> java.lang.AssertionError
> 9b8bb: 12-18 15:40:17.055 618 867 W ActivityManager:
> java.lang.AssertionError
>
>
> BTW could you paste how you run the tests locally (what command) and what is
the
> error you see?
Command line:
build/android/test_runner.py instrumentation --test-apk ContentShellTest
--test_data content:content/test/data/android/device_files --release --verbose
Then it waits for a long time at the following step.
[2014-12-17 11:26:52,968] [9306] [host]>
/home/attila/code/upstream/src/out/Release/host_forwarder
--serial-id=015d3fbaaa601408 --map 8000 8001
Then python script stops with a timeout error.
I tried to debug host_forwarder. It succeeds to setup the socket but never
receives data thus waits forever.
The CQ bit was checked by auygun@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/704573002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Finally it passes all tests.
Almost there, please take a look at one more thing below. https://codereview.chromium.org/704573002/diff/240001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/704573002/diff/240001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:42: return ChildProcessLauncher.allocatedConnectionsCountForTesting( To reliably ensure that the connection was not allocated (during the entire wait time), we should do the inverse of that - assertFalse on pollForCriteria that allocatedConnectionsCountForTesting > 0; Please see also the earlier comment ("we should wait for allocatedConnectionsCountForTesting() or connectedServicesCountForTesting() to become non-zero. We would assert that this did *not* happen, ie. that pollForCriteria returned false.").
Almost there, please take a look at one more thing below.
https://codereview.chromium.org/704573002/diff/240001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/704573002/diff/240001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:42: return ChildProcessLauncher.allocatedConnectionsCountForTesting( On 2014/12/19 12:28:52, ppi wrote: > To reliably ensure that the connection was not allocated (during the entire wait > time), we should do the inverse of that - assertFalse on pollForCriteria that > allocatedConnectionsCountForTesting > 0; > > Please see also the earlier comment ("we should wait for > allocatedConnectionsCountForTesting() or > connectedServicesCountForTesting() to become non-zero. We would assert that this > did *not* happen, ie. that pollForCriteria returned false."). Sure. I'll also change the assert below. It doesn't need to wait for criteria. connectedServicesCountForTesting() should return 0 as the connection was failed.
https://codereview.chromium.org/704573002/diff/260001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/704573002/diff/260001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:38: assertFalse("Failed connection wasn't released from the allocator.", Apologies for the confusion, but reading the code again I realised that perhaps with the delay in deallocating the connection, you actually do want to check that allocatedConnectionsCountForTesting *eventually goes to 0*, not *never gets above 0*. In which case, your earlier patch set 13 is what we need. If it fails after the change, please accept my apologies and let's land it how it was in ps 13, which lgtm (the presubmit bot will now work).
https://codereview.chromium.org/704573002/diff/260001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java (right): https://codereview.chromium.org/704573002/diff/260001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java:38: assertFalse("Failed connection wasn't released from the allocator.", On 2014/12/19 13:40:34, ppi wrote: > Apologies for the confusion, but reading the code again I realised that perhaps > with the delay in deallocating the connection, you actually do want to check > that allocatedConnectionsCountForTesting *eventually goes to 0*, not *never gets > above 0*. > > In which case, your earlier patch set 13 is what we need. If it fails after the > change, please accept my apologies and let's land it how it was in ps 13, which > lgtm (the presubmit bot will now work). No worries. It's now reverted back to patch set 13.
The CQ bit was checked by auygun@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/704573002/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/44f33c01325ccbe4bce804309636b102a74cc179 Cr-Commit-Position: refs/heads/master@{#309199}
Message was sent while issue was closed.
Aaand it's in:). Thanks for working on this! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
