Skip to content

Commit 78a8357

Browse files
pgregorr-amazonpgregorr-amazon
authored
tv-casting-app fixed memory issues around stopDiscovery function (#34758)
* tv-casting-app fixed memory issues around stopDiscovery function * Fixing style issues * Addressed comments by sharadb-amazon
1 parent c05da3f commit 78a8357

10 files changed

+181
-53
lines changed

‎examples/tv-casting-app/android/App/app/src/main/java/com/matter/casting/ConnectionExampleFragment.java

+5-1
Original file line numberDiff line numberDiff line change
@@ -160,13 +160,17 @@ public void handle(Void v) {
160160
Log.i(
161161
TAG,
162162
"Successfully connected to CastingPlayer with deviceId: "
163-
+ targetCastingPlayer.getDeviceId());
163+
+ targetCastingPlayer.getDeviceId()
164+
+ ", useCommissionerGeneratedPasscode: "
165+
+ useCommissionerGeneratedPasscode);
164166
getActivity()
165167
.runOnUiThread(
166168
() -> {
167169
connectionFragmentStatusTextView.setText(
168170
"Successfully connected to Casting Player with device name: "
169171
+ targetCastingPlayer.getDeviceName()
172+
+ "\n\nUsed CastingPlayer Passcode: "
173+
+ useCommissionerGeneratedPasscode
170174
+ "\n\n");
171175
connectionFragmentNextButton.setEnabled(true);
172176
});

‎examples/tv-casting-app/android/App/app/src/main/java/com/matter/casting/DiscoveryExampleFragment.java

+9-5
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,8 @@ public class DiscoveryExampleFragment extends Fragment {
6767
public void onAdded(CastingPlayer castingPlayer) {
6868
Log.i(
6969
TAG,
70-
"onAdded() Discovered CastingPlayer deviceId: " + castingPlayer.getDeviceId());
70+
"DiscoveryExampleFragment onAdded() Discovered CastingPlayer deviceId: "
71+
+ castingPlayer.getDeviceId());
7172
// Display CastingPlayer info on the screen
7273
new Handler(Looper.getMainLooper())
7374
.post(
@@ -80,7 +81,7 @@ public void onAdded(CastingPlayer castingPlayer) {
8081
public void onChanged(CastingPlayer castingPlayer) {
8182
Log.i(
8283
TAG,
83-
"onChanged() Discovered changes to CastingPlayer with deviceId: "
84+
"DiscoveryExampleFragment onChanged() Discovered changes to CastingPlayer with deviceId: "
8485
+ castingPlayer.getDeviceId());
8586
// Update the CastingPlayer on the screen
8687
new Handler(Looper.getMainLooper())
@@ -107,7 +108,7 @@ public void onChanged(CastingPlayer castingPlayer) {
107108
public void onRemoved(CastingPlayer castingPlayer) {
108109
Log.i(
109110
TAG,
110-
"onRemoved() Removed CastingPlayer with deviceId: "
111+
"DiscoveryExampleFragment onRemoved() Removed CastingPlayer with deviceId: "
111112
+ castingPlayer.getDeviceId());
112113
// Remove CastingPlayer from the screen
113114
new Handler(Looper.getMainLooper())
@@ -215,7 +216,10 @@ public void onResume() {
215216
@Override
216217
public void onPause() {
217218
super.onPause();
218-
Log.i(TAG, "onPause() called");
219+
Log.i(TAG, "DiscoveryExampleFragment onPause() called, calling stopDiscovery()");
220+
// Stop discovery when leaving the fragment, for example, while displaying the
221+
// ConnectionExampleFragment.
222+
stopDiscovery();
219223
}
220224

221225
/** Interface for notifying the host. */
@@ -261,7 +265,7 @@ private boolean startDiscovery() {
261265
}
262266

263267
private void stopDiscovery() {
264-
Log.i(TAG, "stopDiscovery() called");
268+
Log.i(TAG, "DiscoveryExampleFragment stopDiscovery() called");
265269
matterDiscoveryErrorMessageTextView.setText(
266270
getString(R.string.matter_discovery_error_message_initial));
267271

‎examples/tv-casting-app/android/App/app/src/main/res/layout/custom_passcode_dialog.xml

+1-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
<?xml version="1.0" encoding="utf-8"?>
22
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
3+
xmlns:tools="http://schemas.android.com/tools"
34
android:layout_width="match_parent"
45
android:layout_height="wrap_content"
56
android:orientation="vertical">
@@ -11,7 +12,6 @@
1112
android:textStyle="bold"
1213
android:textSize="18sp"
1314
android:padding="16dp"
14-
android:textColor="@android:color/white"
1515
android:gravity="center"
1616
android:text="@string/matter_connection_input_dialog_title" />
1717

@@ -24,7 +24,6 @@
2424
android:paddingEnd="16dp"
2525
android:paddingTop="8dp"
2626
android:paddingBottom="8dp"
27-
android:textColor="@android:color/white"
2827
android:text="@string/matter_connection_input_dialog_instructions" />
2928

3029
<EditText

‎examples/tv-casting-app/android/App/app/src/main/res/layout/fragment_matter_connection_example.xml

+22-11
Original file line numberDiff line numberDiff line change
@@ -6,18 +6,29 @@
66
tools:context=".matter.casting.ConnectionExampleFragment"
77
android:padding="10sp">
88

9-
<TextView
10-
android:id="@+id/connectionFragmentStatusText"
11-
android:layout_width="wrap_content"
12-
android:layout_height="wrap_content"
13-
android:textSize="24sp"/>
14-
15-
<Button
16-
android:enabled="false"
17-
android:id="@+id/connectionFragmentNextButton"
18-
android:layout_below="@id/connectionFragmentStatusText"
9+
<LinearLayout
10+
android:id="@+id/connectionFragmentLinearLayout"
1911
android:layout_width="match_parent"
2012
android:layout_height="wrap_content"
21-
android:text="@string/matter_connection_next_button_text" />
13+
android:orientation="vertical"
14+
tools:layout_editor_absoluteX="1dp"
15+
tools:layout_editor_absoluteY="1dp"
16+
android:padding="10sp">
17+
18+
<TextView
19+
android:id="@+id/connectionFragmentStatusText"
20+
android:layout_width="wrap_content"
21+
android:layout_height="wrap_content"
22+
android:textSize="24sp"/>
23+
24+
<Button
25+
android:enabled="false"
26+
android:id="@+id/connectionFragmentNextButton"
27+
android:layout_below="@id/connectionFragmentStatusText"
28+
android:layout_width="match_parent"
29+
android:layout_height="wrap_content"
30+
android:text="@string/matter_connection_next_button_text" />
31+
32+
</LinearLayout>
2233

2334
</RelativeLayout>

‎examples/tv-casting-app/tv-casting-common/core/CastingPlayer.cpp

+39-17
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
*/
1818

1919
#include "CastingPlayer.h"
20-
#include "Endpoint.h"
20+
#include "CastingPlayerDiscovery.h"
2121

2222
#include "support/CastingStore.h"
2323

@@ -27,13 +27,14 @@ namespace matter {
2727
namespace casting {
2828
namespace core {
2929

30-
CastingPlayer * CastingPlayer::mTargetCastingPlayer = nullptr;
30+
memory::Weak<CastingPlayer> CastingPlayer::mTargetCastingPlayer;
3131

3232
void CastingPlayer::VerifyOrEstablishConnection(ConnectionCallbacks connectionCallbacks, uint16_t commissioningWindowTimeoutSec,
3333
IdentificationDeclarationOptions idOptions)
3434
{
3535
ChipLogProgress(AppServer, "CastingPlayer::VerifyOrEstablishConnection() called");
3636

37+
CastingPlayerDiscovery * castingPlayerDiscovery = CastingPlayerDiscovery::GetInstance();
3738
std::vector<core::CastingPlayer>::iterator it;
3839
std::vector<core::CastingPlayer> cachedCastingPlayers = support::CastingStore::GetInstance()->ReadAll();
3940

@@ -53,8 +54,9 @@ void CastingPlayer::VerifyOrEstablishConnection(ConnectionCallbacks connectionCa
5354
mConnectionState = CASTING_PLAYER_CONNECTING;
5455
mOnCompleted = connectionCallbacks.mOnConnectionComplete;
5556
mCommissioningWindowTimeoutSec = commissioningWindowTimeoutSec;
56-
mTargetCastingPlayer = this;
57+
mTargetCastingPlayer = weak_from_this();
5758
mIdOptions = idOptions;
59+
castingPlayerDiscovery->ClearDisconnectedCastingPlayersInternal();
5860

5961
// Register the handler for Commissioner's CommissionerDeclaration messages. The CommissionerDeclaration messages provide
6062
// information indicating the Commissioner's pre-commissioning state.
@@ -120,7 +122,8 @@ void CastingPlayer::VerifyOrEstablishConnection(ConnectionCallbacks connectionCa
120122
nullptr,
121123
[](void * context, chip::Messaging::ExchangeManager & exchangeMgr, const chip::SessionHandle & sessionHandle) {
122124
ChipLogProgress(AppServer,
123-
"CastingPlayer::VerifyOrEstablishConnection() Connection to CastingPlayer successful");
125+
"CastingPlayer::VerifyOrEstablishConnection() FindOrEstablishSession Connection to "
126+
"CastingPlayer successful");
124127
CastingPlayer::GetTargetCastingPlayer()->mConnectionState = CASTING_PLAYER_CONNECTED;
125128

126129
// this async call will Load all the endpoints with their respective attributes into the TargetCastingPlayer
@@ -129,7 +132,9 @@ void CastingPlayer::VerifyOrEstablishConnection(ConnectionCallbacks connectionCa
129132
support::EndpointListLoader::GetInstance()->Load();
130133
},
131134
[](void * context, const chip::ScopedNodeId & peerId, CHIP_ERROR error) {
132-
ChipLogError(AppServer, "CastingPlayer::VerifyOrEstablishConnection() Connection to CastingPlayer failed");
135+
ChipLogError(AppServer,
136+
"CastingPlayer::VerifyOrEstablishConnection() FindOrEstablishSession Connection to "
137+
"CastingPlayer failed");
133138
CastingPlayer::GetTargetCastingPlayer()->mConnectionState = CASTING_PLAYER_NOT_CONNECTED;
134139
CHIP_ERROR e = support::CastingStore::GetInstance()->Delete(*CastingPlayer::GetTargetCastingPlayer());
135140
if (e != CHIP_NO_ERROR)
@@ -139,7 +144,7 @@ void CastingPlayer::VerifyOrEstablishConnection(ConnectionCallbacks connectionCa
139144

140145
VerifyOrReturn(CastingPlayer::GetTargetCastingPlayer()->mOnCompleted);
141146
CastingPlayer::GetTargetCastingPlayer()->mOnCompleted(error, nullptr);
142-
mTargetCastingPlayer = nullptr;
147+
mTargetCastingPlayer.reset();
143148
});
144149
return; // FindOrEstablishSession called. Return early.
145150
}
@@ -196,7 +201,7 @@ CHIP_ERROR CastingPlayer::ContinueConnecting()
196201
"only be called when the CastingPlayer/Commissioner-Generated passcode commissioning flow is in progress."););
197202

198203
CHIP_ERROR err = CHIP_NO_ERROR;
199-
mTargetCastingPlayer = this;
204+
mTargetCastingPlayer = weak_from_this();
200205

201206
ChipLogProgress(AppServer, "CastingPlayer::ContinueConnecting() calling OpenBasicCommissioningWindow()");
202207
SuccessOrExit(err = chip::Server::GetInstance().GetCommissioningWindowManager().OpenBasicCommissioningWindow(
@@ -235,7 +240,8 @@ CHIP_ERROR CastingPlayer::StopConnecting()
235240
mIdOptions.mCancelPasscode = true;
236241
mConnectionState = CASTING_PLAYER_NOT_CONNECTED;
237242
mCommissioningWindowTimeoutSec = kCommissioningWindowTimeoutSec;
238-
mTargetCastingPlayer = nullptr;
243+
mTargetCastingPlayer.reset();
244+
CastingPlayerDiscovery::GetInstance()->ClearCastingPlayersInternal();
239245

240246
// If a CastingPlayer::ContinueConnecting() error occurs, StopConnecting() can be called while sUdcInProgress == true.
241247
// sUdcInProgress should be set to false before sending the CancelPasscode IdentificationDeclaration message to the
@@ -273,18 +279,21 @@ void CastingPlayer::resetState(CHIP_ERROR err)
273279
support::ChipDeviceEventHandler::SetUdcStatus(false);
274280
mConnectionState = CASTING_PLAYER_NOT_CONNECTED;
275281
mCommissioningWindowTimeoutSec = kCommissioningWindowTimeoutSec;
276-
mTargetCastingPlayer = nullptr;
282+
mTargetCastingPlayer.reset();
277283
if (mOnCompleted)
278284
{
279285
mOnCompleted(err, nullptr);
280286
mOnCompleted = nullptr;
281287
}
288+
CastingPlayerDiscovery::GetInstance()->ClearCastingPlayersInternal();
282289
}
283290

284291
void CastingPlayer::Disconnect()
285292
{
286-
mConnectionState = CASTING_PLAYER_NOT_CONNECTED;
287-
mTargetCastingPlayer = nullptr;
293+
ChipLogProgress(AppServer, "CastingPlayer::Disconnect()");
294+
mConnectionState = CASTING_PLAYER_NOT_CONNECTED;
295+
mTargetCastingPlayer.reset();
296+
CastingPlayerDiscovery::GetInstance()->ClearCastingPlayersInternal();
288297
}
289298

290299
void CastingPlayer::RegisterEndpoint(const memory::Strong<Endpoint> endpoint)
@@ -320,6 +329,7 @@ CHIP_ERROR CastingPlayer::SendUserDirectedCommissioningRequest()
320329
ReturnErrorOnFailure(chip::Server::GetInstance().SendUserDirectedCommissioningRequest(
321330
chip::Transport::PeerAddress::UDP(*ipAddressToUse, mAttributes.port, mAttributes.interfaceId), id));
322331

332+
ChipLogProgress(AppServer, "CastingPlayer::SendUserDirectedCommissioningRequest() complete");
323333
return CHIP_NO_ERROR;
324334
}
325335

@@ -478,23 +488,35 @@ ConnectionContext::ConnectionContext(void * clientContext, core::CastingPlayer *
478488

479489
mOnConnectedCallback = new chip::Callback::Callback<chip::OnDeviceConnected>(
480490
[](void * context, chip::Messaging::ExchangeManager & exchangeMgr, const chip::SessionHandle & sessionHandle) {
481-
ChipLogProgress(AppServer, "Device Connection success callback called");
491+
ChipLogProgress(AppServer, "CastingPlayer::ConnectionContext() Device Connection success callback called");
482492
ConnectionContext * connectionContext = static_cast<ConnectionContext *>(context);
483-
VerifyOrReturn(connectionContext != nullptr && connectionContext->mTargetCastingPlayer != nullptr,
484-
ChipLogError(AppServer, "Invalid ConnectionContext received in DeviceConnection success callback"));
493+
VerifyOrReturn(
494+
connectionContext != nullptr && connectionContext->mTargetCastingPlayer != nullptr,
495+
ChipLogError(
496+
AppServer,
497+
"CastingPlayer::ConnectionContext() Invalid ConnectionContext received in DeviceConnection success callback"));
485498

499+
ChipLogProgress(AppServer,
500+
"CastingPlayer::ConnectionContext() calling mConnectionState = core::CASTING_PLAYER_CONNECTED");
486501
connectionContext->mTargetCastingPlayer->mConnectionState = core::CASTING_PLAYER_CONNECTED;
502+
ChipLogProgress(AppServer, "CastingPlayer::ConnectionContext() calling mOnDeviceConnectedFn");
487503
connectionContext->mOnDeviceConnectedFn(connectionContext->mClientContext, exchangeMgr, sessionHandle);
504+
ChipLogProgress(AppServer, "CastingPlayer::ConnectionContext() calling delete connectionContext");
488505
delete connectionContext;
489506
},
490507
this);
491508

492509
mOnConnectionFailureCallback = new chip::Callback::Callback<chip::OnDeviceConnectionFailure>(
493510
[](void * context, const chip::ScopedNodeId & peerId, CHIP_ERROR error) {
494-
ChipLogError(AppServer, "Device Connection failure callback called with %" CHIP_ERROR_FORMAT, error.Format());
511+
ChipLogError(AppServer,
512+
"CastingPlayer::ConnectionContext() Device Connection failure callback called with %" CHIP_ERROR_FORMAT,
513+
error.Format());
495514
ConnectionContext * connectionContext = static_cast<ConnectionContext *>(context);
496-
VerifyOrReturn(connectionContext != nullptr && connectionContext->mTargetCastingPlayer != nullptr,
497-
ChipLogError(AppServer, "Invalid ConnectionContext received in DeviceConnection failure callback"));
515+
VerifyOrReturn(
516+
connectionContext != nullptr && connectionContext->mTargetCastingPlayer != nullptr,
517+
ChipLogError(
518+
AppServer,
519+
"CastingPlayer::ConnectionContext() Invalid ConnectionContext received in DeviceConnection failure callback"));
498520
connectionContext->mTargetCastingPlayer->mConnectionState = CASTING_PLAYER_NOT_CONNECTED;
499521
connectionContext->mOnDeviceConnectionFailureFn(connectionContext->mClientContext, peerId, error);
500522
delete connectionContext;

‎examples/tv-casting-app/tv-casting-common/core/CastingPlayer.h

+32-2
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,28 @@ class CastingPlayer : public std::enable_shared_from_this<CastingPlayer>
100100
/**
101101
* @brief Get the CastingPlayer object targeted currently (may not be connected)
102102
*/
103-
static CastingPlayer * GetTargetCastingPlayer() { return mTargetCastingPlayer; }
103+
static CastingPlayer * GetTargetCastingPlayer()
104+
{
105+
ChipLogProgress(AppServer, "CastingPlayer::GetTargetCastingPlayer() called");
106+
std::shared_ptr<CastingPlayer> sharedPtr = mTargetCastingPlayer.lock();
107+
CastingPlayer * rawPtr = nullptr;
108+
if (sharedPtr)
109+
{
110+
rawPtr = sharedPtr.get();
111+
ChipLogProgress(
112+
AppServer,
113+
"CastingPlayer::GetTargetCastingPlayer() Got rawPtr from mTargetCastingPlayer, sharedPtr reference count: %lu",
114+
sharedPtr.use_count());
115+
sharedPtr.reset();
116+
}
117+
else
118+
{
119+
ChipLogError(AppServer,
120+
"CastingPlayer::GetTargetCastingPlayer() The shared pointer observed by mTargetCastingPlayer has expired "
121+
"(become nullptr)");
122+
}
123+
return rawPtr;
124+
}
104125

105126
/**
106127
* @brief Compares based on the Id
@@ -257,12 +278,21 @@ class CastingPlayer : public std::enable_shared_from_this<CastingPlayer>
257278

258279
void SetFabricIndex(chip::FabricIndex fabricIndex) { mAttributes.fabricIndex = fabricIndex; }
259280

281+
/**
282+
* @brief Return the current state of the CastingPlayer
283+
*/
284+
ConnectionState GetConnectionState() const { return mConnectionState; }
285+
260286
private:
261287
std::vector<memory::Strong<Endpoint>> mEndpoints;
262288
ConnectionState mConnectionState = CASTING_PLAYER_NOT_CONNECTED;
263289
CastingPlayerAttributes mAttributes;
264290
IdentificationDeclarationOptions mIdOptions;
265-
static CastingPlayer * mTargetCastingPlayer;
291+
// This is a std::weak_ptr. A std::weak_ptr is a non-owning reference to an object managed by one
292+
// or more std::shared_ptr instances. When the last std::shared_ptr instance that owns the managed
293+
// object is destroyed or reset, the object itself is automatically destroyed, and all
294+
// std::weak_ptr instances that reference that object become expired.
295+
static memory::Weak<CastingPlayer> mTargetCastingPlayer;
266296
uint16_t mCommissioningWindowTimeoutSec = kCommissioningWindowTimeoutSec;
267297
ConnectCallback mOnCompleted = {};
268298

0 commit comments

Comments
 (0)