Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2222553002:
Removed invalid thread DCHECKs in rtc_peer_connection_handler.cc (Closed)
|
Created:
4 years, 4 months ago by hbos_chromium Modified:
4 years, 4 months ago CC:
chromium-reviews, posciak+watch_chromium.org, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoved CreateSessionDescriptionRequest and SetSessionDescriptionRequest
main-thread DCHECKs from their destructors.
CreateSessionDescriptionRequest and SetSessionDescriptionRequest are
reference counted and have callback methods that are invoked on the
signaling thread. If invoked on a non-main thread a post occurs to the
main thread, referencing the object until the callback has completed on
the main thread. There was an incorrect assumption that the object would
also be destroyed on the main thread. But because the callback may
sometimes complete before the signaling thread has had an opportunity to
dereference the object it can be destroyed on the signaling thread.
This CL allows it to be destroyed on any thread.
BUG=634342
Committed: https://crrev.com/4d31d2e1163f6f3125470695ccd60a30830a1cde
Cr-Commit-Position: refs/heads/master@{#412523}
Patch Set 1 : (Incorrectly thinking "this" was not ref counted when posting, not a fix) #
Total comments: 4
Patch Set 2 : Removed destructor DCHECKs and added a comment #
Total comments: 2
Patch Set 3 : Comment addressed #Messages
Total messages: 27 (17 generated)
The CQ bit was checked by hbos@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...
hbos@chromium.org changed reviewers: + perkj@chromium.org
Please take a look, perkj.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
perkj@chromium.org changed reviewers: + tommi@chromium.org
https://codereview.chromium.org/2222553002/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2222553002/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_peer_connection_handler.cc:369: void OnSuccess(webrtc::SessionDescriptionInterface* desc) override { webrtc::SessionDescriptionInterface* desc should have been const scoped_refptr<webrtc::SessionDescriptionInterface>& desc instead to make this more clear. But I think this was actually correct from the beginning. Base::Bind will treat * desc as a ref counted object unless you use base::Unretained.
https://codereview.chromium.org/2222553002/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2222553002/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_peer_connection_handler.cc:369: void OnSuccess(webrtc::SessionDescriptionInterface* desc) override { On 2016/08/15 05:37:57, perkj_chrome wrote: > webrtc::SessionDescriptionInterface* desc should have been const > scoped_refptr<webrtc::SessionDescriptionInterface>& desc instead to make this > more clear. > But I think this was actually correct from the beginning. > Base::Bind will treat * desc as a ref counted object unless you use > base::Unretained. Oh, back to the drawing board.
Please see comment, perkj. https://codereview.chromium.org/2222553002/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2222553002/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_peer_connection_handler.cc:369: void OnSuccess(webrtc::SessionDescriptionInterface* desc) override { On 2016/08/15 14:16:55, hbos_chromium wrote: > On 2016/08/15 05:37:57, perkj_chrome wrote: > > webrtc::SessionDescriptionInterface* desc should have been const > > scoped_refptr<webrtc::SessionDescriptionInterface>& desc instead to make this > > more clear. > > But I think this was actually correct from the beginning. > > Base::Bind will treat * desc as a ref counted object unless you use > > base::Unretained. > > Oh, back to the drawing board. The template magic of base::Bind is beyond me, but I did some printf debugging. At main_thread_->PostTask, if I pass |this| both AddRef and Release occurs before we return. The Release is unexpected. If I pass |scoped_refptr<CreateSessionDescriptionRequest>(this)| AddRef occurs but no Release, as it should be.
Patchset #2 (id:20001) has been deleted
Description was changed from ========== scoped_refptr to protect SessionDescriptionRequests from being destroyed CreateSessionDescriptionRequest and SetSessionDescriptionRequest are ref counted objects that are DCHECK'd to be destroyed on the main thread. When OnSuccess/OnFailure is called on a non-main thread a post happens to the main thread with a raw pointer to "this" request. I think there is a risk that the last reference is destroyed before the post completes. Added scoped_refptr to protect against destruction until post completes. BUG=634342 ========== to ========== Removed CreateSessionDescriptionRequest and SetSessionDescriptionRequest destructor is on main thread-DCHECK. CreateSessionDescriptionRequest and SetSessionDescriptionRequest are reference counted and have callback methods that are invoked on the signaling thread. If invoked on a non-main thread a post occurs to the main thread, referencing the object until the callback has completed on the main thread. There was an incorrect assumption that the object would also be destroyed on the main thread. But because the callback may sometimes complete before the signaling thread has had an opportunity to dereference the object it can be destroyed on the signaling thread. This CL allows it to be destroyed on any thread. BUG=634342 ==========
Description was changed from ========== Removed CreateSessionDescriptionRequest and SetSessionDescriptionRequest destructor is on main thread-DCHECK. CreateSessionDescriptionRequest and SetSessionDescriptionRequest are reference counted and have callback methods that are invoked on the signaling thread. If invoked on a non-main thread a post occurs to the main thread, referencing the object until the callback has completed on the main thread. There was an incorrect assumption that the object would also be destroyed on the main thread. But because the callback may sometimes complete before the signaling thread has had an opportunity to dereference the object it can be destroyed on the signaling thread. This CL allows it to be destroyed on any thread. BUG=634342 ========== to ========== Removed CreateSessionDescriptionRequest and SetSessionDescriptionRequest main-thread DCHECKs from their destructors. CreateSessionDescriptionRequest and SetSessionDescriptionRequest are reference counted and have callback methods that are invoked on the signaling thread. If invoked on a non-main thread a post occurs to the main thread, referencing the object until the callback has completed on the main thread. There was an incorrect assumption that the object would also be destroyed on the main thread. But because the callback may sometimes complete before the signaling thread has had an opportunity to dereference the object it can be destroyed on the signaling thread. This CL allows it to be destroyed on any thread. BUG=634342 ==========
PTAL perkj. As discussed, the first patch set did not fix the problem but now the bug has been identified and resolved in patch set 2. Sleeping did indeed trigger it. https://codereview.chromium.org/2222553002/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2222553002/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_peer_connection_handler.cc:369: void OnSuccess(webrtc::SessionDescriptionInterface* desc) override { On 2016/08/16 13:43:51, hbos_chromium wrote: > On 2016/08/15 14:16:55, hbos_chromium wrote: > > On 2016/08/15 05:37:57, perkj_chrome wrote: > > > webrtc::SessionDescriptionInterface* desc should have been const > > > scoped_refptr<webrtc::SessionDescriptionInterface>& desc instead to make > this > > > more clear. > > > But I think this was actually correct from the beginning. > > > Base::Bind will treat * desc as a ref counted object unless you use > > > base::Unretained. > > > > Oh, back to the drawing board. > > The template magic of base::Bind is beyond me, but I did some printf debugging. > > At main_thread_->PostTask, if I pass |this| both AddRef and Release occurs > before we return. The Release is unexpected. If I pass > |scoped_refptr<CreateSessionDescriptionRequest>(this)| AddRef occurs but no > Release, as it should be. Nevermind. The different printfs can be explained by races between threads or whatnot.
The CQ bit was checked by hbos@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.
lgtm with the comment considered https://codereview.chromium.org/2222553002/diff/40001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2222553002/diff/40001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:396: // |OnFailure| may be invoked on any thread, for non-main threads the I had problem reading the sentence "This object is reference counted and its callback methods |OnSuccess| and |OnFailure| may be invoked on any thread, for non-main threads the ...." May I suggest something like // |OnFailure| will be invoked on libjingles signaling thread and posted to the main thread. Since the main thread may complete before the signaling thread has deferenced this object there is no guarante....
https://codereview.chromium.org/2222553002/diff/40001/content/renderer/media/... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2222553002/diff/40001/content/renderer/media/... content/renderer/media/rtc_peer_connection_handler.cc:396: // |OnFailure| may be invoked on any thread, for non-main threads the On 2016/08/17 11:16:14, perkj_chrome wrote: > I had problem reading the sentence > "This object is reference counted and its callback methods |OnSuccess| and > |OnFailure| may be invoked on any thread, for non-main threads the ...." > > May I suggest something like // |OnFailure| will be invoked on libjingles > signaling thread and posted to the main thread. Since the main thread may > complete before the signaling thread has deferenced this object there is no > guarante.... Done.
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org Link to the patchset: https://codereview.chromium.org/2222553002/#ps60001 (title: "Comment addressed")
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 ========== Removed CreateSessionDescriptionRequest and SetSessionDescriptionRequest main-thread DCHECKs from their destructors. CreateSessionDescriptionRequest and SetSessionDescriptionRequest are reference counted and have callback methods that are invoked on the signaling thread. If invoked on a non-main thread a post occurs to the main thread, referencing the object until the callback has completed on the main thread. There was an incorrect assumption that the object would also be destroyed on the main thread. But because the callback may sometimes complete before the signaling thread has had an opportunity to dereference the object it can be destroyed on the signaling thread. This CL allows it to be destroyed on any thread. BUG=634342 ========== to ========== Removed CreateSessionDescriptionRequest and SetSessionDescriptionRequest main-thread DCHECKs from their destructors. CreateSessionDescriptionRequest and SetSessionDescriptionRequest are reference counted and have callback methods that are invoked on the signaling thread. If invoked on a non-main thread a post occurs to the main thread, referencing the object until the callback has completed on the main thread. There was an incorrect assumption that the object would also be destroyed on the main thread. But because the callback may sometimes complete before the signaling thread has had an opportunity to dereference the object it can be destroyed on the signaling thread. This CL allows it to be destroyed on any thread. BUG=634342 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Removed CreateSessionDescriptionRequest and SetSessionDescriptionRequest main-thread DCHECKs from their destructors. CreateSessionDescriptionRequest and SetSessionDescriptionRequest are reference counted and have callback methods that are invoked on the signaling thread. If invoked on a non-main thread a post occurs to the main thread, referencing the object until the callback has completed on the main thread. There was an incorrect assumption that the object would also be destroyed on the main thread. But because the callback may sometimes complete before the signaling thread has had an opportunity to dereference the object it can be destroyed on the signaling thread. This CL allows it to be destroyed on any thread. BUG=634342 ========== to ========== Removed CreateSessionDescriptionRequest and SetSessionDescriptionRequest main-thread DCHECKs from their destructors. CreateSessionDescriptionRequest and SetSessionDescriptionRequest are reference counted and have callback methods that are invoked on the signaling thread. If invoked on a non-main thread a post occurs to the main thread, referencing the object until the callback has completed on the main thread. There was an incorrect assumption that the object would also be destroyed on the main thread. But because the callback may sometimes complete before the signaling thread has had an opportunity to dereference the object it can be destroyed on the signaling thread. This CL allows it to be destroyed on any thread. BUG=634342 Committed: https://crrev.com/4d31d2e1163f6f3125470695ccd60a30830a1cde Cr-Commit-Position: refs/heads/master@{#412523} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4d31d2e1163f6f3125470695ccd60a30830a1cde Cr-Commit-Position: refs/heads/master@{#412523} |
