Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2164503002:
Do not delete blink::WebRTCCertificateCallback on non-main thread (Closed)
|
Created:
4 years, 5 months ago by tzik Modified:
4 years, 4 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, gavinp+memory_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not delete blink::WebRTCCertificateCallback on non-main thread
blink::WebRTCCertificateCallback has a blink::Persistent, which can not
be destroyed on the original thread. However, RTC implementation brings
it to a worker thread and occasionally deletes it on that thread, in
case the main thread is already gone.
This CL replaces the deleter of the std::unique_ptr that holds
WebRTCCertificateCallback with OnTaskRunnerDeleter, which ensures the
objects is deleted on the original thread.
BUG=627004
Committed: https://crrev.com/edbd386e6c520cf327e530f65988587eea25d65c
Cr-Commit-Position: refs/heads/master@{#411965}
Patch Set 1 #Patch Set 2 : +test #Patch Set 3 : fix #Patch Set 4 : fix #
Total comments: 12
Patch Set 5 : -copy ctor & assign from OnTaskRunnerDeleter #Patch Set 6 : readd #Patch Set 7 : +std::move, -auto, s/std::move/&/ #
Total comments: 4
Patch Set 8 : -wrapUnique #Patch Set 9 : rebase #
Messages
Total messages: 52 (36 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
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 ========== Do not delete blink::WebRTCCertificateCallback on non-main thread blink::WebRTCCertificateCallback has a blink::Persistent, which can not outlive blink::ThreadState. However, RTC implementation brings it to a worker thread and occasionally deletes it on that thread, in case the main thread is already gone. This CL replaces the deleter of the std::unique_ptr that holds WebRTCCertificateCallback with OnTaskRunnerDeleter, which ensures the objects is deleted on the original thread. BUG= ========== to ========== Do not delete blink::WebRTCCertificateCallback on non-main thread blink::WebRTCCertificateCallback has a blink::Persistent, which can not outlive blink::ThreadState. However, RTC implementation brings it to a worker thread and occasionally deletes it on that thread, in case the main thread is already gone. This CL replaces the deleter of the std::unique_ptr that holds WebRTCCertificateCallback with OnTaskRunnerDeleter, which ensures the objects is deleted on the original thread. BUG=627004 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
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 ========== Do not delete blink::WebRTCCertificateCallback on non-main thread blink::WebRTCCertificateCallback has a blink::Persistent, which can not outlive blink::ThreadState. However, RTC implementation brings it to a worker thread and occasionally deletes it on that thread, in case the main thread is already gone. This CL replaces the deleter of the std::unique_ptr that holds WebRTCCertificateCallback with OnTaskRunnerDeleter, which ensures the objects is deleted on the original thread. BUG=627004 ========== to ========== Do not delete blink::WebRTCCertificateCallback on non-main thread blink::WebRTCCertificateCallback has a blink::Persistent, which can not be destroyed on the original thread. However, RTC implementation brings it to a worker thread and occasionally deletes it on that thread, in case the main thread is already gone. This CL replaces the deleter of the std::unique_ptr that holds WebRTCCertificateCallback with OnTaskRunnerDeleter, which ensures the objects is deleted on the original thread. BUG=627004 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
tzik@chromium.org changed reviewers: + dcheng@chromium.org, hbos@chromium.org, perkj@chromium.org
PTAL
lgtm https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... File base/sequenced_task_runner.cc (right): https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... base/sequenced_task_runner.cc:42: OnTaskRunnerDeleter&&) = default; Why do you need to explicitly define these if they are default-implementations? https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... File base/sequenced_task_runner.h (right): https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... base/sequenced_task_runner.h:158: explicit OnTaskRunnerDeleter(scoped_refptr<SequencedTaskRunner> task_runner); nit: const scoped_refptr<...>&
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... File base/sequenced_task_runner.cc (right): https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... base/sequenced_task_runner.cc:42: OnTaskRunnerDeleter&&) = default; On 2016/07/20 12:42:32, hbos_chromium wrote: > Why do you need to explicitly define these if they are default-implementations? This was to (implicitly) make this class move-only. However it seems harmless to make this class copyable. Removed these move ctor and assign op. https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... File base/sequenced_task_runner.h (right): https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... base/sequenced_task_runner.h:158: explicit OnTaskRunnerDeleter(scoped_refptr<SequencedTaskRunner> task_runner); On 2016/07/20 12:42:32, hbos_chromium wrote: > nit: const scoped_refptr<...>& Newer styleguide prefer the pass-by-value on smart pointers to pass-by-ref. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... File base/sequenced_task_runner.h (right): https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... base/sequenced_task_runner.h:158: explicit OnTaskRunnerDeleter(scoped_refptr<SequencedTaskRunner> task_runner); On 2016/07/20 14:37:50, tzik wrote: > On 2016/07/20 12:42:32, hbos_chromium wrote: > > nit: const scoped_refptr<...>& > > Newer styleguide prefer the pass-by-value on smart pointers to pass-by-ref. > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Oh... The chromium-style plugin complains we need an explicit out-of-line copy/move ctor. :-( Let me readd them.
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I see! https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... File base/sequenced_task_runner.h (right): https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... base/sequenced_task_runner.h:158: explicit OnTaskRunnerDeleter(scoped_refptr<SequencedTaskRunner> task_runner); On 2016/07/20 14:42:26, tzik wrote: > On 2016/07/20 14:37:50, tzik wrote: > > On 2016/07/20 12:42:32, hbos_chromium wrote: > > > nit: const scoped_refptr<...>& > > > > Newer styleguide prefer the pass-by-value on smart pointers to pass-by-ref. > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > Oh... The chromium-style plugin complains we need an explicit out-of-line > copy/move ctor. :-( > Let me readd them. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... File base/sequenced_task_runner.cc (right): https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... base/sequenced_task_runner.cc:33: : task_runner_(task_runner) { Nit: std::move() https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... File base/sequenced_task_runner_unittest.cc (right): https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... base/sequenced_task_runner_unittest.cc:52: Passed(std::move(ptr)))); Nit: Passed(&ptr) https://codereview.chromium.org/2164503002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_certificate_generator.cc (right): https://codereview.chromium.org/2164503002/diff/60001/content/renderer/media/... content/renderer/media/rtc_certificate_generator.cc:67: auto transition = base::WrapUnique( Since this is typedefed, maybe we can just write CertificateCallbackPtr(observer.release(), base::OnTaskRunnerDeleter(...)) This feels like a pretty niche use case.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
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/2164503002/diff/60001/base/sequenced_task_run... File base/sequenced_task_runner.cc (right): https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... base/sequenced_task_runner.cc:33: : task_runner_(task_runner) { On 2016/07/22 06:12:04, dcheng wrote: > Nit: std::move() Done. https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... File base/sequenced_task_runner_unittest.cc (right): https://codereview.chromium.org/2164503002/diff/60001/base/sequenced_task_run... base/sequenced_task_runner_unittest.cc:52: Passed(std::move(ptr)))); On 2016/07/22 06:12:04, dcheng wrote: > Nit: Passed(&ptr) Done. https://codereview.chromium.org/2164503002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_certificate_generator.cc (right): https://codereview.chromium.org/2164503002/diff/60001/content/renderer/media/... content/renderer/media/rtc_certificate_generator.cc:67: auto transition = base::WrapUnique( On 2016/07/22 06:12:04, dcheng wrote: > Since this is typedefed, maybe we can just write > CertificateCallbackPtr(observer.release(), base::OnTaskRunnerDeleter(...)) > > This feels like a pretty niche use case. Done.
https://codereview.chromium.org/2164503002/diff/140001/base/memory/ptr_util.h File base/memory/ptr_util.h (right): https://codereview.chromium.org/2164503002/diff/140001/base/memory/ptr_util.h... base/memory/ptr_util.h:17: std::unique_ptr<T, D> WrapUnique(T* ptr, D deleter = D()) { We don't need the custom deleter argument anymore, right? https://codereview.chromium.org/2164503002/diff/140001/base/sequenced_task_ru... File base/sequenced_task_runner.h (right): https://codereview.chromium.org/2164503002/diff/140001/base/sequenced_task_ru... base/sequenced_task_runner.h:173: }; Do you anticipate this being used in a bunch of other places?
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
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/2164503002/diff/140001/base/memory/ptr_util.h File base/memory/ptr_util.h (right): https://codereview.chromium.org/2164503002/diff/140001/base/memory/ptr_util.h... base/memory/ptr_util.h:17: std::unique_ptr<T, D> WrapUnique(T* ptr, D deleter = D()) { On 2016/07/22 07:29:00, dcheng wrote: > We don't need the custom deleter argument anymore, right? Ah, right. Removed. https://codereview.chromium.org/2164503002/diff/140001/base/sequenced_task_ru... File base/sequenced_task_runner.h (right): https://codereview.chromium.org/2164503002/diff/140001/base/sequenced_task_ru... base/sequenced_task_runner.h:173: }; On 2016/07/22 07:29:00, dcheng wrote: > Do you anticipate this being used in a bunch of other places? Yes. I saw several places that hold an object on a foreign thread as a unique_ptr and post it back to the original thread for deletion. They tend to ignore PostTask failure, and that causes object deletion on the foreign thread. I think we should use this deleter for them.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
//base LGTM, under the assumption that we'll be adding more uses of this throughout the codebase. Out of curiosity, are the problems mostly in //content or scattered throughout Chrome? Is there a good way to find code like this that violates thread invariants?
I hope you have not waited for me. (Vacation) hbos knows much more about the certificates than me. but lgtm and thanks.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/25 03:01:48, dcheng wrote: > //base LGTM, under the assumption that we'll be adding more uses of this > throughout the codebase. Out of curiosity, are the problems mostly in //content > or scattered throughout Chrome? Is there a good way to find code like this that > violates thread invariants? I think it's scattered throughout Chrome. Many of PostTask() callers ignore the return value, and just drop the posted closure on the wrong thread. We will be able to catch a part of them (subclasses of NonThreadSafe and RefCounted) by encoding the thread restriction into the callback type. Though it's hard to detect it in general case.
On 2016/08/15 05:43:39, perkj_chrome wrote: > I hope you have not waited for me. (Vacation) hbos knows much more about the > certificates than me. > > but lgtm and thanks. Thanks for notifying me. I just forgot to land this.
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 tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hbos@chromium.org, perkj@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2164503002/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Do not delete blink::WebRTCCertificateCallback on non-main thread blink::WebRTCCertificateCallback has a blink::Persistent, which can not be destroyed on the original thread. However, RTC implementation brings it to a worker thread and occasionally deletes it on that thread, in case the main thread is already gone. This CL replaces the deleter of the std::unique_ptr that holds WebRTCCertificateCallback with OnTaskRunnerDeleter, which ensures the objects is deleted on the original thread. BUG=627004 ========== to ========== Do not delete blink::WebRTCCertificateCallback on non-main thread blink::WebRTCCertificateCallback has a blink::Persistent, which can not be destroyed on the original thread. However, RTC implementation brings it to a worker thread and occasionally deletes it on that thread, in case the main thread is already gone. This CL replaces the deleter of the std::unique_ptr that holds WebRTCCertificateCallback with OnTaskRunnerDeleter, which ensures the objects is deleted on the original thread. BUG=627004 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Do not delete blink::WebRTCCertificateCallback on non-main thread blink::WebRTCCertificateCallback has a blink::Persistent, which can not be destroyed on the original thread. However, RTC implementation brings it to a worker thread and occasionally deletes it on that thread, in case the main thread is already gone. This CL replaces the deleter of the std::unique_ptr that holds WebRTCCertificateCallback with OnTaskRunnerDeleter, which ensures the objects is deleted on the original thread. BUG=627004 ========== to ========== Do not delete blink::WebRTCCertificateCallback on non-main thread blink::WebRTCCertificateCallback has a blink::Persistent, which can not be destroyed on the original thread. However, RTC implementation brings it to a worker thread and occasionally deletes it on that thread, in case the main thread is already gone. This CL replaces the deleter of the std::unique_ptr that holds WebRTCCertificateCallback with OnTaskRunnerDeleter, which ensures the objects is deleted on the original thread. BUG=627004 Committed: https://crrev.com/edbd386e6c520cf327e530f65988587eea25d65c Cr-Commit-Position: refs/heads/master@{#411965} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/edbd386e6c520cf327e530f65988587eea25d65c Cr-Commit-Position: refs/heads/master@{#411965} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
