mollyim/webrtc
1
0
Fork 0
mirror of https://github.com/mollyim/webrtc.git synced 2025-05-12 21:30:45 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions

Revert "Handle corner case in SctpDataChannel::ObserverAdapter"

Browse source 
This reverts commit 1b3c89878e.

Reason for revert: The issue wasn't solved by this.

Original change's description:
> Handle corner case in SctpDataChannel::ObserverAdapter
>
> This handles a corner case whereby an OnStateChange implementation
> synchronously calls UnregisterObserver, which would (before this CL)
> delete the observer adapter.
>
> (Using No-Try since an import bot won't pass until this CL lands)
>
> No-Try: True
> Bug: webrtc:11547
> Change-Id: I33a13495aad6151fdd76becfa9a2c8672d80d825
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300280
> Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#39761}

Bug: webrtc:11547
Change-Id: I6af67a367631b9d853a848811a98b0f1cc576f71
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300340
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Tomas Gunnarsson <tommi@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39762}
This commit is contained in:
Tomas Gunnarsson 2023-04-05 06:14:23 +00:00 committed by WebRTC LUCI CQ
parent 1b3c89878e
commit d09103718e
2 changed files with 10 additions and 58 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

27
pc/data_channel_unittest.cc
View file

@ -179,33 +179,6 @@ TEST_F(SctpDataChannelTest, ConnectedToTransportOnCreated) {
EXPECT_TRUE(controller_->IsStreamAdded(sid));
}
// Tests that calling `UnregisterObserver()` from within the `OnStateChange`
// is safe.
TEST_F(SctpDataChannelTest, UnregisterObserverFromOnStateChange) {
class TrickyObserver : public DataChannelObserver {
public:
explicit TrickyObserver(DataChannelInterface* channel)
: channel_(channel) {}
void OnStateChange() override { channel_->UnregisterObserver(); }
void OnBufferedAmountChange(uint64_t previous_amount) override {}
void OnMessage(const DataBuffer& buffer) override {}
// This test is specifically for the observer adapter inside SctpDataChannel
// that kicks in when the return value from `IsOkToCallOnTheNetworkThread()`
// is false.
bool IsOkToCallOnTheNetworkThread() override { return false; }
private:
DataChannelInterface* channel_;
};
EXPECT_EQ(DataChannelInterface::kConnecting, channel_->state());
TrickyObserver observer(channel_.get());
channel_->RegisterObserver(&observer);
SetChannelReady();
EXPECT_EQ(DataChannelInterface::kOpen, channel_->state());
}
// Tests the state of the data channel.
TEST_F(SctpDataChannelTest, StateTransition) {
AddObserver();

41
pc/sctp_data_channel.cc
View file

@ -173,12 +173,6 @@ class SctpDataChannel::ObserverAdapter : public DataChannelObserver {
return cached_state_;
}
void SetDelegate(DataChannelObserver* delegate) {
RTC_DCHECK_RUN_ON(signaling_thread());
delegate_ = delegate;
safety_.reset(webrtc::PendingTaskSafetyFlag::CreateDetached());
}
private:
void OnStateChange() override {
RTC_DCHECK_RUN_ON(network_thread());
@ -187,8 +181,7 @@ class SctpDataChannel::ObserverAdapter : public DataChannelObserver {
RTC_DCHECK_RUN_ON(signaling_thread());
cached_state_ = new_state;
inside_state_change_ = true;
if (delegate_)
delegate_->OnStateChange();
delegate_->OnStateChange();
inside_state_change_ = false;
}));
}
@ -196,27 +189,22 @@ class SctpDataChannel::ObserverAdapter : public DataChannelObserver {
void OnMessage(const DataBuffer& buffer) override {
RTC_DCHECK_RUN_ON(network_thread());
signaling_thread()->PostTask(
SafeTask(safety_.flag(), [this, buffer = buffer] {
RTC_DCHECK_RUN_ON(signaling_thread());
if (delegate_)
delegate_->OnMessage(buffer);
}));
SafeTask(safety_.flag(),
[this, buffer = buffer] { delegate_->OnMessage(buffer); }));
}
void OnBufferedAmountChange(uint64_t sent_data_size) override {
RTC_DCHECK_RUN_ON(network_thread());
signaling_thread()->PostTask(
SafeTask(safety_.flag(), [this, sent_data_size] {
RTC_DCHECK_RUN_ON(signaling_thread());
if (delegate_)
delegate_->OnBufferedAmountChange(sent_data_size);
delegate_->OnBufferedAmountChange(sent_data_size);
}));
}
rtc::Thread* signaling_thread() const { return channel_->signaling_thread_; }
rtc::Thread* network_thread() const { return channel_->network_thread_; }
DataChannelObserver* delegate_ RTC_GUARDED_BY(signaling_thread());
DataChannelObserver* const delegate_;
SctpDataChannel* const channel_;
ScopedTaskSafety safety_;
bool inside_state_change_ RTC_GUARDED_BY(signaling_thread()) = false;
@ -303,11 +291,8 @@ void SctpDataChannel::RegisterObserver(DataChannelObserver* observer) {
// should be called on the network thread and IsOkToCallOnTheNetworkThread().
if (!observer->IsOkToCallOnTheNetworkThread()) {
auto prepare_observer = [&]() {
if (observer_adapter_) {
observer_adapter_->SetDelegate(observer);
} else {
observer_adapter_ = std::make_unique<ObserverAdapter>(observer, this);
}
RTC_DCHECK(!observer_adapter_);
observer_adapter_ = std::make_unique<ObserverAdapter>(observer, this);
return observer_adapter_.get();
};
// Instantiate the adapter in the right context and then substitute the
@ -359,18 +344,12 @@ void SctpDataChannel::UnregisterObserver() {
network_thread_->BlockingCall(std::move(unregister_observer));
}
auto clear_delegate = [&]() {
// In case an implementation decides to unregister an observer while
// in a callback from the observer adapter, we can't delete the adapter.
// Instead we'll just clear the delegate pointer.
if (observer_adapter_)
observer_adapter_->SetDelegate(nullptr);
};
auto clear_observer = [&]() { observer_adapter_.reset(); };
if (current_thread != signaling_thread_) {
signaling_thread_->BlockingCall(std::move(clear_delegate));
signaling_thread_->BlockingCall(std::move(clear_observer));
} else {
clear_delegate();
clear_observer();
}
}