Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2525213002:
Mus: Avoid deadlock during teardown (Closed)
|
Created:
4 years ago by Fady Samuel Modified:
4 years ago CC:
chromium-reviews, rjkroege, piman+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMus: Avoid deadlock during teardown
The display compositor blocks on the gpu thread to tear down. If
GpuServiceInternal tears down before display compositor has a chance
to tear down then Mus may crash during tear down or deadlock waiting
for a task to execute that never will.
BUG=668105
TBR=sadrul@chromium.org, sky@chromium.org
NOTRY=true
NOPRESUBMIT=true
Committed: https://crrev.com/5059e49d0a21c1c311653c1613de16b27aefc963
Cr-Commit-Position: refs/heads/master@{#434310}
Patch Set 1 #Patch Set 2 : Don't call DestroyDisplayCompositor during GpuServiceInternal destruction #
Total comments: 2
Patch Set 3 : Address Sadrul's comment #
Messages
Total messages: 44 (25 generated)
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
fsamuel@chromium.org changed reviewers: + sadrul@chromium.org, sky@chromium.org
I'm TBR'ing you both since this is causing a bunch of bot flake and I wasn't able to get to it earlier today.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Mus: Avoid deadlock during teardown The display compositor blocks on the gpu thread to tear down. If GpuServiceInternal tears down before display compositor has a chance to tear down then Mus may crash during tear down or deadlock waiting for a task to execute that never will. BUG=668105 ========== to ========== Mus: Avoid deadlock during teardown The display compositor blocks on the gpu thread to tear down. If GpuServiceInternal tears down before display compositor has a chance to tear down then Mus may crash during tear down or deadlock waiting for a task to execute that never will. BUG=668105 TBR=sadrul@chromium.org, sky@chromium.org ==========
The CQ bit was checked by fsamuel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2525213002/diff/20001/services/ui/gpu/gpu_mai... File services/ui/gpu/gpu_main.cc (right): https://codereview.chromium.org/2525213002/diff/20001/services/ui/gpu/gpu_mai... services/ui/gpu/gpu_main.cc:132: gpu_service_internal_->DestroyDisplayCompositor(); It is possible to be here without having the |gpu_service_internal_| been created at all, I think? (e.g. gpu-thread hasn't run InitOnGpuThread() yet, which means gpu_service_internal_ is still uninitialized) Can we move DisplayCompositor ownership out of GpuServiceInternal, and into here in GpuMain instead?
The CQ bit was unchecked by fsamuel@chromium.org
PTAL https://codereview.chromium.org/2525213002/diff/20001/services/ui/gpu/gpu_mai... File services/ui/gpu/gpu_main.cc (right): https://codereview.chromium.org/2525213002/diff/20001/services/ui/gpu/gpu_mai... services/ui/gpu/gpu_main.cc:132: gpu_service_internal_->DestroyDisplayCompositor(); On 2016/11/24 05:07:17, sadrul wrote: > It is possible to be here without having the |gpu_service_internal_| been > created at all, I think? (e.g. gpu-thread hasn't run InitOnGpuThread() yet, > which means gpu_service_internal_ is still uninitialized) > > Can we move DisplayCompositor ownership out of GpuServiceInternal, and into here > in GpuMain instead? The problem is "CreateDisplayCompositor" is part of the GpuServiceInternal interface. I have filed a bug to pull that out: https://bugs.chromium.org/p/chromium/issues/detail?id=668357 I'll check that gpu_service_internal_ is not null here, and do the right thing tomorrow or Friday.
The CQ bit was checked by fsamuel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/24 05:21:55, Fady Samuel wrote: > PTAL > > https://codereview.chromium.org/2525213002/diff/20001/services/ui/gpu/gpu_mai... > File services/ui/gpu/gpu_main.cc (right): > > https://codereview.chromium.org/2525213002/diff/20001/services/ui/gpu/gpu_mai... > services/ui/gpu/gpu_main.cc:132: > gpu_service_internal_->DestroyDisplayCompositor(); > On 2016/11/24 05:07:17, sadrul wrote: > > It is possible to be here without having the |gpu_service_internal_| been > > created at all, I think? (e.g. gpu-thread hasn't run InitOnGpuThread() yet, > > which means gpu_service_internal_ is still uninitialized) > > > > Can we move DisplayCompositor ownership out of GpuServiceInternal, and into > here > > in GpuMain instead? > > The problem is "CreateDisplayCompositor" is part of the GpuServiceInternal > interface. I have filed a bug to pull that out: > https://bugs.chromium.org/p/chromium/issues/detail?id=668357 > > I'll check that gpu_service_internal_ is not null here, and do the right thing > tomorrow or Friday. Sounds good. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by kolos@chromium.org
The CQ bit was unchecked by kolos@chromium.org
On 2016/11/24 08:42:33, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux > (JOB_TIMED_OUT, no build URL) Hmmm... this CL cannot pass the tryservers because of the infra issue. Let me try to land this using NOTRY and monitor the waterfall.
Description was changed from ========== Mus: Avoid deadlock during teardown The display compositor blocks on the gpu thread to tear down. If GpuServiceInternal tears down before display compositor has a chance to tear down then Mus may crash during tear down or deadlock waiting for a task to execute that never will. BUG=668105 TBR=sadrul@chromium.org, sky@chromium.org ========== to ========== Mus: Avoid deadlock during teardown The display compositor blocks on the gpu thread to tear down. If GpuServiceInternal tears down before display compositor has a chance to tear down then Mus may crash during tear down or deadlock waiting for a task to execute that never will. BUG=668105 TBR=sadrul@chromium.org, sky@chromium.org NOTRY=true ==========
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
On 2016/11/24 10:04:14, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build > URL) Oh... NOTRY couldn't get over the presubmit task :( kolos@ (the next sheriff): could you take over this?
On 2016/11/24 10:08:50, nhiroki (slow-sheriff) wrote: > On 2016/11/24 10:04:14, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no > build > > URL) > > Oh... NOTRY couldn't get over the presubmit task :( > > kolos@ (the next sheriff): could you take over this? I take it.
Description was changed from ========== Mus: Avoid deadlock during teardown The display compositor blocks on the gpu thread to tear down. If GpuServiceInternal tears down before display compositor has a chance to tear down then Mus may crash during tear down or deadlock waiting for a task to execute that never will. BUG=668105 TBR=sadrul@chromium.org, sky@chromium.org NOTRY=true ========== to ========== Mus: Avoid deadlock during teardown The display compositor blocks on the gpu thread to tear down. If GpuServiceInternal tears down before display compositor has a chance to tear down then Mus may crash during tear down or deadlock waiting for a task to execute that never will. BUG=668105 TBR=sadrul@chromium.org, sky@chromium.org NOTRY=true NOPRESUBMIT=true ==========
The CQ bit was checked by kolos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by kolos@chromium.org
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479983115867350,
"parent_rev": "ff78f2380388747eddc2c60c8d05bb51819fdca9", "commit_rev":
"c37974c615f4e6fcff7a438e209b686d336923b0"}
Description was changed from ========== Mus: Avoid deadlock during teardown The display compositor blocks on the gpu thread to tear down. If GpuServiceInternal tears down before display compositor has a chance to tear down then Mus may crash during tear down or deadlock waiting for a task to execute that never will. BUG=668105 TBR=sadrul@chromium.org, sky@chromium.org NOTRY=true NOPRESUBMIT=true ========== to ========== Mus: Avoid deadlock during teardown The display compositor blocks on the gpu thread to tear down. If GpuServiceInternal tears down before display compositor has a chance to tear down then Mus may crash during tear down or deadlock waiting for a task to execute that never will. BUG=668105 TBR=sadrul@chromium.org, sky@chromium.org NOPRESUBMIT=true ==========
The CQ bit was checked by kolos@chromium.org
Message was sent while issue was closed.
Description was changed from ========== Mus: Avoid deadlock during teardown The display compositor blocks on the gpu thread to tear down. If GpuServiceInternal tears down before display compositor has a chance to tear down then Mus may crash during tear down or deadlock waiting for a task to execute that never will. BUG=668105 TBR=sadrul@chromium.org, sky@chromium.org NOPRESUBMIT=true ========== to ========== Mus: Avoid deadlock during teardown The display compositor blocks on the gpu thread to tear down. If GpuServiceInternal tears down before display compositor has a chance to tear down then Mus may crash during tear down or deadlock waiting for a task to execute that never will. BUG=668105 TBR=sadrul@chromium.org, sky@chromium.org NOTRY=true NOPRESUBMIT=true ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Mus: Avoid deadlock during teardown The display compositor blocks on the gpu thread to tear down. If GpuServiceInternal tears down before display compositor has a chance to tear down then Mus may crash during tear down or deadlock waiting for a task to execute that never will. BUG=668105 TBR=sadrul@chromium.org, sky@chromium.org NOTRY=true NOPRESUBMIT=true ========== to ========== Mus: Avoid deadlock during teardown The display compositor blocks on the gpu thread to tear down. If GpuServiceInternal tears down before display compositor has a chance to tear down then Mus may crash during tear down or deadlock waiting for a task to execute that never will. BUG=668105 TBR=sadrul@chromium.org, sky@chromium.org NOTRY=true NOPRESUBMIT=true Committed: https://crrev.com/5059e49d0a21c1c311653c1613de16b27aefc963 Cr-Commit-Position: refs/heads/master@{#434310} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5059e49d0a21c1c311653c1613de16b27aefc963 Cr-Commit-Position: refs/heads/master@{#434310} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
