Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3010433002:
Android: Don't check ref count when releasing PeerConnectionInterface (Closed)
|
Created:
3 years, 3 months ago by magjed_webrtc Modified:
3 years, 3 months ago Reviewers:
Taylor Brandstetter CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAndroid: Don't check ref count when releasing PeerConnectionInterface
BUG=webrtc:8043
Review-Url: https://codereview.webrtc.org/3010433002
Cr-Commit-Position: refs/heads/master@{#19539}
Committed: https://chromium.googlesource.com/external/webrtc/+/7eb4082a666676fc488e7a0be8c753b4bc4ab1fd
Patch Set 1 #Patch Set 2 : Rebase #Messages
Total messages: 19 (12 generated)
The CQ bit was checked by magjed@webrtc.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.webrtc.org/...
Description was changed from ========== Android: Don't check ref count when releasing PeerConnectionInterface BUG=webrtc:8043 ========== to ========== Android: Don't check ref count when releasing PeerConnectionInterface BUG=webrtc:8043 ==========
magjed@webrtc.org changed reviewers: + deadbeef@webrtc.org
Taylor - please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. Though we're losing something small here: an implicit test that "PeerConnection.dispose", if called at the "right" time, will result in the last reference being removed, and the underlying PeerConnection being destroyed. Is there anything else that could provide that test coverage? Maybe if we supported injecting a fake C++ PeerConnectionFactory into the Java wrapper, which would track deletions. May not be worth the effort though.
On 2017/08/25 16:55:17, Taylor Brandstetter wrote: > lgtm. > > Though we're losing something small here: an implicit test that > "PeerConnection.dispose", if called at the "right" time, will result in the last > reference being removed, and the underlying PeerConnection being destroyed. > > Is there anything else that could provide that test coverage? Maybe if we > supported injecting a fake C++ PeerConnectionFactory into the Java wrapper, > which would track deletions. May not be worth the effort though. Yeah, agreed that we lose a bit of checking here. I don't feel like spending the time to add a test right now. We have many classes with similar behavior that we could also test. Maybe we could investigate some general asan/msan test that would catch all these cases. A somewhat similar issue is that we would like to check if someone forgets to call dispose() on the Java objects. We have an idea to use finalize() for this and I filed an issue here: https://bugs.chromium.org/p/webrtc/issues/detail?id=8165.
The CQ bit was checked by magjed@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/20772)
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/3010433002/#ps20001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1503750622441530,
"parent_rev": "09f3f6eb268b7d84cc8aa2e36a84cf6380f74037", "commit_rev":
"7eb4082a666676fc488e7a0be8c753b4bc4ab1fd"}
Message was sent while issue was closed.
Description was changed from ========== Android: Don't check ref count when releasing PeerConnectionInterface BUG=webrtc:8043 ========== to ========== Android: Don't check ref count when releasing PeerConnectionInterface BUG=webrtc:8043 Review-Url: https://codereview.webrtc.org/3010433002 Cr-Commit-Position: refs/heads/master@{#19539} Committed: https://chromium.googlesource.com/external/webrtc/+/7eb4082a666676fc488e7a0be... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/7eb4082a666676fc488e7a0be... |
