Skip to content

Commit 372ce65

Browse files
JunielKatarnNickGerleman
JunielKatarn
and
NickGerleman
authored
[0.67] Avoid capturing raw this pointer in WebSocket module (#9296)
* Avoid capturing raw `this` pointer in WebSocket module (#9247) * Use implicit (brace) constructor for getMethods() * Define CxxModuleHolder * Make m_asyncStorageManager shared_ptr * Change files * Initialize m_holder * Implement WebSocketModule::SharedState * Move ResourceFactory into shared state * Revert AsyncStorage changes * Add URL argument to IWebSocketResource::Connect * Drop URL from factory method * Update BeastWebSocketResource * Handle malformed Uris within ::Connect() * Remove urlString argument from IWebSocketResource::Make * Use constexpr for commont test URLs * Remove redundant move in to_hstring * Add/remove std::move where applicable Co-authored-by: Nick Gerleman <ngerlem@microsoft.com> * Use shared pointer in WebSocket MessageReceived (#9293) * Set MessageReceived to self-capturing lambda * clang format * Change files * Remove change files * Change files Co-authored-by: Nick Gerleman <ngerlem@microsoft.com>
1 parent b70d464 commit 372ce65

20 files changed

+459
-268
lines changed

‎change/react-native-windows-56c880d7-2062-4dd3-828a-a054307770a4.json

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"type": "prerelease",
3+
"comment": "[0.67] Avoid capturing raw this pointer in WebSocket module",
4+
"packageName": "react-native-windows",
5+
"email": "julio.rocha@microsoft.com",
6+
"dependentChangeType": "patch"
7+
}

‎vnext/Desktop.DLL/react-native-win32.x64.def

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ EXPORTS
2222
?GetConstants@ViewManagerBase@react@facebook@@UEBA?AUdynamic@folly@@XZ
2323
?InitializeLogging@react@facebook@@YAX$$QEAV?$function@$$A6AXW4RCTLogLevel@react@facebook@@PEBD@Z@std@@@Z
2424
?InitializeTracing@react@facebook@@YAXPEAUINativeTraceHandler@12@@Z
25-
?Make@IWebSocketResource@React@Microsoft@@SA?AV?$shared_ptr@UIWebSocketResource@React@Microsoft@@@std@@$$QEAV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@5@@Z
25+
?Make@IWebSocketResource@React@Microsoft@@SA?AV?$shared_ptr@UIWebSocketResource@React@Microsoft@@@std@@XZ
2626
?Make@JSBigAbiString@react@facebook@@SA?AV?$unique_ptr@$$CBUJSBigAbiString@react@facebook@@U?$default_delete@$$CBUJSBigAbiString@react@facebook@@@std@@@std@@$$QEAV?$unique_ptr@U?$IAbiArray@D@AbiSafe@@UAbiObjectDeleter@2@@5@@Z
2727
?at@dynamic@folly@@QEGBAAEBU12@V?$Range@PEBD@2@@Z
2828
?atImpl@dynamic@folly@@AEGBAAEBU12@AEBU12@@Z

‎vnext/Desktop.DLL/react-native-win32.x86.def

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ EXPORTS
2222
?GetConstants@ViewManagerBase@react@facebook@@UBE?AUdynamic@folly@@XZ
2323
?InitializeLogging@react@facebook@@YGX$$QAV?$function@$$A6GXW4RCTLogLevel@react@facebook@@PBD@Z@std@@@Z
2424
?InitializeTracing@react@facebook@@YGXPAUINativeTraceHandler@12@@Z
25-
?Make@IWebSocketResource@React@Microsoft@@SG?AV?$shared_ptr@UIWebSocketResource@React@Microsoft@@@std@@$$QAV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@5@@Z
25+
?Make@IWebSocketResource@React@Microsoft@@SG?AV?$shared_ptr@UIWebSocketResource@React@Microsoft@@@std@@XZ
2626
?Make@JSBigAbiString@react@facebook@@SG?AV?$unique_ptr@$$CBUJSBigAbiString@react@facebook@@U?$default_delete@$$CBUJSBigAbiString@react@facebook@@@std@@@std@@$$QAV?$unique_ptr@U?$IAbiArray@D@AbiSafe@@UAbiObjectDeleter@2@@5@@Z
2727
?moduleNames@ModuleRegistry@react@facebook@@QAE?AV?$vector@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V?$allocator@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@2@@std@@XZ
2828
?at@dynamic@folly@@QGBEABU12@V?$Range@PBD@2@@Z

‎vnext/Desktop.IntegrationTests/WebSocketIntegrationTest.cpp

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ TEST_CLASS (WebSocketIntegrationTest)
3939
string scheme = "ws";
4040
if (isSecure)
4141
scheme += "s";
42-
auto ws = IWebSocketResource::Make(scheme + "://localhost:5556/");
42+
auto ws = IWebSocketResource::Make();
4343
promise<size_t> sentSizePromise;
4444
ws->SetOnSend([&sentSizePromise](size_t size)
4545
{
@@ -61,7 +61,7 @@ TEST_CLASS (WebSocketIntegrationTest)
6161
server->Start();
6262
string sent = "prefix";
6363
auto expectedSize = sent.size();
64-
ws->Connect();
64+
ws->Connect(scheme + "://localhost:5556/");
6565
ws->Send(std::move(sent));
6666

6767
// Block until response is received. Fail in case of a remote endpoint failure.
@@ -85,7 +85,7 @@ TEST_CLASS (WebSocketIntegrationTest)
8585
TEST_METHOD(ConnectClose)
8686
{
8787
auto server = make_shared<Test::WebSocketServer>(5556);
88-
auto ws = IWebSocketResource::Make("ws://localhost:5556/");
88+
auto ws = IWebSocketResource::Make();
8989
Assert::IsFalse(nullptr == ws);
9090
bool connected = false;
9191
bool closed = false;
@@ -105,7 +105,7 @@ TEST_CLASS (WebSocketIntegrationTest)
105105
});
106106

107107
server->Start();
108-
ws->Connect();
108+
ws->Connect("ws://localhost:5556/");
109109
ws->Close(CloseCode::Normal, "Closing");
110110
server->Stop();
111111

@@ -124,7 +124,7 @@ TEST_CLASS (WebSocketIntegrationTest)
124124

125125
// IWebSocketResource scope. Ensures object is closed implicitly by destructor.
126126
{
127-
auto ws = IWebSocketResource::Make("ws://localhost:5556");
127+
auto ws = IWebSocketResource::Make();
128128
ws->SetOnConnect([&connected]()
129129
{
130130
connected = true;
@@ -138,7 +138,7 @@ TEST_CLASS (WebSocketIntegrationTest)
138138
errorMessage = error.Message;
139139
});
140140

141-
ws->Connect();
141+
ws->Connect("ws://localhost:5556");
142142
ws->Close();//TODO: Either remove or rename test.
143143
}
144144

@@ -156,7 +156,7 @@ TEST_CLASS (WebSocketIntegrationTest)
156156
auto server = make_shared<Test::WebSocketServer>(5556);
157157
server->Start();
158158

159-
auto ws = IWebSocketResource::Make("ws://localhost:5556");
159+
auto ws = IWebSocketResource::Make();
160160
promise<bool> pingPromise;
161161
ws->SetOnPing([&pingPromise]()
162162
{
@@ -168,7 +168,7 @@ TEST_CLASS (WebSocketIntegrationTest)
168168
errorString = err.Message;
169169
});
170170

171-
ws->Connect();
171+
ws->Connect("ws://localhost:5556");
172172
ws->Ping();
173173
auto pingFuture = pingPromise.get_future();
174174
pingFuture.wait();
@@ -189,14 +189,14 @@ TEST_CLASS (WebSocketIntegrationTest)
189189
TEST_METHOD(SendReceiveLargeMessage) {
190190
auto server = make_shared<Test::WebSocketServer>(5556);
191191
server->SetMessageFactory([](string &&message) { return message + "_response"; });
192-
auto ws = IWebSocketResource::Make("ws://localhost:5556/");
192+
auto ws = IWebSocketResource::Make();
193193
promise<string> response;
194194
ws->SetOnMessage([&response](size_t size, const string &message, bool isBinary) { response.set_value(message); });
195195
string errorMessage;
196196
ws->SetOnError([&errorMessage](Error err) { errorMessage = err.Message; });
197197

198198
server->Start();
199-
ws->Connect();
199+
ws->Connect("ws://localhost:5556");
200200

201201
char digits[] = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F'};
202202
#define LEN 4096 + 4096 * 2 + 1
@@ -246,12 +246,12 @@ TEST_CLASS (WebSocketIntegrationTest)
246246
auto cookie = response[boost::beast::http::field::cookie].to_string();
247247
server->SetMessageFactory([cookie](string &&) { return cookie; });
248248
});
249-
auto ws = IWebSocketResource::Make("ws://localhost:5556/");
249+
auto ws = IWebSocketResource::Make();
250250
promise<string> response;
251251
ws->SetOnMessage([&response](size_t size, const string &message, bool isBinary) { response.set_value(message); });
252252

253253
server->Start();
254-
ws->Connect({}, {{L"Cookie", "JSESSIONID=AD9A320CC4034641997FF903F1D10906"}});
254+
ws->Connect("ws://localhost:5556/", {}, {{L"Cookie", "JSESSIONID=AD9A320CC4034641997FF903F1D10906"}});
255255
ws->Send("");
256256

257257
auto future = response.get_future();
@@ -285,7 +285,7 @@ TEST_CLASS (WebSocketIntegrationTest)
285285
return message;
286286
});
287287

288-
auto ws = IWebSocketResource::Make("ws://localhost:5556/");
288+
auto ws = IWebSocketResource::Make();
289289

290290
std::vector<string> messages{
291291
"AQ==", // [ 01 ]
@@ -307,7 +307,7 @@ TEST_CLASS (WebSocketIntegrationTest)
307307
});
308308

309309
server->Start();
310-
ws->Connect();
310+
ws->Connect("ws://localhost:5556");
311311

312312
// Send all but the last message.
313313
// Compare result with the next message in the sequence.
@@ -336,7 +336,7 @@ TEST_CLASS (WebSocketIntegrationTest)
336336
{
337337
return message;
338338
});
339-
auto ws = IWebSocketResource::Make("ws://localhost:5556/");
339+
auto ws = IWebSocketResource::Make();
340340

341341
string expected = "ABCDEFGHIJ";
342342
string result(expected.size(), '0');
@@ -356,7 +356,7 @@ TEST_CLASS (WebSocketIntegrationTest)
356356
});
357357

358358
server->Start();
359-
ws->Connect();
359+
ws->Connect("ws://localhost:5556");
360360

361361
// Consecutive immediate writes should be enqueued.
362362
// The WebSocket library (WinRT or Beast) can't handle multiple write operations

‎vnext/Desktop.IntegrationTests/WebSocketResourcePerformanceTests.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ TEST_CLASS (WebSocketResourcePerformanceTest) {
7171
{
7272
vector<shared_ptr<IWebSocketResource>> resources;
7373
for (int i = 0; i < resourceTotal; i++) {
74-
auto ws = IWebSocketResource::Make("ws://localhost:5555/"); // TODO: Switch to port 5556
74+
auto ws = IWebSocketResource::Make();
7575
ws->SetOnMessage([this, &threadCount, &errorFound](size_t size, const string &message, bool isBinary) {
7676
if (errorFound)
7777
return;
@@ -103,7 +103,7 @@ TEST_CLASS (WebSocketResourcePerformanceTest) {
103103
errorFound = true;
104104
errorMessage = error.Message;
105105
});
106-
ws->Connect();
106+
ws->Connect("ws://localhost:5555"); // TODO: Switch to port 5556
107107

108108
resources.push_back(std::move(ws));
109109
} // Create and store WS resources.

‎vnext/Desktop.UnitTests/BaseWebSocketTests.cpp

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,12 @@ using std::make_unique;
1515
using std::promise;
1616
using std::string;
1717

18+
namespace {
19+
20+
constexpr char testUrl[] = "ws://localhost";
21+
22+
} // namespace
23+
1824
namespace Microsoft::React::Test {
1925

2026
using CloseCode = IWebSocketResource::CloseCode;
@@ -30,7 +36,7 @@ TEST_CLASS(BaseWebSocketTest){
3036
END_TEST_CLASS_ATTRIBUTE()
3137

3238
TEST_METHOD(CreateAndSetHandlers){
33-
auto ws = make_unique<TestWebSocketResource>(Url("ws://localhost"));
39+
auto ws = make_unique<TestWebSocketResource>(Url(testUrl));
3440

3541
Assert::IsFalse(nullptr == ws);
3642
ws->SetOnConnect([]() {});
@@ -44,11 +50,11 @@ TEST_CLASS(BaseWebSocketTest){
4450
TEST_METHOD(ConnectSucceeds) {
4551
string errorMessage;
4652
bool connected = false;
47-
auto ws = make_unique<TestWebSocketResource>(Url("ws://localhost"));
53+
auto ws = make_unique<TestWebSocketResource>(Url(testUrl));
4854
ws->SetOnError([&errorMessage](Error err) { errorMessage = err.Message; });
4955
ws->SetOnConnect([&connected]() { connected = true; });
5056

51-
ws->Connect({}, {});
57+
ws->Connect(testUrl, {}, {});
5258
ws->Close(CloseCode::Normal, {});
5359

5460
Assert::AreEqual({}, errorMessage);
@@ -58,14 +64,14 @@ TEST_CLASS(BaseWebSocketTest){
5864
TEST_METHOD(ConnectFails) {
5965
string errorMessage;
6066
bool connected = false;
61-
auto ws = make_unique<TestWebSocketResource>(Url("ws://localhost"));
67+
auto ws = make_unique<TestWebSocketResource>(Url(testUrl));
6268
ws->SetOnError([&errorMessage](Error err) { errorMessage = err.Message; });
6369
ws->SetOnConnect([&connected]() { connected = true; });
6470
ws->SetConnectResult([]() -> error_code {
6571
return make_error_code(errc::state_not_recoverable);
6672
});
6773

68-
ws->Connect({}, {});
74+
ws->Connect(testUrl, {}, {});
6975
ws->Close(CloseCode::Normal, {});
7076

7177
Assert::AreNotEqual({}, errorMessage);
@@ -78,14 +84,14 @@ TEST_CLASS(BaseWebSocketTest){
7884
TEST_METHOD(HandshakeFails) {
7985
string errorMessage;
8086
bool connected = false;
81-
auto ws = make_unique<TestWebSocketResource>(Url("ws://localhost"));
87+
auto ws = make_unique<TestWebSocketResource>(Url(testUrl));
8288
ws->SetOnError([&errorMessage](Error err) { errorMessage = err.Message; });
8389
ws->SetOnConnect([&connected]() { connected = true; });
8490
ws->SetHandshakeResult([](string, string) -> error_code {
8591
return make_error_code(errc::state_not_recoverable);
8692
});
8793

88-
ws->Connect({}, {});
94+
ws->Connect(testUrl, {}, {});
8995
ws->Close(CloseCode::Normal, {});
9096

9197
Assert::AreNotEqual({}, errorMessage);
@@ -99,7 +105,7 @@ TEST_CLASS(BaseWebSocketTest){
99105
string errorMessage;
100106
promise<void> connected;
101107
bool closed = false;
102-
auto ws = make_unique<TestWebSocketResource>(Url("ws://localhost"));
108+
auto ws = make_unique<TestWebSocketResource>(Url(testUrl));
103109
ws->SetOnError([&errorMessage](Error err) { errorMessage = err.Message; });
104110
ws->SetOnConnect([&connected]() { connected.set_value(); });
105111
ws->SetOnClose(
@@ -108,7 +114,7 @@ TEST_CLASS(BaseWebSocketTest){
108114
ws->SetCloseResult(
109115
[]() -> error_code { return make_error_code(errc::success); });
110116

111-
ws->Connect({}, {});
117+
ws->Connect(testUrl, {}, {});
112118
connected.get_future().wait();
113119

114120
ws->Close(CloseCode::Normal, "Normal");

‎vnext/Desktop.UnitTests/WebSocketMocks.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,13 @@ MockWebSocketResource::~MockWebSocketResource() {}
1010

1111
#pragma region IWebSocketResource overrides
1212

13-
void MockWebSocketResource::Connect(const Protocols &protocols, const Options &options) noexcept /*override*/
13+
void MockWebSocketResource::Connect(
14+
string &&url,
15+
const Protocols &protocols,
16+
const Options &options) noexcept /*override*/
1417
{
1518
if (Mocks.Connect)
16-
return Mocks.Connect(protocols, options);
19+
return Mocks.Connect(std::move(url), protocols, options);
1720
}
1821

1922
void MockWebSocketResource::Ping() noexcept /*override*/

‎vnext/Desktop.UnitTests/WebSocketMocks.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ struct MockWebSocketResource : public IWebSocketResource {
77
~MockWebSocketResource();
88

99
struct Mocks {
10-
std::function<void(const Protocols &, const Options &)> Connect;
10+
std::function<void(std::string &&url, const Protocols &, const Options &)> Connect;
1111
std::function<void()> Ping;
1212
std::function<void(const std::string &)> Send;
1313
std::function<void(const std::string &)> SendBinary;
@@ -25,7 +25,7 @@ struct MockWebSocketResource : public IWebSocketResource {
2525

2626
#pragma region IWebSocketResource overrides
2727

28-
void Connect(const Protocols &, const Options &) noexcept override;
28+
void Connect(std::string &&url, const Protocols &, const Options &) noexcept override;
2929

3030
void Ping() noexcept override;
3131

‎vnext/Desktop.UnitTests/WebSocketModuleTest.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ TEST_CLASS (WebSocketModuleTest) {
7474
module->setInstance(instance);
7575
module->SetResourceFactory([](const string &) {
7676
auto rc = make_shared<MockWebSocketResource>();
77-
rc->Mocks.Connect = [rc](const IWebSocketResource::Protocols &, const IWebSocketResource::Options &) {
77+
rc->Mocks.Connect = [rc](string &&, const IWebSocketResource::Protocols &, const IWebSocketResource::Options &) {
7878
rc->OnConnect();
7979
};
8080

‎vnext/Desktop.UnitTests/WinRTWebSocketResourceUnitTest.cpp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ IAsyncAction ThrowAsync() {
3232
co_return;
3333
}
3434

35+
constexpr char testUrl[] = "ws://host:0";
36+
3537
} // namespace
3638

3739
namespace Microsoft::React::Test {
@@ -51,12 +53,11 @@ TEST_CLASS (WinRTWebSocketResourceUnitTest) {
5153
mws->Mocks.Close = [](uint16_t, const hstring &) {};
5254

5355
// Test APIs
54-
auto rc =
55-
make_shared<WinRTWebSocketResource>(std::move(imws), MockDataWriter{}, Uri{L"ws://host:0"}, CertExceptions{});
56+
auto rc = make_shared<WinRTWebSocketResource>(std::move(imws), MockDataWriter{}, CertExceptions{});
5657
rc->SetOnConnect([&connected]() { connected = true; });
5758
rc->SetOnError([&errorMessage](Error &&error) { errorMessage = error.Message; });
5859

59-
rc->Connect({}, {});
60+
rc->Connect(testUrl, {}, {});
6061
rc->Close(CloseCode::Normal, {});
6162

6263
Assert::AreEqual({}, errorMessage);
@@ -77,12 +78,11 @@ TEST_CLASS (WinRTWebSocketResourceUnitTest) {
7778
mws->Mocks.Close = [](uint16_t, const hstring &) {};
7879

7980
// Test APIs
80-
auto rc =
81-
make_shared<WinRTWebSocketResource>(std::move(imws), MockDataWriter{}, Uri{L"ws://host:0"}, CertExceptions{});
81+
auto rc = make_shared<WinRTWebSocketResource>(std::move(imws), MockDataWriter{}, CertExceptions{});
8282
rc->SetOnConnect([&connected]() { connected = true; });
8383
rc->SetOnError([&errorMessage](Error &&error) { errorMessage = error.Message; });
8484

85-
rc->Connect({}, {});
85+
rc->Connect(testUrl, {}, {});
8686
rc->Close(CloseCode::Normal, {});
8787

8888
Assert::AreEqual({"[0x80004005] Expected Failure"}, errorMessage);
@@ -95,7 +95,7 @@ TEST_CLASS (WinRTWebSocketResourceUnitTest) {
9595

9696
auto lambda = [&rc]() mutable {
9797
rc = make_shared<WinRTWebSocketResource>(
98-
winrt::make<ThrowingMessageWebSocket>(), MockDataWriter{}, Uri{L"ws://host:0"}, CertExceptions{});
98+
winrt::make<ThrowingMessageWebSocket>(), MockDataWriter{}, CertExceptions{});
9999
};
100100

101101
Assert::ExpectException<winrt::hresult_error>(lambda);

0 commit comments

Comments
 (0)