Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2783273002:
Fix DCHECK getting hit on destruction of GpuJpegDecodeAcceleratorHost (Closed)
|
Created:
3 years, 8 months ago by chfremer Modified:
3 years, 8 months ago Reviewers:
sandersd (OOO until July 31) CC:
chromium-reviews, posciak+watch_chromium.org, piman+watch_chromium.org, feature-media-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix DCHECK getting hit on destruction of GpuJpegDecodeAcceleratorHost
The previous fix for a deadlock (https://codereview.chromium.org/2786503002/)
was only partly effective. Even though it prevented the deadlock waiting for
a method that was never run, it now hits a DCHECK when the WeakPtrs are
invalidated on the wrong thread (synchronously as part of the destructor call).
To prevent this, we add a |io_task_runner_->DeleteSoon(weak_factory_for_io_)| to
the destructor of GpuJpegDecodeAcceleratorHost::Receiver. When |io_task_runner_|
no longer accepts tasks, this effectively leaks |weak_factory_for_io_|, and
therefore avoids hitting the DCHECK.
BUG=706186
TEST=
content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"
content_unittests --gtest_filter="*Video*"
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2783273002
Cr-Commit-Position: refs/heads/master@{#461053}
Committed: https://chromium.googlesource.com/chromium/src/+/b5d9d73d3108767c038c2b9a8d6aff786edbaa1e
Patch Set 1 #
Total comments: 6
Patch Set 2 : Incorporated suggestions from PatchSet 1 #Messages
Total messages: 19 (13 generated)
Description was changed from ========== Fix DCHECK getting hit on destruction of GpuJpegDecodeAcceleratorHost The previous fix for a deadlock (https://codereview.chromium.org/2786503002/) was only partly effective. Even though it prevented the deadlock waiting for a method that was never run, it now hits a DCHECK when the WeakPtrs are invalidated on the wrong thread (synchronously as part of the destructor call). To prevent this, we add a |io_task_runner_->DeleteSoon(weak_factory_for_io_)| to the destructor of GpuJpegDecodeAcceleratorHost::Receiver. When |io_task_runner_| no longer accepts tasks, this effectively leaks |weak_factory_for_io_|, and therefore avoids hitting the DCHECK. BUG=706186 TEST= content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" content_unittests --gtest_filter="*Video*" ========== to ========== Fix DCHECK getting hit on destruction of GpuJpegDecodeAcceleratorHost The previous fix for a deadlock (https://codereview.chromium.org/2786503002/) was only partly effective. Even though it prevented the deadlock waiting for a method that was never run, it now hits a DCHECK when the WeakPtrs are invalidated on the wrong thread (synchronously as part of the destructor call). To prevent this, we add a |io_task_runner_->DeleteSoon(weak_factory_for_io_)| to the destructor of GpuJpegDecodeAcceleratorHost::Receiver. When |io_task_runner_| no longer accepts tasks, this effectively leaks |weak_factory_for_io_|, and therefore avoids hitting the DCHECK. BUG=706186 TEST= content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" content_unittests --gtest_filter="*Video*" CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
chfremer@chromium.org changed reviewers: + sandersd@chromium.org
sandersd@: PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2783273002/diff/1/media/gpu/ipc/client/gpu_jp... File media/gpu/ipc/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/2783273002/diff/1/media/gpu/ipc/client/gpu_jp... media/gpu/ipc/client/gpu_jpeg_decode_accelerator_host.cc:35: base::MakeUnique<base::WeakPtrFactory<Receiver>>(this)) { I recommend creating a WeakPtr here in the constructor, and handing out copies of it from AsWeakPtrForIO(). https://codereview.chromium.org/2783273002/diff/1/media/gpu/ipc/client/gpu_jp... media/gpu/ipc/client/gpu_jpeg_decode_accelerator_host.cc:41: if (weak_factory_for_io_->HasWeakPtrs()) I recommend always posting this task, it would be more clear if this factory was only ever destructed on the IO thread. Might be worth a comment that this leaks if the IO thread is stopped, but that's not something that's usually commented on. (Just possibly relevant in this case since there has been a bug.) https://codereview.chromium.org/2783273002/diff/1/media/gpu/ipc/client/gpu_jp... media/gpu/ipc/client/gpu_jpeg_decode_accelerator_host.cc:128: base::Unretained(receiver_.get()), I'd add a comment here explaining that Unretained is safe because we always wait until the method finishes executing if there is any chance the task actually executes.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chfremer@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2783273002/diff/1/media/gpu/ipc/client/gpu_jp... File media/gpu/ipc/client/gpu_jpeg_decode_accelerator_host.cc (right): https://codereview.chromium.org/2783273002/diff/1/media/gpu/ipc/client/gpu_jp... media/gpu/ipc/client/gpu_jpeg_decode_accelerator_host.cc:35: base::MakeUnique<base::WeakPtrFactory<Receiver>>(this)) { On 2017/03/30 20:58:50, sandersd wrote: > I recommend creating a WeakPtr here in the constructor, and handing out copies > of it from AsWeakPtrForIO(). Done. https://codereview.chromium.org/2783273002/diff/1/media/gpu/ipc/client/gpu_jp... media/gpu/ipc/client/gpu_jpeg_decode_accelerator_host.cc:41: if (weak_factory_for_io_->HasWeakPtrs()) On 2017/03/30 20:58:49, sandersd wrote: > I recommend always posting this task, it would be more clear if this factory was > only ever destructed on the IO thread. > > Might be worth a comment that this leaks if the IO thread is stopped, but that's > not something that's usually commented on. (Just possibly relevant in this case > since there has been a bug.) Done. https://codereview.chromium.org/2783273002/diff/1/media/gpu/ipc/client/gpu_jp... media/gpu/ipc/client/gpu_jpeg_decode_accelerator_host.cc:128: base::Unretained(receiver_.get()), On 2017/03/30 20:58:50, sandersd wrote: > I'd add a comment here explaining that Unretained is safe because we always wait > until the method finishes executing if there is any chance the task actually > executes. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chfremer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490939813331330,
"parent_rev": "93cf8ced501ad85664fa80e69610621b69b6dd09", "commit_rev":
"b5d9d73d3108767c038c2b9a8d6aff786edbaa1e"}
Message was sent while issue was closed.
Description was changed from ========== Fix DCHECK getting hit on destruction of GpuJpegDecodeAcceleratorHost The previous fix for a deadlock (https://codereview.chromium.org/2786503002/) was only partly effective. Even though it prevented the deadlock waiting for a method that was never run, it now hits a DCHECK when the WeakPtrs are invalidated on the wrong thread (synchronously as part of the destructor call). To prevent this, we add a |io_task_runner_->DeleteSoon(weak_factory_for_io_)| to the destructor of GpuJpegDecodeAcceleratorHost::Receiver. When |io_task_runner_| no longer accepts tasks, this effectively leaks |weak_factory_for_io_|, and therefore avoids hitting the DCHECK. BUG=706186 TEST= content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" content_unittests --gtest_filter="*Video*" CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Fix DCHECK getting hit on destruction of GpuJpegDecodeAcceleratorHost The previous fix for a deadlock (https://codereview.chromium.org/2786503002/) was only partly effective. Even though it prevented the deadlock waiting for a method that was never run, it now hits a DCHECK when the WeakPtrs are invalidated on the wrong thread (synchronously as part of the destructor call). To prevent this, we add a |io_task_runner_->DeleteSoon(weak_factory_for_io_)| to the destructor of GpuJpegDecodeAcceleratorHost::Receiver. When |io_task_runner_| no longer accepts tasks, this effectively leaks |weak_factory_for_io_|, and therefore avoids hitting the DCHECK. BUG=706186 TEST= content_browsertests --gtest_filter="VideoCaptureBrowserTest.*" content_unittests --gtest_filter="*Video*" CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2783273002 Cr-Commit-Position: refs/heads/master@{#461053} Committed: https://chromium.googlesource.com/chromium/src/+/b5d9d73d3108767c038c2b9a8d6a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/b5d9d73d3108767c038c2b9a8d6a... |
