Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1307793003:
Execute operations of ChildProcessLauncher on launcher thread (Closed)
|
Created:
5 years, 3 months ago by Jaekyun Seok (inactive) Modified:
5 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, jam, Maria, Jinsuk Kim Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExecute operations of ChildProcessLauncher on launcher thread
- run all codes accessing BindingManager on launcher thread except isOomProtected()
- check checkCalledOnProcessLauncherThread for public methods of BindingManagerImpl except isOomProtected()
- avoid NPE in ManagedConnection.isOomProtected
- remove unnecessary codes for thread safe
BUG=515715
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : Move operations on mConnection out of synchronization #Patch Set 5 : Execute operations on LauncherThread #Patch Set 6 : Fix PrerenderServiceTest and ChildProcessLauncherTest #
Total comments: 22
Patch Set 7 : Apply Yaron's comments #
Total comments: 22
Patch Set 8 : Remove cleanup of mManagedConnections #Patch Set 9 : Add checkIfCalledOnValidThread into inner classes of BindingManagerImpl #Patch Set 10 : #Patch Set 11 : Break down inner classes #Patch Set 12 : #
Total comments: 2
Patch Set 13 : Resolve Yaron's comments #
Total comments: 24
Patch Set 14 : Apply Yaron's comments #
Total comments: 4
Patch Set 15 : Apply Yaron's comments #Messages
Total messages: 43 (4 generated)
jaekyun@chromium.org changed reviewers: + sievers@chromium.org
sievers@, please review this CL.
On 2015/08/26 03:26:10, Jaekyun Seok wrote: > sievers@, please review this CL. Ping?
jaekyun@chromium.org changed reviewers: + mariakhomenko@chromium.org
mariakhomenko@, could you please review this CL?
mariakhomenko@chromium.org changed reviewers: + yfriedman@chromium.org
Hey Jaekyun, I don't have that much experience with Java synchronization, so going to pass this code review to Yaron. Yaron, can you take a look instead of me?
Isn't that equivalent to just making all methods in ManagedConnection 'synchronized'?
On 2015/08/29 00:15:19, sievers wrote: > Isn't that equivalent to just making all methods in ManagedConnection > 'synchronized'? Calling methods of ChildProcessConnectionImpl inside the lock of ManagedConnection is dangerous because there are two callbacks from ChildProcessConnectionImpl to ChildProcessLauncher which eventually reaches ManagedConnection. So I want to keep calls to ChildProcessConnectionImpl outside the lock of ManagedConnection.
On 2015/08/29 00:15:19, sievers wrote: > Isn't that equivalent to just making all methods in ManagedConnection > 'synchronized'? Calling methods of ChildProcessConnectionImpl inside the lock of ManagedConnection is dangerous because there are two callbacks from ChildProcessConnectionImpl to ChildProcessLauncher which eventually reaches ManagedConnection. So I want to keep calls to ChildProcessConnectionImpl outside the lock of ManagedConnection.
On 2015/08/31 00:50:44, Jaekyun Seok wrote: > On 2015/08/29 00:15:19, sievers wrote: > > Isn't that equivalent to just making all methods in ManagedConnection > > 'synchronized'? > > Calling methods of ChildProcessConnectionImpl inside the lock of > ManagedConnection is dangerous because there are two callbacks from > ChildProcessConnectionImpl to ChildProcessLauncher which eventually reaches > ManagedConnection. > > So I want to keep calls to ChildProcessConnectionImpl outside the lock of > ManagedConnection. Understood. I share your concerns about deadlocking but this still seems fragile (having to know the impl details of both classes) and this could easily be broken by future change. Perhaps the locking can be done at a different layer/level? Or alternatively, can you devise proper rules/expectations for this code? I think the two main actors are: main thread and process launcher thread. Can you split the API/objects such that it's clear which operations are safe to execute on which threads and restrict the locking to a few common elements? Another q: given current design is it possible that the UI thread gets blocked on a process launch?
On 2015/08/31 17:33:36, Yaron wrote: > On 2015/08/31 00:50:44, Jaekyun Seok wrote: > > On 2015/08/29 00:15:19, sievers wrote: > > > Isn't that equivalent to just making all methods in ManagedConnection > > > 'synchronized'? > > > > Calling methods of ChildProcessConnectionImpl inside the lock of > > ManagedConnection is dangerous because there are two callbacks from > > ChildProcessConnectionImpl to ChildProcessLauncher which eventually reaches > > ManagedConnection. > > > > So I want to keep calls to ChildProcessConnectionImpl outside the lock of > > ManagedConnection. > > Understood. I share your concerns about deadlocking but this still seems fragile > (having to know the impl details of both classes) and this could easily be > broken by future change. Perhaps the locking can be done at a different > layer/level? Or alternatively, can you devise proper rules/expectations for this > code? I think the two main actors are: main thread and process launcher thread. > Can you split the API/objects such that it's clear which operations are safe to > execute on which threads and restrict the locking to a few common elements? > Another q: given current design is it possible that the UI thread gets blocked > on a process launch? +1 I don't think this explicit locking everywhere will scale. Would be nice to have some object that the launcher can talk to, and for which the calls always happen on the launcher thread. I think that's most of ManagedConnection. By the way - it looks like mManagedConnections is leaky too. We never remove PID->ManagedConnection mappings, or do we? Can we just have the native ChildProcessLauncher object own something similar to a ManagedConnection instead? Because these lookups based on PID are a bit silly too.
On 2015/08/31 17:33:36, Yaron wrote: > On 2015/08/31 00:50:44, Jaekyun Seok wrote: > > On 2015/08/29 00:15:19, sievers wrote: > > > Isn't that equivalent to just making all methods in ManagedConnection > > > 'synchronized'? > > > > Calling methods of ChildProcessConnectionImpl inside the lock of > > ManagedConnection is dangerous because there are two callbacks from > > ChildProcessConnectionImpl to ChildProcessLauncher which eventually reaches > > ManagedConnection. > > > > So I want to keep calls to ChildProcessConnectionImpl outside the lock of > > ManagedConnection. > > Understood. I share your concerns about deadlocking but this still seems fragile > (having to know the impl details of both classes) and this could easily be > broken by future change. Perhaps the locking can be done at a different > layer/level? Or alternatively, can you devise proper rules/expectations for this > code? I think the two main actors are: main thread and process launcher thread. > Can you split the API/objects such that it's clear which operations are safe to > execute on which threads and restrict the locking to a few common elements? I will try that. > Another q: given current design is it possible that the UI thread gets blocked > on a process launch? That is possible because operations on ChildProcessConnectionImpl could happen on both threads.
On 2015/09/01 00:56:32, sievers wrote: > On 2015/08/31 17:33:36, Yaron wrote: > > On 2015/08/31 00:50:44, Jaekyun Seok wrote: > > > On 2015/08/29 00:15:19, sievers wrote: > > > > Isn't that equivalent to just making all methods in ManagedConnection > > > > 'synchronized'? > > > > > > Calling methods of ChildProcessConnectionImpl inside the lock of > > > ManagedConnection is dangerous because there are two callbacks from > > > ChildProcessConnectionImpl to ChildProcessLauncher which eventually reaches > > > ManagedConnection. > > > > > > So I want to keep calls to ChildProcessConnectionImpl outside the lock of > > > ManagedConnection. > > > > Understood. I share your concerns about deadlocking but this still seems > fragile > > (having to know the impl details of both classes) and this could easily be > > broken by future change. Perhaps the locking can be done at a different > > layer/level? Or alternatively, can you devise proper rules/expectations for > this > > code? I think the two main actors are: main thread and process launcher > thread. > > Can you split the API/objects such that it's clear which operations are safe > to > > execute on which threads and restrict the locking to a few common elements? > > Another q: given current design is it possible that the UI thread gets blocked > > on a process launch? > > +1 > > I don't think this explicit locking everywhere will scale. > Would be nice to have some object that the launcher can talk to, and for which > the calls always happen on the launcher thread. > I think that's most of ManagedConnection. Is there any way to get the launcher thread on Java layer? > > By the way - it looks like mManagedConnections is leaky too. We never remove > PID->ManagedConnection mappings, or do we? I will fix this. > > Can we just have the native ChildProcessLauncher object own something similar to > a ManagedConnection instead? Because these lookups based on PID are a bit silly > too. I will try that.
On 2015/09/01 02:04:21, Jaekyun Seok wrote: > On 2015/09/01 00:56:32, sievers wrote: > > On 2015/08/31 17:33:36, Yaron wrote: > > > On 2015/08/31 00:50:44, Jaekyun Seok wrote: > > > > On 2015/08/29 00:15:19, sievers wrote: > > > > > Isn't that equivalent to just making all methods in ManagedConnection > > > > > 'synchronized'? > > > > > > > > Calling methods of ChildProcessConnectionImpl inside the lock of > > > > ManagedConnection is dangerous because there are two callbacks from > > > > ChildProcessConnectionImpl to ChildProcessLauncher which eventually > reaches > > > > ManagedConnection. > > > > > > > > So I want to keep calls to ChildProcessConnectionImpl outside the lock of > > > > ManagedConnection. > > > > > > Understood. I share your concerns about deadlocking but this still seems > > fragile > > > (having to know the impl details of both classes) and this could easily be > > > broken by future change. Perhaps the locking can be done at a different > > > layer/level? Or alternatively, can you devise proper rules/expectations for > > this > > > code? I think the two main actors are: main thread and process launcher > > thread. > > > Can you split the API/objects such that it's clear which operations are safe > > to > > > execute on which threads and restrict the locking to a few common elements? > > > Another q: given current design is it possible that the UI thread gets > blocked > > > on a process launch? > > > > +1 > > > > I don't think this explicit locking everywhere will scale. > > Would be nice to have some object that the launcher can talk to, and for which > > the calls always happen on the launcher thread. > > I think that's most of ManagedConnection. > > Is there any way to get the launcher thread on Java layer? You can call up to java from the launcher thread (e.g. Java_ChildProcess_Launcher) and you can probably get a thread id but I don't think it has a java Looper or Handler. See the thread on the team list called [Does the IO thread have a Java Looper?] That might actually complicate things for you. Are there callbacks that you need to post from java to the java childprocess thread? > > > > > By the way - it looks like mManagedConnections is leaky too. We never remove > > PID->ManagedConnection mappings, or do we? > > I will fix this. > > > > > Can we just have the native ChildProcessLauncher object own something similar > to > > a ManagedConnection instead? Because these lookups based on PID are a bit > silly > > too. > > I will try that.
On 2015/09/01 16:01:32, Yaron wrote: > On 2015/09/01 02:04:21, Jaekyun Seok wrote: > > On 2015/09/01 00:56:32, sievers wrote: > > > On 2015/08/31 17:33:36, Yaron wrote: > > > > On 2015/08/31 00:50:44, Jaekyun Seok wrote: > > > > > On 2015/08/29 00:15:19, sievers wrote: > > > > > > Isn't that equivalent to just making all methods in ManagedConnection > > > > > > 'synchronized'? > > > > > > > > > > Calling methods of ChildProcessConnectionImpl inside the lock of > > > > > ManagedConnection is dangerous because there are two callbacks from > > > > > ChildProcessConnectionImpl to ChildProcessLauncher which eventually > > reaches > > > > > ManagedConnection. > > > > > > > > > > So I want to keep calls to ChildProcessConnectionImpl outside the lock > of > > > > > ManagedConnection. > > > > > > > > Understood. I share your concerns about deadlocking but this still seems > > > fragile > > > > (having to know the impl details of both classes) and this could easily be > > > > broken by future change. Perhaps the locking can be done at a different > > > > layer/level? Or alternatively, can you devise proper rules/expectations > for > > > this > > > > code? I think the two main actors are: main thread and process launcher > > > thread. > > > > Can you split the API/objects such that it's clear which operations are > safe > > > to > > > > execute on which threads and restrict the locking to a few common > elements? > > > > Another q: given current design is it possible that the UI thread gets > > blocked > > > > on a process launch? > > > > > > +1 > > > > > > I don't think this explicit locking everywhere will scale. > > > Would be nice to have some object that the launcher can talk to, and for > which > > > the calls always happen on the launcher thread. > > > I think that's most of ManagedConnection. > > > > Is there any way to get the launcher thread on Java layer? > > You can call up to java from the launcher thread (e.g. > Java_ChildProcess_Launcher) and you can probably get a thread id but I don't > think it has a java Looper or Handler. See the thread on the team list called > [Does the IO thread have a Java Looper?] That might actually complicate things > for you. Are there callbacks that you need to post from java to the java > childprocess thread? I modified codes to run operations on the launcher thread from Java layer using JNI. And there are two callbacks from ChildProcessConnectionImpl to ChildProcessLauncher. > > > > > > > > > By the way - it looks like mManagedConnections is leaky too. We never remove > > > PID->ManagedConnection mappings, or do we? > > > > I will fix this. > > > > > > > > Can we just have the native ChildProcessLauncher object own something > similar > > to > > > a ManagedConnection instead? Because these lookups based on PID are a bit > > silly > > > too. > > > > I will try that.
PTAL. I modified codes to execute all the operations of ChildProcessLauncher on launcher thread.
Honestly I'm struggling a bit with the review. It's hard to tell which operations should be on what thread and when you need to perform a thread hop (particularly in BindingManagerImpl) because there are so many locks already and we're adding additional thread hops. Naively, I had thought that if we change to a more strict threading model, we'd be able to remove some of the existing locking. At a minimum you need to update the CL description with more information about your change. Ideally though this can be captured in code. I've left a few comments, but let me try and be more constructive: - ChildProcessLauncher mostly makes sense. For the most part it lives on the PROCESS_LAUNCHER thread but it gets signals from the UI thread and various callbacks so those need to be posted. - Perhaps use @NonThreadSafe and calledOnValidThread to start making it clear which operations are on what threads? - I think BindingManagerImpl may need to be broken up although perhaps that can wait. I left more comments in line about things that were confusing https://codereview.chromium.org/1307793003/diff/100001/content/browser/androi... File content/browser/android/child_process_launcher_android.cc (right): https://codereview.chromium.org/1307793003/diff/100001/content/browser/androi... content/browser/android/child_process_launcher_android.cc:237: jboolean RunOnLauncherThread(JNIEnv* env, jclass clazz, jobject runnable) { I think you're going to have to rebase on torne's change and make this a JavaRef and call .obj within this function instead of in the caller. https://codereview.chromium.org/1307793003/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:47: interface LauncherThreadExecutor { This is basically a static but you're wrapping to avoid threading in tests (presumably?). If we make the threading model actually work on tests, then I'd prefer to see this be a static and avoid passing it around everywhere. https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:72: @Override Here you're posting to launcher thread within the function but addConnection you've done it in the caller. Can you do this more consistently so it's easier to reason about? https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:107: int oldSize = size(); Assuming you follow other suggestions, this would assert it's on launcher thread (or use @NonThreadSafe and verify it's the right thread). https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:176: mHandler.postDelayed(mDelayedClearer, onTesting This function is now very peculiar. We have an mHandler for the UIThread. We post a task for it to run after some delay. As soon as it runs, it grabs a mutex to ensure it can run but then posts back to the launcher thread to do its work. I know there was confusion abou t loopers/handlers so it might not be possible to post a delayed task for the current thread. (Maybe it's worth creating a native wrapper for Runnable so you can post to the native message loop). Regardless of that, I'm not sure the mutex is doing the right thing anymore. mDelayedClearer is now already cleared when the task is just posted to the process launcher thread. Is evictAll guaranteed to do the right thing? https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:263: ThreadUtils.postOnUiThreadDelayed(new Runnable() { General question: it seems like in many places we are now posting to the ui thread and then immediately going to process_launcher. Why bother going through the UI thread anymore? https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:528: }, MANAGED_CONNECTION_DELETE_DELAY_MILLS); This was intentionally leaky (specifically for the case of mWasOomProtected). See the class-level comments for ManagedConnection. Maybe that's not the best strategy, but how did you choose 3 seconds? https://codereview.chromium.org/1307793003/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:556: assert !ThreadUtils.runningOnUiThread(); Hmm. Why bother asserting if you're just posting to another thread? This doesn't do anything interesting anymore so I think it's fine to update callers to execute on UI thread (they're all currently using AsyncTasks). Oh now I see the comment below. So in the even that the process launcher thread isn't available, we execute synchronously? Maybe the best bet is to put that in an AsyncTask? https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:698: assert ret; erm, asserts kinda suck. They don't even work on art (there's a bug filed for this) but can you make this throw a RuntimeException instead (same throughout your CL) https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:885: sOnTesting = onTesting; I'm not thrilled with this because it's no longer testing how things normally work. Could try starting up the native PROCESS_LAUNCHER thread: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... ?
On 2015/09/04 16:04:04, Yaron wrote: > Honestly I'm struggling a bit with the review. It's hard to tell which > operations should be on what thread and when you need to perform a thread hop > (particularly in BindingManagerImpl) because there are so many locks already and > we're adding additional thread hops. Naively, I had thought that if we change to > a more strict threading model, we'd be able to remove some of the existing > locking. > > At a minimum you need to update the CL description with more information about > your change. Ideally though this can be captured in code. I've left a few > comments, but let me try and be more constructive: > - ChildProcessLauncher mostly makes sense. For the most part it lives on the > PROCESS_LAUNCHER thread but it gets signals from the UI thread and various > callbacks so those need to be posted. > - Perhaps use @NonThreadSafe and calledOnValidThread to start making it clear > which operations are on what threads? > - I think BindingManagerImpl may need to be broken up although perhaps that can > wait. I left more comments in line about things that were confusing > > https://codereview.chromium.org/1307793003/diff/100001/content/browser/androi... > File content/browser/android/child_process_launcher_android.cc (right): > > https://codereview.chromium.org/1307793003/diff/100001/content/browser/androi... > content/browser/android/child_process_launcher_android.cc:237: jboolean > RunOnLauncherThread(JNIEnv* env, jclass clazz, jobject runnable) { > I think you're going to have to rebase on torne's change and make this a JavaRef > and call .obj within this function instead of in the caller. > > https://codereview.chromium.org/1307793003/diff/100001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java > (right): > > https://codereview.chromium.org/1307793003/diff/100001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:47: > interface LauncherThreadExecutor { > This is basically a static but you're wrapping to avoid threading in tests > (presumably?). If we make the threading model actually work on tests, then I'd > prefer to see this be a static and avoid passing it around everywhere. > > https://codereview.chromium.org/1307793003/diff/100001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:72: > @Override > Here you're posting to launcher thread within the function but addConnection > you've done it in the caller. Can you do this more consistently so it's easier > to reason about? > > https://codereview.chromium.org/1307793003/diff/100001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:107: > int oldSize = size(); > Assuming you follow other suggestions, this would assert it's on launcher thread > (or use @NonThreadSafe and verify it's the right thread). > > https://codereview.chromium.org/1307793003/diff/100001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:176: > mHandler.postDelayed(mDelayedClearer, onTesting > This function is now very peculiar. We have an mHandler for the UIThread. We > post a task for it to run after some delay. As soon as it runs, it grabs a mutex > to ensure it can run but then posts back to the launcher thread to do its work. > I know there was confusion abou t loopers/handlers so it might not be possible > to post a delayed task for the current thread. (Maybe it's worth creating a > native wrapper for Runnable so you can post to the native message loop). > > Regardless of that, I'm not sure the mutex is doing the right thing anymore. > mDelayedClearer is now already cleared when the task is just posted to the > process launcher thread. Is evictAll guaranteed to do the right thing? > > https://codereview.chromium.org/1307793003/diff/100001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:263: > ThreadUtils.postOnUiThreadDelayed(new Runnable() { > General question: it seems like in many places we are now posting to the ui > thread and then immediately going to process_launcher. Why bother going through > the UI thread anymore? > > https://codereview.chromium.org/1307793003/diff/100001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:528: > }, MANAGED_CONNECTION_DELETE_DELAY_MILLS); > This was intentionally leaky (specifically for the case of mWasOomProtected). > See the class-level comments for ManagedConnection. Maybe that's not the best > strategy, but how did you choose 3 seconds? > > https://codereview.chromium.org/1307793003/diff/100001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java > (right): > > https://codereview.chromium.org/1307793003/diff/100001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:556: > assert !ThreadUtils.runningOnUiThread(); > Hmm. Why bother asserting if you're just posting to another thread? This doesn't > do anything interesting anymore so I think it's fine to update callers to > execute on UI thread (they're all currently using AsyncTasks). > > Oh now I see the comment below. So in the even that the process launcher thread > isn't available, we execute synchronously? Maybe the best bet is to put that in > an AsyncTask? > > https://codereview.chromium.org/1307793003/diff/100001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:698: > assert ret; > erm, asserts kinda suck. They don't even work on art (there's a bug filed for > this) but can you make this throw a RuntimeException instead (same throughout > your CL) > > https://codereview.chromium.org/1307793003/diff/100001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:885: > sOnTesting = onTesting; > I'm not thrilled with this because it's no longer testing how things normally > work. Could try starting up the native PROCESS_LAUNCHER thread: > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > ? FYI, while resolving Yaron's comments, I found that ChildProcessLauncher.isOomProtected can be called on main thread like the followings. #0 content::IsChildProcessOomProtected (handle=21611) at ../../content/browser/android/child_process_launcher_android.cc:158 #1 0x9e34a72a in content::ChildProcessLauncher::UpdateTerminationStatus ( this=0xa27892b0, known_dead=false) at ../../content/browser/child_process_launcher.cc:419 #2 0x9e34a9a2 in content::ChildProcessLauncher::GetChildTerminationStatus ( this=0xa27892b0, known_dead=false, exit_code=0xbef37cf4) at ../../content/browser/child_process_launcher.cc:531 #3 0x9e64f192 in content::RenderProcessHostImpl::ProcessDied ( this=0xb5085080, already_dead=false, known_details=0x0) at ../../content/browser/renderer_host/render_process_host_impl.cc:2167 #4 0x9e64d24a in content::RenderProcessHostImpl::FastShutdownIfPossible ( this=0xb5085080) at ../../content/browser/renderer_host/render_process_host_impl.cc:1531 #5 0x9e64e4a4 in content::RenderProcessHostImpl::FastShutdownForPageCount ( this=0xb5085080, count=1) at ../../content/browser/renderer_host/render_process_host_impl.cc:1870 #6 0x9af69b48 in TabAndroid::DestroyWebContents (this=0x94d93560, env=0xb5051240, obj=0xbef380cc, delete_native=1 '\001') at ../../chrome/browser/android/tab_android.cc:519 #7 0x9af66788 in Java_org_chromium_chrome_browser_tab_Tab_nativeDestroyWebContents (env=0xb5051240, jcaller=0xbef380cc, nativeTabAndroid=-1797704352, deleteNative=1 '\001') at gen/chrome/jni/Tab_jni.h:72 #8 0xa3915140 in ?? () I will handle this as well in the next patch set.
PTAL. https://codereview.chromium.org/1307793003/diff/100001/content/browser/androi... File content/browser/android/child_process_launcher_android.cc (right): https://codereview.chromium.org/1307793003/diff/100001/content/browser/androi... content/browser/android/child_process_launcher_android.cc:237: jboolean RunOnLauncherThread(JNIEnv* env, jclass clazz, jobject runnable) { On 2015/09/04 16:04:03, Yaron wrote: > I think you're going to have to rebase on torne's change and make this a JavaRef > and call .obj within this function instead of in the caller. Done. https://codereview.chromium.org/1307793003/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:47: interface LauncherThreadExecutor { On 2015/09/04 16:04:04, Yaron wrote: > This is basically a static but you're wrapping to avoid threading in tests > (presumably?). If we make the threading model actually work on tests, then I'd > prefer to see this be a static and avoid passing it around everywhere. This is only for BindingManagerImplTest which is actually unittests for BindingManagerImp; we don't need to test the threading model there. So I want to keep this logic. https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:72: @Override On 2015/09/04 16:04:04, Yaron wrote: > Here you're posting to launcher thread within the function but addConnection > you've done it in the caller. Can you do this more consistently so it's easier > to reason about? I separated ComponentCallbacks2 from ModerateBindingPool for clear understanding. https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:107: int oldSize = size(); On 2015/09/04 16:04:03, Yaron wrote: > Assuming you follow other suggestions, this would assert it's on launcher thread > (or use @NonThreadSafe and verify it's the right thread). Done. https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:176: mHandler.postDelayed(mDelayedClearer, onTesting On 2015/09/04 16:04:04, Yaron wrote: > This function is now very peculiar. We have an mHandler for the UIThread. We > post a task for it to run after some delay. As soon as it runs, it grabs a mutex > to ensure it can run but then posts back to the launcher thread to do its work. > I know there was confusion abou t loopers/handlers so it might not be possible > to post a delayed task for the current thread. (Maybe it's worth creating a > native wrapper for Runnable so you can post to the native message loop). > > Regardless of that, I'm not sure the mutex is doing the right thing anymore. > mDelayedClearer is now already cleared when the task is just posted to the > process launcher thread. Is evictAll guaranteed to do the right thing? I will add a native wrapper for posting, but in this case I want to use java Handler because the task can be canceled. https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:263: ThreadUtils.postOnUiThreadDelayed(new Runnable() { On 2015/09/04 16:04:03, Yaron wrote: > General question: it seems like in many places we are now posting to the ui > thread and then immediately going to process_launcher. Why bother going through > the UI thread anymore? Added a native wrapper to post a task on launcher thread. https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:528: }, MANAGED_CONNECTION_DELETE_DELAY_MILLS); On 2015/09/04 16:04:04, Yaron wrote: > This was intentionally leaky (specifically for the case of mWasOomProtected). > See the class-level comments for ManagedConnection. Maybe that's not the best > strategy, but how did you choose 3 seconds? I picked 3s because I thought that it was big enough to keep the closed connection for the last access. Is there any case to access the closed connection other than checking mWasOomProtected to show a sad tab? https://codereview.chromium.org/1307793003/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:556: assert !ThreadUtils.runningOnUiThread(); On 2015/09/04 16:04:04, Yaron wrote: > Hmm. Why bother asserting if you're just posting to another thread? This doesn't > do anything interesting anymore so I think it's fine to update callers to > execute on UI thread (they're all currently using AsyncTasks). > > Oh now I see the comment below. So in the even that the process launcher thread > isn't available, we execute synchronously? Maybe the best bet is to put that in > an AsyncTask? Done. https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:698: assert ret; On 2015/09/04 16:04:04, Yaron wrote: > erm, asserts kinda suck. They don't even work on art (there's a bug filed for > this) but can you make this throw a RuntimeException instead (same throughout > your CL) Done. https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:885: sOnTesting = onTesting; On 2015/09/04 16:04:04, Yaron wrote: > I'm not thrilled with this because it's no longer testing how things normally > work. Could try starting up the native PROCESS_LAUNCHER thread: > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > ? This is only for ChildProcessLauncherTest which is actually unittests for ChildProcessLauncher; it doesn't test how things normally work anyway. So I want keep this.
Ugh, sorry I'm still struggling to review this and have confidence in its correctness. There are too many internal classes with access to each other and the threading model is hard to reason about. I think that breaking that down would actually make this a lot easier to review/reason about. wdyt? https://codereview.chromium.org/1307793003/diff/100001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:72: @Override On 2015/09/08 09:45:12, Jaekyun Seok wrote: > On 2015/09/04 16:04:04, Yaron wrote: > > Here you're posting to launcher thread within the function but addConnection > > you've done it in the caller. Can you do this more consistently so it's easier > > to reason about? > > I separated ComponentCallbacks2 from ModerateBindingPool for clear > understanding. Nice, this helps https://codereview.chromium.org/1307793003/diff/100001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:528: }, MANAGED_CONNECTION_DELETE_DELAY_MILLS); On 2015/09/08 09:45:12, Jaekyun Seok wrote: > On 2015/09/04 16:04:04, Yaron wrote: > > This was intentionally leaky (specifically for the case of mWasOomProtected). > > See the class-level comments for ManagedConnection. Maybe that's not the best > > strategy, but how did you choose 3 seconds? > > I picked 3s because I thought that it was big enough to keep the closed > connection for the last access. > Is there any case to access the closed connection other than checking > mWasOomProtected to show a sad tab? I think that's the only reason. Still, I think we have enough going on in this CL that we can do this separately https://codereview.chromium.org/1307793003/diff/120001/content/browser/androi... File content/browser/android/child_process_launcher_android.cc (right): https://codereview.chromium.org/1307793003/diff/120001/content/browser/androi... content/browser/android/child_process_launcher_android.cc:244: if (jrunnable.is_null()) Does this actually happen? https://codereview.chromium.org/1307793003/diff/120001/content/browser/androi... content/browser/android/child_process_launcher_android.cc:247: if (!BrowserThread::IsThreadInitialized(BrowserThread::PROCESS_LAUNCHER)) Is this also because of tests? Or can this actually happen too early? The reason I ask is because of the complexity of the |asserted| flag you added on the java side. I'd prefer if we can get rid of that and assume that this is always successful https://codereview.chromium.org/1307793003/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (left): https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:340: // Synchronizes operations that access mLastInForeground: setInForeground() and This removal looks good https://codereview.chromium.org/1307793003/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:119: mDelayedClearer = new Runnable() { I think you were following my suggestion but as-is it's still accessed on multiple threads so the lock is needed. What I was hoping for was getting rid of mDelayedClearer (or having all mutations on one thread) and then you can get rid of the lock but if you need the cancellation on UI thread that means this is racey. https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:208: doUnbind.run(); doesn't this affect mModerateBindingPool on a different thread now? https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:286: ChildProcessConnection connection = mConnection; Why was this changed? https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:328: void checkIfCalledOnValidThread() { why not re-use NonThreadSafe? Because of the testing aspect again? https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:493: /** @return true iff the connection reference is no longer held */ Also called on any thread? https://codereview.chromium.org/1307793003/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:562: * in parallel to other startup work. Must not be called on the UI thread. Spare connection is Update to match code
> Ugh, sorry I'm still struggling to review this and have confidence in its > correctness. There are too many internal classes with access to each other and > the threading model is hard to reason about. I think that breaking that down > would actually make this a lot easier to review/reason about. wdyt? All the operations inside BindingManagerImpl happen on launcher thread except isOomProtected(), which is guaranteed by checkIfCalledOnValidThread on all public methods. Could you please elaborate more which part is unclear? > I think that's the only reason. Still, I think we have enough going on in this > CL that we can do this separately I will leave only TODO for later cleanup. https://codereview.chromium.org/1307793003/diff/120001/content/browser/androi... File content/browser/android/child_process_launcher_android.cc (right): https://codereview.chromium.org/1307793003/diff/120001/content/browser/androi... content/browser/android/child_process_launcher_android.cc:244: if (jrunnable.is_null()) On 2015/09/09 01:00:20, Yaron wrote: > Does this actually happen? No. I will remove this. https://codereview.chromium.org/1307793003/diff/120001/content/browser/androi... content/browser/android/child_process_launcher_android.cc:247: if (!BrowserThread::IsThreadInitialized(BrowserThread::PROCESS_LAUNCHER)) On 2015/09/09 01:00:20, Yaron wrote: > Is this also because of tests? Or can this actually happen too early? The reason > I ask is because of the complexity of the |asserted| flag you added on the java > side. I'd prefer if we can get rid of that and assume that this is always > successful This happens actually when ChildProcessLauncher.warmUp() is called very early. https://codereview.chromium.org/1307793003/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:119: mDelayedClearer = new Runnable() { On 2015/09/09 01:00:20, Yaron wrote: > I think you were following my suggestion but as-is it's still accessed on > multiple threads so the lock is needed. > What I was hoping for was getting rid of mDelayedClearer (or having all > mutations on one thread) and then you can get rid of the lock but if you need > the cancellation on UI thread that means this is racey. mDelayedClearer is only accessed on launcher thread because onSentToBackground() and onBroughtToForeground() are called on launcher thread. https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:208: doUnbind.run(); On 2015/09/09 01:00:20, Yaron wrote: > doesn't this affect mModerateBindingPool on a different thread now? No. All accesses to mModerateBindingPool happen on launcher thread, and all accesses to ManagedConnection except isOomProtected() happen on launcher thread as well. https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:286: ChildProcessConnection connection = mConnection; On 2015/09/09 01:00:21, Yaron wrote: > Why was this changed? isOomProtected() could be called from any thread, and so mConnection could be null unexpectedly. https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:328: void checkIfCalledOnValidThread() { On 2015/09/09 01:00:20, Yaron wrote: > why not re-use NonThreadSafe? Because of the testing aspect again? The class can't be used here because it is under chrome/. Moreover its initial thread id is determined in creator, which isn't proper in this case. https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:493: /** @return true iff the connection reference is no longer held */ On 2015/09/09 01:00:20, Yaron wrote: > Also called on any thread? Yes. https://codereview.chromium.org/1307793003/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:562: * in parallel to other startup work. Must not be called on the UI thread. Spare connection is On 2015/09/09 01:00:21, Yaron wrote: > Update to match code Done.
PTAL.
PTAL. I added checkIfCalledOnValidThread into methods of ModerateBindingPool and ManagedConnection as well to make things clear.
PTAL. I removed any direct access between ModerateBindingPool and ManagedConnection. And so all accesses to them happen from BindingManagerImpl explicitly.
On 2015/09/10 03:22:24, Jaekyun Seok wrote: > PTAL. > > I removed any direct access between ModerateBindingPool and ManagedConnection. > And so all accesses to them happen from BindingManagerImpl explicitly. Yaron told me that he isn't able to see this CL until tomorrow. Daniel, could you please take a look at this CL?
https://codereview.chromium.org/1307793003/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:286: ChildProcessConnection connection = mConnection; On 2015/09/09 02:02:41, Jaekyun Seok wrote: > On 2015/09/09 01:00:21, Yaron wrote: > > Why was this changed? > > isOomProtected() could be called from any thread, and so mConnection could be > null unexpectedly. Right. Hmm, so shouldn't mConnection be declared as volatile then? https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:328: void checkIfCalledOnValidThread() { On 2015/09/09 02:02:41, Jaekyun Seok wrote: > On 2015/09/09 01:00:20, Yaron wrote: > > why not re-use NonThreadSafe? Because of the testing aspect again? > > The class can't be used here because it is under chrome/. Moreover its initial > thread id is determined in creator, which isn't proper in this case. Well the simple solution would be to move it out of chrome/. It's not clear to me why it's there and not in base/. I see what you're saying though cause the binding manager is static so it's hard to control it being created on the process launcher thread https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:493: /** @return true iff the connection reference is no longer held */ On 2015/09/09 02:02:42, Jaekyun Seok wrote: > On 2015/09/09 01:00:20, Yaron wrote: > > Also called on any thread? > > Yes. add comment https://codereview.chromium.org/1307793003/diff/220001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/220001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:323: void checkIfCalledOnValidThread() { If you are defining a custom instance, let's name this more clearly: checkCalledOnProcessLauncherThread.
PTAL. https://codereview.chromium.org/1307793003/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:286: ChildProcessConnection connection = mConnection; On 2015/09/11 22:15:24, Yaron wrote: > On 2015/09/09 02:02:41, Jaekyun Seok wrote: > > On 2015/09/09 01:00:21, Yaron wrote: > > > Why was this changed? > > > > isOomProtected() could be called from any thread, and so mConnection could be > > null unexpectedly. > > Right. Hmm, so shouldn't mConnection be declared as volatile then? Done. https://codereview.chromium.org/1307793003/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:493: /** @return true iff the connection reference is no longer held */ On 2015/09/11 22:15:24, Yaron wrote: > On 2015/09/09 02:02:42, Jaekyun Seok wrote: > > On 2015/09/09 01:00:20, Yaron wrote: > > > Also called on any thread? > > > > Yes. > > add comment Done. https://codereview.chromium.org/1307793003/diff/220001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/220001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:323: void checkIfCalledOnValidThread() { On 2015/09/11 22:15:24, Yaron wrote: > If you are defining a custom instance, let's name this more clearly: > checkCalledOnProcessLauncherThread. Done.
I think we're getting close. sievers@ worth looking now https://codereview.chromium.org/1307793003/diff/230001/content/browser/androi... File content/browser/android/child_process_launcher_android.cc (right): https://codereview.chromium.org/1307793003/diff/230001/content/browser/androi... content/browser/android/child_process_launcher_android.cc:91: static void OnChildProcessStarted(JNIEnv*, This is now always happening on the process_launcher thread but note the comment in child_process_launcher.cc. Please remove the comment and the posted task. https://codereview.chromium.org/1307793003/diff/230001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:48: private class ModerateBindingPool extends LruCache<Integer, ChildProcessConnection> { Can you add a brief comment on this? It should explain that this lives on ProcessLauncher thread and what it actually does. https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:484: synchronized (mManagedConnections) { so interestingly, this isn't unlike the synchronization on mConnection which we made volatile rather than locking https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:491: public void clearConnection(final int pid) { why is this final now? https://codereview.chromium.org/1307793003/diff/230001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:519: sBindingManager.onSentToBackground(); Why is this not happening in the case below ok? Because the thread isn't set up yet so we couldn't have created a binding manager? https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:566: new AsyncTask<Void, Void, Void>() { Two of the callers to this are trivial to switch to not using AsyncTask. The other already has a TODO to clean it up. https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:572: }.execute(); Use AsyncTask.THREAD_POOL_EXECUTOR https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:579: synchronized (ChildProcessLauncher.class) { Darn - if we didn't have the failure case below, we could get rid of this synchronization too. https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:916: Runnable runnable, long delayInMS, boolean asserted) { s/asserted/assertOnPostingFailure/ (also in above function) https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:927: boolean ret = nativeRunOnLauncherThread(runnable, delayInMS); s/ret/wasPosted/
PTAL. https://codereview.chromium.org/1307793003/diff/230001/content/browser/androi... File content/browser/android/child_process_launcher_android.cc (right): https://codereview.chromium.org/1307793003/diff/230001/content/browser/androi... content/browser/android/child_process_launcher_android.cc:91: static void OnChildProcessStarted(JNIEnv*, On 2015/09/14 22:30:59, Yaron wrote: > This is now always happening on the process_launcher thread but note the comment > in child_process_launcher.cc. Please remove the comment and the posted task. Done. https://codereview.chromium.org/1307793003/diff/230001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:48: private class ModerateBindingPool extends LruCache<Integer, ChildProcessConnection> { On 2015/09/14 22:30:59, Yaron wrote: > Can you add a brief comment on this? It should explain that this lives on > ProcessLauncher thread and what it actually does. Done. https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:484: synchronized (mManagedConnections) { On 2015/09/14 22:30:59, Yaron wrote: > so interestingly, this isn't unlike the synchronization on mConnection which we > made volatile rather than locking This is to get a correct managed connection. And then ManagedConnection.isOomProtected() will access the volatile mConnection. https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:491: public void clearConnection(final int pid) { On 2015/09/14 22:30:59, Yaron wrote: > why is this final now? Removed. https://codereview.chromium.org/1307793003/diff/230001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java (right): https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:519: sBindingManager.onSentToBackground(); On 2015/09/14 22:30:59, Yaron wrote: > Why is this not happening in the case below ok? Because the thread isn't set up > yet so we couldn't have created a binding manager? There will be no binding in this case. https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:566: new AsyncTask<Void, Void, Void>() { On 2015/09/14 22:30:59, Yaron wrote: > Two of the callers to this are trivial to switch to not using AsyncTask. The > other already has a TODO to clean it up. Done. https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:572: }.execute(); On 2015/09/14 22:30:59, Yaron wrote: > Use AsyncTask.THREAD_POOL_EXECUTOR Done. https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:579: synchronized (ChildProcessLauncher.class) { On 2015/09/14 22:30:59, Yaron wrote: > Darn - if we didn't have the failure case below, we could get rid of this > synchronization too. I left TODO for the later investigation to avoid the failure case. https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:916: Runnable runnable, long delayInMS, boolean asserted) { On 2015/09/14 22:30:59, Yaron wrote: > s/asserted/assertOnPostingFailure/ > > (also in above function) Done. https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:927: boolean ret = nativeRunOnLauncherThread(runnable, delayInMS); On 2015/09/14 22:30:59, Yaron wrote: > s/ret/wasPosted/ Done.
https://codereview.chromium.org/1307793003/diff/230001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:484: synchronized (mManagedConnections) { On 2015/09/14 23:44:11, Jaekyun Seok wrote: > On 2015/09/14 22:30:59, Yaron wrote: > > so interestingly, this isn't unlike the synchronization on mConnection which > we > > made volatile rather than locking > > This is to get a correct managed connection. And then > ManagedConnection.isOomProtected() will access the volatile mConnection. But my question is whether mManagedConnections could've also been made volatile for the same reasons and still be correct? https://codereview.chromium.org/1307793003/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java (left): https://codereview.chromium.org/1307793003/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:76: // TODO(yusufo) : Investigate using an AsyncTask for this. Sorry if I was unclear - I actually meant this is fine to leave as-is since it has a TODO to fix which is good but I don't think it should be done in the same CL. The other related changes are very straightforward and make sense to include. https://codereview.chromium.org/1307793003/diff/250001/content/browser/child_... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/1307793003/diff/250001/content/browser/child_... content/browser/child_process_launcher.cc:74: // TODO(sievers): Remove this by defining better what happens on what Remove TODO
PTAL. https://codereview.chromium.org/1307793003/diff/230001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:484: synchronized (mManagedConnections) { On 2015/09/16 01:20:16, Yaron wrote: > On 2015/09/14 23:44:11, Jaekyun Seok wrote: > > On 2015/09/14 22:30:59, Yaron wrote: > > > so interestingly, this isn't unlike the synchronization on mConnection which > > we > > > made volatile rather than locking > > > > This is to get a correct managed connection. And then > > ManagedConnection.isOomProtected() will access the volatile mConnection. > > But my question is whether mManagedConnections could've also been made volatile > for the same reasons and still be correct? SparseArray isn't thread-safe, and so I believe that we should use a lock to put/get an element properly. https://codereview.chromium.org/1307793003/diff/250001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java (left): https://codereview.chromium.org/1307793003/diff/250001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:76: // TODO(yusufo) : Investigate using an AsyncTask for this. On 2015/09/16 01:20:16, Yaron wrote: > Sorry if I was unclear - I actually meant this is fine to leave as-is since it > has a TODO to fix which is good but I don't think it should be done in the same > CL. The other related changes are very straightforward and make sense to > include. Reverted. https://codereview.chromium.org/1307793003/diff/250001/content/browser/child_... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/1307793003/diff/250001/content/browser/child_... content/browser/child_process_launcher.cc:74: // TODO(sievers): Remove this by defining better what happens on what On 2015/09/16 01:20:16, Yaron wrote: > Remove TODO Done.
https://codereview.chromium.org/1307793003/diff/230001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:484: synchronized (mManagedConnections) { On 2015/09/16 01:46:47, Jaekyun Seok wrote: > On 2015/09/16 01:20:16, Yaron wrote: > > On 2015/09/14 23:44:11, Jaekyun Seok wrote: > > > On 2015/09/14 22:30:59, Yaron wrote: > > > > so interestingly, this isn't unlike the synchronization on mConnection > which > > > we > > > > made volatile rather than locking > > > > > > This is to get a correct managed connection. And then > > > ManagedConnection.isOomProtected() will access the volatile mConnection. > > > > But my question is whether mManagedConnections could've also been made > volatile > > for the same reasons and still be correct? > > SparseArray isn't thread-safe, and so I believe that we should use a lock to > put/get an element properly. I believe so because accessing a volatile variable itself is atomic, but it doesn't mean that operating inside it also becomes atomic automatically.
https://codereview.chromium.org/1307793003/diff/230001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java (right): https://codereview.chromium.org/1307793003/diff/230001/content/public/android... content/public/android/java/src/org/chromium/content/browser/BindingManagerImpl.java:484: synchronized (mManagedConnections) { On 2015/09/16 02:20:33, Jaekyun Seok wrote: > On 2015/09/16 01:46:47, Jaekyun Seok wrote: > > On 2015/09/16 01:20:16, Yaron wrote: > > > On 2015/09/14 23:44:11, Jaekyun Seok wrote: > > > > On 2015/09/14 22:30:59, Yaron wrote: > > > > > so interestingly, this isn't unlike the synchronization on mConnection > > which > > > > we > > > > > made volatile rather than locking > > > > > > > > This is to get a correct managed connection. And then > > > > ManagedConnection.isOomProtected() will access the volatile mConnection. > > > > > > But my question is whether mManagedConnections could've also been made > > volatile > > > for the same reasons and still be correct? > > > > SparseArray isn't thread-safe, and so I believe that we should use a lock to > > put/get an element properly. > > I believe so because accessing a volatile variable itself is atomic, but it > doesn't mean that operating inside it also becomes atomic automatically. Right, makes sense.
Yaron, do you have any remaining concern? Or could you please give LGTM to this CL?
On 2015/09/17 21:50:36, Jaekyun Seok wrote: > Yaron, do you have any remaining concern? Or could you please give LGTM to this > CL? I think you've satisfied my concerns but I'd prefer to have another set of eyes given upcoming changes.
On 2015/09/18 16:49:43, Yaron wrote: > On 2015/09/17 21:50:36, Jaekyun Seok wrote: > > Yaron, do you have any remaining concern? Or could you please give LGTM to > this > > CL? > > I think you've satisfied my concerns but I'd prefer to have another set of eyes > given upcoming changes. Maria and Daniel, could you please review this CL?
On 2015/09/19 03:37:08, Jaekyun Seok wrote: > On 2015/09/18 16:49:43, Yaron wrote: > > On 2015/09/17 21:50:36, Jaekyun Seok wrote: > > > Yaron, do you have any remaining concern? Or could you please give LGTM to > > this > > > CL? > > > > I think you've satisfied my concerns but I'd prefer to have another set of > eyes > > given upcoming changes. > > Maria and Daniel, could you please review this CL? Ping?
jaekyun@chromium.org changed reviewers: + jdduke@chromium.org
Hi Jared, I've been trying to land this CL for a month, but it is still in review. Please take a look at this CL as an owner of process management. Or you can take over the original issue and write your own CL. Thanks,
Ping?
On 2015/10/05 00:41:52, Jaekyun Seok wrote: > Ping? Hey, thanks for following up on this, and sorry for the delayed reply. Sounds great to leave it with Jared to take it over. We talked last week, and did some sort of brainstorming and ended up wondering if it's time to revisit some of the very original code and assumptions here. It's gotten very complicated (lots of classes and interactions), while at the same time it's not really abstracted well in a way that deals with threading or visibility (it's really hardcoded to renderers, and then there is more hardcoding for low-end device hacks).
On 2015/10/05 17:56:37, sievers wrote: > On 2015/10/05 00:41:52, Jaekyun Seok wrote: > > Ping? > > Hey, thanks for following up on this, and sorry for the delayed reply. > Sounds great to leave it with Jared to take it over. We talked last week, and > did some sort of brainstorming and ended up wondering if it's time to revisit > some of the very original code and assumptions here. It's gotten very > complicated (lots of classes and interactions), while at the same time it's not > really abstracted well in a way that deals with threading or visibility (it's > really hardcoded to renderers, and then there is more hardcoding for low-end > device hacks). I see. I will close this CL and hand over the original issue to Jared. |
