Skip to content

Commit

Permalink
Fix ReactNotificationService memory leak in ExecuteJsi (#10408)
Browse files Browse the repository at this point in the history 
* Fix ReactNotificationService memory leak

The memory leak may happen when ReactNotificationService uses wrong
IReactNotificationSubscription for the notification handler when handler
calls Unsubscribe() method for it.

* Change files

* Release notification handler after Unsubscribe

* Add verification to Subscribe method

* yarn format

* Fix compilation issue for Desktop code
  • Loading branch information
@vmoroz
vmoroz committed Aug 17, 2022
1 parent 40f4502 commit 047ab1e
Show file tree
Hide file tree
Showing 4 changed files with 238 additions and 104 deletions.
7 changes: 7 additions & 0 deletions change/react-native-windows-ddcc5f68-cdab-4bae-a2e7-714fdc0de029.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "Fix ReactNotificationService memory leak",
"packageName": "react-native-windows",
"email": "vmorozov@microsoft.com",
"dependentChangeType": "patch"
}
27 changes: 23 additions & 4 deletions vnext/Microsoft.ReactNative.IntegrationTests/ReactNotificationServiceTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,19 @@ struct NotificationTestModule {
void Initialize(ReactContext const &reactContext) noexcept {
m_reactContext = reactContext;

// Check that lambda receives correct subscription
struct SubscriptionHolder {
ReactNotificationSubscription Subscription{nullptr};
};
auto subscriptionHolder = std::make_shared<SubscriptionHolder>();

// Subscribe to a notification from the app.
reactContext.Notifications().Subscribe(
notifyModuleFromApp, [](IInspectable const & /*sender*/, ReactNotificationArgs<int> const &args) noexcept {
subscriptionHolder->Subscription = reactContext.Notifications().Subscribe(
notifyModuleFromApp,
[subscriptionHolder](IInspectable const & /*sender*/, ReactNotificationArgs<int> const &args) noexcept {
TestEventService::LogEvent("NotifyModuleFromApp", args.Data());
TestEventService::LogEvent(
"CheckModuleSubscription", args.Subscription() == subscriptionHolder->Subscription);

// Unsubscribe after the first notification.
args.Subscription().Unsubscribe();
Expand Down Expand Up @@ -301,12 +310,20 @@ TEST_CLASS (ReactNotificationServiceTests) {
TestReactNativeHostHolder(L"ReactNotificationServiceTests", [](ReactNativeHost const &host) noexcept {
host.PackageProviders().Append(winrt::make<NotificationTestPackageProvider>());

// Check that lambda receives correct subscription
struct SubscriptionHolder {
ReactNotificationSubscription Subscription{nullptr};
};
auto subscriptionHolder = std::make_shared<SubscriptionHolder>();

// Subscribe to a notification from module to app.
ReactNotificationService::Subscribe(
subscriptionHolder->Subscription = ReactNotificationService::Subscribe(
host.InstanceSettings().Notifications(),
notifyAppFromModule,
[](IInspectable const &, ReactNotificationArgs<int> const &args) {
[subscriptionHolder](IInspectable const &, ReactNotificationArgs<int> const &args) {
TestEventService::LogEvent("NotifyAppFromModule", args.Data());
TestEventService::LogEvent(
"CheckAppSubscription", args.Subscription() == subscriptionHolder->Subscription);

// Send a notification from app to the module.
args.Subscription().NotificationService().SendNotification(notifyModuleFromApp, 42);
Expand All @@ -319,7 +336,9 @@ TEST_CLASS (ReactNotificationServiceTests) {
TestEventService::ObserveEvents({
TestEvent{"Test started", nullptr},
TestEvent{"NotifyAppFromModule", 15},
TestEvent{"CheckAppSubscription", true},
TestEvent{"NotifyModuleFromApp", 42},
TestEvent{"CheckModuleSubscription", true},
TestEvent{"Test ended", nullptr},
});
}
Expand Down
267 changes: 197 additions & 70 deletions vnext/Microsoft.ReactNative/IReactNotificationService.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,77 +8,183 @@
namespace winrt::Microsoft::ReactNative::implementation {

//=============================================================================
// ReactNotificationSubscription implementation
// IReactNotificationSubscription implementation
//=============================================================================

ReactNotificationSubscription::ReactNotificationSubscription(
IReactNotificationSubscription const &parentSubscription,
weak_ref<ReactNotificationService> &&notificationService,
IReactPropertyName const &notificationName,
IReactDispatcher const &dispatcher) noexcept
: m_parentSubscription{parentSubscription},
m_notificationService{std::move(notificationService)},
m_notificationName{notificationName},
m_dispatcher{dispatcher} {}

ReactNotificationSubscription::ReactNotificationSubscription(
weak_ref<ReactNotificationService> &&notificationService,
IReactPropertyName const &notificationName,
IReactDispatcher const &dispatcher,
ReactNotificationHandler const &handler) noexcept
: m_notificationService{std::move(notificationService)},
m_notificationName{notificationName},
m_dispatcher{dispatcher},
m_handler{handler} {}

ReactNotificationSubscription::~ReactNotificationSubscription() noexcept {
Unsubscribe();
}
// Common interface to share functionality between ReactNotificationSubscription and ReactNotificationSubscriptionView
MSO_GUID(IReactNotificationSubscriptionPrivate, "09437980-3508-4690-930c-7c310e205e6b")
struct IReactNotificationSubscriptionPrivate : ::IUnknown {
virtual void SetParent(IReactNotificationSubscription const &parentSubscription) noexcept = 0;
virtual void CallHandler(
Windows::Foundation::IInspectable const &sender,
Windows::Foundation::IInspectable const &data) noexcept = 0;
};

// The Notification subscription class.
// Instances of this class are stored in the "child" notification services.
struct ReactNotificationSubscription
: implements<ReactNotificationSubscription, IReactNotificationSubscription, IReactNotificationSubscriptionPrivate> {
ReactNotificationSubscription(
Mso::RefCountedPtr<std::mutex> const &mutex,
weak_ref<ReactNotificationService> &&notificationService,
IReactPropertyName const &notificationName,
IReactDispatcher const &dispatcher,
ReactNotificationHandler const &handler) noexcept
: m_mutex{mutex},
m_notificationService{std::move(notificationService)},
m_notificationName{notificationName},
m_dispatcher{dispatcher},
m_handler{handler} {}

~ReactNotificationSubscription() noexcept {
Unsubscribe();
}

IReactNotificationService ReactNotificationSubscription::NotificationService() const noexcept {
return m_notificationService.get().as<IReactNotificationService>();
}
public: // IReactNotificationSubscription implementation
IReactNotificationService NotificationService() const noexcept {
return m_notificationService.get().as<IReactNotificationService>();
}

IReactPropertyName ReactNotificationSubscription::NotificationName() const noexcept {
return m_notificationName;
}
IReactPropertyName NotificationName() const noexcept {
return m_notificationName;
}

IReactDispatcher ReactNotificationSubscription::Dispatcher() const noexcept {
return m_dispatcher;
}
IReactDispatcher Dispatcher() const noexcept {
return m_dispatcher;
}

bool ReactNotificationSubscription::IsSubscribed() const noexcept {
return m_isSubscribed;
}
bool IsSubscribed() const noexcept {
return GetHandler() != nullptr;
}

void ReactNotificationSubscription::Unsubscribe() noexcept {
if (m_parentSubscription) {
m_parentSubscription.Unsubscribe();
void Unsubscribe() noexcept {
if (m_parentSubscription) {
m_parentSubscription.Unsubscribe();
}

bool isSubscribed{false};
{
std::scoped_lock lock{*m_mutex};
if (m_handler) {
isSubscribed = true;
// Remove handler to free any objects captured by it.
m_handler = nullptr;
}
}

if (isSubscribed) {
if (auto notificationService = m_notificationService.get()) {
notificationService->Unsubscribe(*this);
}
}
}

public: // IReactNotificationSubscriptionPrivate implementation
void SetParent(IReactNotificationSubscription const &parentSubscription) noexcept override {
m_parentSubscription = parentSubscription;
}

if (m_isSubscribed.exchange(false)) {
if (auto notificationService = m_notificationService.get()) {
notificationService->Unsubscribe(*this);
void CallHandler(IInspectable const &sender, IInspectable const &data) noexcept override {
auto args = make<ReactNotificationArgs>(*this, data);
if (auto handler = GetHandler()) {
if (m_dispatcher) {
m_dispatcher.Post([thisPtr = get_strong(), sender, args]() noexcept {
if (auto handler = thisPtr->GetHandler()) {
handler(sender, args);
}
});
} else {
handler(sender, args);
}
}
}
}

void ReactNotificationSubscription::CallHandler(
IInspectable const &sender,
IReactNotificationArgs const &args) noexcept {
VerifyElseCrashSz(!m_parentSubscription, "CallHandler must not be called on the child subscription.");
if (IsSubscribed()) {
if (m_dispatcher) {
m_dispatcher.Post([thisPtr = get_strong(), sender, args]() noexcept {
if (thisPtr->IsSubscribed()) {
thisPtr->m_handler(sender, args);
}
});
private:
ReactNotificationHandler GetHandler() const noexcept {
std::scoped_lock lock{*m_mutex};
return m_handler;
}

private:
Mso::RefCountedPtr<std::mutex> m_mutex;
IReactNotificationSubscription m_parentSubscription{nullptr};
const weak_ref<ReactNotificationService> m_notificationService{nullptr};
const IReactPropertyName m_notificationName{nullptr};
const IReactDispatcher m_dispatcher{nullptr};
ReactNotificationHandler m_handler{nullptr};
};

// The notification subscription view to wrap up child notification service.
// Instances of this class are stored in the parent notification services.
struct ReactNotificationSubscriptionView : implements<
ReactNotificationSubscriptionView,
IReactNotificationSubscription,
IReactNotificationSubscriptionPrivate> {
ReactNotificationSubscriptionView(
weak_ref<ReactNotificationService> &&notificationService,
IReactNotificationSubscription const &childSubscription) noexcept
: m_notificationService{std::move(notificationService)}, m_childSubscription{weak_ref(childSubscription)} {
childSubscription.as<IReactNotificationSubscriptionPrivate>()->SetParent(*this);
}

~ReactNotificationSubscriptionView() noexcept {
Unsubscribe();
}

public: // IReactNotificationSubscription implementation
IReactNotificationService NotificationService() const noexcept {
return m_notificationService.get().as<IReactNotificationService>();
}

IReactPropertyName NotificationName() const noexcept {
if (auto childSubscription = m_childSubscription.get()) {
return childSubscription.NotificationName();
} else {
m_handler(sender, args);
return IReactPropertyName{nullptr};
}
}
}

IReactDispatcher Dispatcher() const noexcept {
if (auto childSubscription = m_childSubscription.get()) {
return childSubscription.Dispatcher();
} else {
return IReactDispatcher{nullptr};
}
}

bool IsSubscribed() const noexcept {
return m_isSubscribed;
}

void Unsubscribe() noexcept {
if (m_parentSubscription) {
m_parentSubscription.Unsubscribe();
}

if (m_isSubscribed.exchange(false)) {
if (auto notificationService = m_notificationService.get()) {
notificationService->Unsubscribe(*this);
}
}
}

public: // IReactNotificationSubscriptionPrivate implementation
void SetParent(IReactNotificationSubscription const &parentSubscription) noexcept override {
m_parentSubscription = parentSubscription;
}

void CallHandler(IInspectable const &sender, IInspectable const &data) noexcept override {
if (auto childSubscription = m_childSubscription.get()) {
childSubscription.as<IReactNotificationSubscriptionPrivate>()->CallHandler(sender, data);
}
}

private:
IReactNotificationSubscription m_parentSubscription{nullptr};
const weak_ref<IReactNotificationSubscription> m_childSubscription{nullptr};
const weak_ref<ReactNotificationService> m_notificationService{nullptr};
std::atomic_bool m_isSubscribed{true};
};

//=============================================================================
// ReactNotificationService implementation
Expand All @@ -99,7 +205,7 @@ void ReactNotificationService::ModifySubscriptions(
// Get the current snapshot under the lock
SubscriptionSnapshotPtr currentSnapshotPtr;
{
std::scoped_lock lock{m_mutex};
std::scoped_lock lock{*m_mutex};
auto it = m_subscriptions.find(notificationName);
if (it != m_subscriptions.end()) {
currentSnapshotPtr = it->second;
Expand All @@ -113,7 +219,7 @@ void ReactNotificationService::ModifySubscriptions(

// Try to set the new snapshot under the lock
SubscriptionSnapshotPtr snapshotPtr;
std::scoped_lock lock{m_mutex};
std::scoped_lock lock{*m_mutex};
auto it = m_subscriptions.find(notificationName);
if (it != m_subscriptions.end()) {
snapshotPtr = it->second;
Expand Down Expand Up @@ -146,20 +252,42 @@ IReactNotificationSubscription ReactNotificationService::Subscribe(
IReactPropertyName const &notificationName,
IReactDispatcher const &dispatcher,
ReactNotificationHandler const &handler) noexcept {
// Make sure that parent notification service also subscribes to this notification.
auto parentSubscription = m_parentNotificationService
? m_parentNotificationService.Subscribe(notificationName, dispatcher, handler)
: IReactNotificationSubscription{nullptr};
auto subscription = parentSubscription
? make<ReactNotificationSubscription>(parentSubscription, get_weak(), notificationName, dispatcher)
: make<ReactNotificationSubscription>(get_weak(), notificationName, dispatcher, handler);
VerifyElseCrashSz(notificationName, "notificationName must be not null");
VerifyElseCrashSz(handler, "handler must be not null");

IReactNotificationSubscription subscription =
make<ReactNotificationSubscription>(m_mutex, get_weak(), notificationName, dispatcher, handler);
AddSubscription(notificationName, subscription);
AddSubscriptionToParent(notificationName, subscription);
return subscription;
}

void ReactNotificationService::AddSubscription(
IReactPropertyName const &notificationName,
IReactNotificationSubscription const &subscription) noexcept {
ModifySubscriptions(
notificationName, [&subscription](std::vector<IReactNotificationSubscription> const &snapshot) noexcept {
auto newSnapshot = std::vector<IReactNotificationSubscription>(snapshot);
newSnapshot.push_back(subscription);
return newSnapshot;
});
return subscription;
}

void ReactNotificationService::AddSubscriptionToParent(
IReactPropertyName const &notificationName,
IReactNotificationSubscription const &subscription) noexcept {
if (m_parentNotificationService) {
get_self<ReactNotificationService>(m_parentNotificationService)
->AddSubscriptionFromChild(notificationName, subscription);
}
}

void ReactNotificationService::AddSubscriptionFromChild(
IReactPropertyName const &notificationName,
IReactNotificationSubscription const &childSubscription) noexcept {
auto subscription = make<ReactNotificationSubscriptionView>(get_weak(), childSubscription);
AddSubscription(notificationName, subscription);
AddSubscriptionToParent(notificationName, subscription);
}

void ReactNotificationService::Unsubscribe(IReactNotificationSubscription const &subscription) noexcept {
Expand All @@ -180,7 +308,7 @@ void ReactNotificationService::UnsubscribeAll() noexcept {
// The subscription will call the parent Unsubscribe on its own.
decltype(m_subscriptions) subscriptions;
{
std::scoped_lock lock{m_mutex};
std::scoped_lock lock{*m_mutex};
subscriptions = std::move(m_subscriptions);
}

Expand All @@ -203,7 +331,7 @@ void ReactNotificationService::SendNotification(
SubscriptionSnapshotPtr currentSnapshotPtr;

{
std::scoped_lock lock{m_mutex};
std::scoped_lock lock{*m_mutex};
auto it = m_subscriptions.find(notificationName);
if (it != m_subscriptions.end()) {
currentSnapshotPtr = it->second;
Expand All @@ -213,8 +341,7 @@ void ReactNotificationService::SendNotification(
// Call notification handlers outside of lock.
if (currentSnapshotPtr) {
for (auto &subscription : *currentSnapshotPtr) {
auto args = make<ReactNotificationArgs>(subscription, data);
get_self<ReactNotificationSubscription>(subscription)->CallHandler(sender, args);
subscription.as<IReactNotificationSubscriptionPrivate>()->CallHandler(sender, data);
}
}
}
Expand Down

0 comments on commit 047ab1e

Please sign in to comment.