Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
913203004:
Clean up Android dangling g_child_thread pointer on shutdown. (Closed)
|
Created:
5 years, 10 months ago by aelias_OOO_until_Jul13 Modified:
5 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClean up Android dangling g_child_thread pointer on shutdown.
In the GPU process, GpuMain can return, destroying the process and
thread, before ChildProcessService.onDestroy (which calls
ShutdownThread) runs, leaving g_child_thread invalid. Set it to null on
ChildThreadImpl destructor and check for that.
BUG=458868
Committed: https://crrev.com/ed97167c14dc05f071895ec08848485d073d312a
Cr-Commit-Position: refs/heads/master@{#316979}
Patch Set 1 #Patch Set 2 : Make lock affect all code in ShutdownThread #Patch Set 3 : Move nulling to top of destructor #
Total comments: 13
Patch Set 4 : sievers@ code review comments #Patch Set 5 : Change comment text to address previous code review feedback #Messages
Total messages: 17 (5 generated)
aelias@chromium.org changed reviewers: + feng@chromium.org
https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... File content/child/child_thread_impl.cc (right): https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... content/child/child_thread_impl.cc:616: return; Seems that only path to get here is that ChildThreadImpl::~ChildThreadImpl called before ChildThreadImpl::ShutdownThread(), but this shouldn't happen.
lgtm
aelias@chromium.org changed reviewers: + sievers@chromium.org
Daniel, does this look sane to you as well?
https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... File content/child/child_thread_impl.cc (right): https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... content/child/child_thread_impl.cc:616: return; On 2015/02/18 at 01:47:32, Feng Qian wrote: > Seems that only path to get here is that ChildThreadImpl::~ChildThreadImpl called before ChildThreadImpl::ShutdownThread(), but this shouldn't happen. As discussed offline, ~ChildThreadImpl is called from one thread from GpuMain(), while ShutdownThread is called from Java on the Android UI thread, so it seems they're racing each other and it can happen.
https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... File content/child/child_thread_impl.cc (right): https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... content/child/child_thread_impl.cc:610: base::AutoLock lock(g_lazy_child_thread_lock.Get()); What about the two lazy instances (lock and cv), could they be also gone when we get here, since they both get destroyed when the AtExit handler goes out of scope (which I think is when the ContentMainRunner gets deleted)? https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... content/child/child_thread_impl.cc:614: // g_child_thread may already have been destructed elsewhere. nit: "...may already have been destructed while we didn't hold the lock" would make it more obvious and also explain why we need the extra variable (g_child_thread_initialized).
https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... File content/child/child_thread_impl.cc (right): https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... content/child/child_thread_impl.cc:610: base::AutoLock lock(g_lazy_child_thread_lock.Get()); On 2015/02/18 at 19:19:42, sievers wrote: > What about the two lazy instances (lock and cv), could they be also gone when we get here, since they both get destroyed when the AtExit handler goes out of scope (which I think is when the ContentMainRunner gets deleted)? One hint FWIW is that this is a preexisting problem: the fuzzer didn't crash on the lazy instance, it crashed on the g_child_thread. Does that prove that this LazyInstance is OK? I'm not familiar with the AtExit handler.
https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... File content/child/child_thread_impl.cc (right): https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... content/child/child_thread_impl.cc:167: base::LazyInstance<base::Lock> g_lazy_child_thread_lock = base::LazyInstance<base::Lock>::Leaky https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... content/child/child_thread_impl.cc:174: static const bool kRegisterOnExit = true; kRegisterOnExit = false; should have the same effect as ::Leaky. (I guess it would be more straightforward to wrap ConditionVariable in another class than overriding all the traits and new/delete function here.) https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... content/child/child_thread_impl.cc:176: static const bool kAllowedToAccessOnNonjoinableThread = false; Hmm, this might be wrong too since the thread we call Shutdown() from (Java UI thread) is likely not set as joinable and could cause an assertion in debug builds. https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... content/child/child_thread_impl.cc:610: base::AutoLock lock(g_lazy_child_thread_lock.Get()); On 2015/02/18 19:37:27, aelias wrote: > On 2015/02/18 at 19:19:42, sievers wrote: > > What about the two lazy instances (lock and cv), could they be also gone when > we get here, since they both get destroyed when the AtExit handler goes out of > scope (which I think is when the ContentMainRunner gets deleted)? > > One hint FWIW is that this is a preexisting problem: the fuzzer didn't crash on > the lazy instance, it crashed on the g_child_thread. Does that prove that this > LazyInstance is OK? I'm not familiar with the AtExit handler. I think it's wrong that we access the lazy instances here while we are not on the thread where they get deleted. We can just make them leaky, see above.
New patchsets have been uploaded after l-g-t-m from feng@chromium.org
https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... File content/child/child_thread_impl.cc (right): https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... content/child/child_thread_impl.cc:167: base::LazyInstance<base::Lock> g_lazy_child_thread_lock = On 2015/02/18 20:48:30, sievers wrote: > base::LazyInstance<base::Lock>::Leaky Done. https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... content/child/child_thread_impl.cc:174: static const bool kRegisterOnExit = true; On 2015/02/18 20:48:30, sievers wrote: > kRegisterOnExit = false; > > should have the same effect as ::Leaky. > > (I guess it would be more straightforward to wrap ConditionVariable in another > class than overriding all the traits and new/delete function here.) OK, set to "false". I stayed with the traits to keep this patch minimal as we plan to cherry-pick it. https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... content/child/child_thread_impl.cc:176: static const bool kAllowedToAccessOnNonjoinableThread = false; On 2015/02/18 20:48:30, sievers wrote: > Hmm, this might be wrong too since the thread we call Shutdown() from (Java UI > thread) is likely not set as joinable and could cause an assertion in debug > builds. Changed to "true". https://codereview.chromium.org/913203004/diff/40001/content/child/child_thre... content/child/child_thread_impl.cc:610: base::AutoLock lock(g_lazy_child_thread_lock.Get()); On 2015/02/18 20:48:30, sievers wrote: > On 2015/02/18 19:37:27, aelias wrote: > > On 2015/02/18 at 19:19:42, sievers wrote: > > > What about the two lazy instances (lock and cv), could they be also gone > when > > we get here, since they both get destroyed when the AtExit handler goes out of > > scope (which I think is when the ContentMainRunner gets deleted)? > > > > One hint FWIW is that this is a preexisting problem: the fuzzer didn't crash > on > > the lazy instance, it crashed on the g_child_thread. Does that prove that > this > > LazyInstance is OK? I'm not familiar with the AtExit handler. > > I think it's wrong that we access the lazy instances here while we are not on > the thread where they get deleted. We can just make them leaky, see above. > > Done.
lgtm
New patchsets have been uploaded after l-g-t-m from sievers@chromium.org
The CQ bit was checked by aelias@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/913203004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ed97167c14dc05f071895ec08848485d073d312a Cr-Commit-Position: refs/heads/master@{#316979} |
