Don't crash when the ADM fails to init. #7

peter-signal · 2020-05-20T22:25:49Z

No description provided.

jim-signal

LGTM

…eger scaling factors are used // Skipping CQ, because bots are broken. The same bot passed yesterday. // Now two windows bots fail with "no executable found" or gn errors. TBR=srpang@webrtc.org (cherry picked from commit 09eb6e2) No-Try: True Bug: webrtc:11652 Change-Id: Id3642d607f62b72a567d521d9874b8588c2ce429 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176517 Reviewed-by: Erik Språng <sprang@webrtc.org> Commit-Queue: Ilya Nikolaevskiy <ilnik@webrtc.org> Cr-Original-Commit-Position: refs/heads/master@{#31465} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176848 Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org> Cr-Commit-Position: refs/branch-heads/4147@{#7} Cr-Branched-From: 2b7d969-refs/heads/master@{#31262}

…layers No-try, because IOS bots are broken on release branch. (cherry picked from commit 1dea1ea) No-Try: True Bug: chromium:1051476 Change-Id: Iaf2b6ab6640cd314a620dbdf1547d8f1b2f40693 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/168921 Reviewed-by: Evan Shrubsole <eshr@google.com> Commit-Queue: Ilya Nikolaevskiy <ilnik@webrtc.org> Cr-Original-Commit-Position: refs/heads/master@{#30581} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/168923 Cr-Commit-Position: refs/branch-heads/4044@{#7} Cr-Branched-From: be99ee8-refs/heads/master@{#30432}

The wrapping ICE controller will allow existing ICE controller implementations to migrate to the active interface, and eventually deprecate the legacy interface. Follow-up CL has unit tests for P2PTransportChannel using the new wrapping controller. Bug: webrtc:14367, webrtc:14131 Change-Id: I6c517449ff1e503e8268a7ef91afda793723fdeb Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/275302 Reviewed-by: Per Kjellander <perkj@webrtc.org> Reviewed-by: Jonas Oreland <jonaso@webrtc.org> Commit-Queue: Sameer Vijaykar <samvi@google.com> Cr-Commit-Position: refs/heads/main@{#38130}

…/n)" This reverts commit 6326c9c. Reason for revert: breaks upstream project Original change's description: > Add an active ICE controller that wraps a legacy controller (#7/n) > > The wrapping ICE controller will allow existing ICE controller implementations to migrate to the active interface, and eventually deprecate the legacy interface. > > Follow-up CL has unit tests for P2PTransportChannel using the new wrapping controller. > > Bug: webrtc:14367, webrtc:14131 > Change-Id: I6c517449ff1e503e8268a7ef91afda793723fdeb > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/275302 > Reviewed-by: Per Kjellander <perkj@webrtc.org> > Reviewed-by: Jonas Oreland <jonaso@webrtc.org> > Commit-Queue: Sameer Vijaykar <samvi@google.com> > Cr-Commit-Position: refs/heads/main@{#38130} Bug: webrtc:14367, webrtc:14131 Change-Id: I61dd98de62657852068c7566b55f19f662df9ff4 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/276043 Reviewed-by: Per Kjellander <perkj@webrtc.org> Reviewed-by: Jonas Oreland <jonaso@webrtc.org> Commit-Queue: Per Kjellander <perkj@webrtc.org> Auto-Submit: Sameer Vijaykar <samvi@google.com> Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com> Cr-Commit-Position: refs/heads/main@{#38131}

…/n)" This is a reland of commit 6326c9c Original change's description: > Add an active ICE controller that wraps a legacy controller (#7/n) > > The wrapping ICE controller will allow existing ICE controller implementations to migrate to the active interface, and eventually deprecate the legacy interface. > > Follow-up CL has unit tests for P2PTransportChannel using the new wrapping controller. > > Bug: webrtc:14367, webrtc:14131 > Change-Id: I6c517449ff1e503e8268a7ef91afda793723fdeb > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/275302 > Reviewed-by: Per Kjellander <perkj@webrtc.org> > Reviewed-by: Jonas Oreland <jonaso@webrtc.org> > Commit-Queue: Sameer Vijaykar <samvi@google.com> > Cr-Commit-Position: refs/heads/main@{#38130} Bug: webrtc:14367, webrtc:14131 Change-Id: I5662595db1df8c06b3acac9b39749f236906fa7e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/276044 Auto-Submit: Sameer Vijaykar <samvi@google.com> Reviewed-by: Jonas Oreland <jonaso@webrtc.org> Reviewed-by: Per Kjellander <perkj@webrtc.org> Commit-Queue: Per Kjellander <perkj@webrtc.org> Cr-Commit-Position: refs/heads/main@{#38149}

(cherry picked from commit 9d8fb97) No-try: true Bug: chromium:1477075 Change-Id: Ia05a868bfab9e99ef66704e8d6bce516a7a43b0a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/318440 Reviewed-by: Sergey Silkin <ssilkin@webrtc.org> Commit-Queue: Harald Alvestrand <hta@webrtc.org> Cr-Original-Commit-Position: refs/heads/main@{#40673} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/319320 Reviewed-by: Philipp Hancke <phancke@microsoft.com> Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org> Cr-Commit-Position: refs/branch-heads/5845@{#7} Cr-Branched-From: f80cf81-refs/heads/main@{#40319}

This was a fun bug which proved to be challenging to find a good solution for. The issue comes from the combination of partial reliability and stream resetting, which are covered in different RFCs, and where they don't refer to each other... Stream resetting (RFC 6525) is used in WebRTC for closing a Data Channel, and is done by signaling to the receiver that the stream sequence number (SSN) should be set to zero (0) at some time. Partial reliability (RFC 3758) - and expiring messages that will not be retransmitted - is done by signaling that the SSN should be set to a certain value at a certain TSN, as the messages up until the provided SSN are not to be expected to be sent again. As these two functionalities both work by signaling to the receiver what the next expected SSN should be, they need to do it correctly not to overwrite each others' intent. And here was the bug. An example scenario where this caused issues, where we are Z (the receiver), getting packets from the sender (A): 5 A->Z DATA (TSN=30, B, SID=2, SSN=0) 6 Z->A SACK (Ack=30) 7 A->Z DATA (TSN=31, E, SID=2, SSN=0) 8 A->Z RE_CONFIG (REQ=30, TSN=31, SID=2) 9 Z->A RE_CONFIG (RESP=30, Performed) 10 Z->A SACK (Ack=31) 11 A->Z DATA (TSN=32, SID=1) 12 A->Z FORWARD_TSN (TSN=32, SID=2, SSN=0) Let's assume that the path Z->A had packet loss and A never really received our responses (#6, #9, #10) in time. At #5, Z receives a DATA fragment, which it acks, and at #7 the end of that message. The stream is then reset (#8) which it signals that it was performed (#9) and acked (#10), and data on another stream (2) was received (#11). Since A hasn't received any ACKS yet, and those chunks on SID=2 all expired, A sends a FORWARD-TSN saying that "Skip to TSN=32, and don't expect SID=2, SSN=0". That makes the receiver expect the SSN on SID=2 to be SSN=1 next time at TSN=32. But that's not good at all - A reset the stream at #8 and will want to send the next message on SID=2 using SSN=0 - not 1. The FORWARD-TSN clearly can't have a TSN that is beyond the stream reset TSN for that stream. This is just one example - combining stream resetting and partial reliability, together with a lossy network, and different variants of this can occur, which results in the receiver possibly not delivering packets because it expects a different SSN than the one the sender is later using. So this CL adds "breakpoints" to how far a FORWARD-TSN can stretch. It will simply not cross any Stream Reset last assigned TSNs, and only when a receiver has acked that all TSNs up till the Stream Reset last assigned TSN has been received, it will proceed expiring chunks after that. Bug: webrtc:14600 Change-Id: Ibae8c9308f5dfe8d734377d42cce653e69e95731 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/321600 Commit-Queue: Victor Boivie <boivie@webrtc.org> Reviewed-by: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#40829}

The new version of MSan (rolled by [1]) detects the following: ``` ==39908==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x5591400a52ef in GetPlayoutDelayMs ./../../modules/audio_coding/neteq/decision_logic.cc:466:35 #1 0x5591400a52ef in webrtc::DecisionLogic::ExpectedPacketAvailable(webrtc::NetEqController::NetEqStatus) ./../../modules/audio_coding/neteq/decision_logic.cc:311:36 #2 0x5591400a39e9 in webrtc::DecisionLogic::GetDecision(webrtc::NetEqController::NetEqStatus const&, bool*) ./../../modules/audio_coding/neteq/decision_logic.cc:0:0 #3 0x55913cf590c9 in webrtc::DecisionLogicTest_PreemptiveExpand_Test::TestBody() ./../../modules/audio_coding/neteq/decision_logic_unittest.cc:139:3 #4 0x55913ef28283 in HandleExceptionsInMethodIfSupported<testing::Test, void> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:3 #5 0x55913ef28283 in testing::Test::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2710:5 #6 0x55913ef2ab46 in testing::TestInfo::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:2856:11 #7 0x55913ef2da34 in testing::TestSuite::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:3034:30 #8 0x55913ef621e8 in testing::internal::UnitTestImpl::RunAllTests() ./../../third_party/googletest/src/googletest/src/gtest.cc:5964:44 #9 0x55913ef60f54 in HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> ./../../third_party/googletest/src/googletest/src/gtest.cc:0:0 #10 0x55913ef60f54 in testing::UnitTest::Run() ./../../third_party/googletest/src/googletest/src/gtest.cc:5543:10 #11 0x55913ee1a944 in RUN_ALL_TESTS ./../../third_party/googletest/src/googletest/include/gtest/gtest.h:2334:73 #12 0x55913ee1a944 in webrtc::(anonymous namespace)::TestMainImpl::Run(int, char**) ./../../test/test_main_lib.cc:203:21 #13 0x55913cbd36b8 in main ./../../test/test_main.cc:72:16 #14 0x7fdb18c73082 in __libc_start_main /build/glibc-LcI20x/glibc-2.31/csu/../csu/libc-start.c:308:16 #15 0x55913cb3e1a9 in _start ??:0:0 ``` [1] - https://webrtc-review.googlesource.com/c/src/+/353620 Bug: b/344970813 Change-Id: I9b5d7791e68b4c494168ba9f007a3099ae21fed4 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/353581 Auto-Submit: Mirko Bonadei <mbonadei@webrtc.org> Reviewed-by: Jakob Ivarsson‎ <jakobi@webrtc.org> Commit-Queue: Jakob Ivarsson‎ <jakobi@webrtc.org> Cr-Commit-Position: refs/heads/main@{#42433}

Don't crash when the ADM fails to init.

8d1f780

jim-signal self-requested a review May 20, 2020 22:32

jim-signal approved these changes May 20, 2020

View reviewed changes

peter-signal merged commit a716a9d into master May 20, 2020

peter-signal deleted the dont-crash-adm-init branch July 15, 2020 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't crash when the ADM fails to init. #7

Don't crash when the ADM fails to init. #7

peter-signal commented May 20, 2020

jim-signal left a comment

Don't crash when the ADM fails to init. #7

Don't crash when the ADM fails to init. #7

Conversation

peter-signal commented May 20, 2020

jim-signal left a comment

Choose a reason for hiding this comment