Avoid capturing raw `this` pointer in WebSocket module #9247

JunielKatarn · 2021-12-08T10:17:36Z

Description

Defines WebSocketModule::SharedState struct.
Enforces weak-pointer-only captures within WebSocketModule.
Removes URL as a member of IWebSocketResource implementations.

Type of Change

Bug fix

Why

CxxModules frequently pass their resources to asynchronous tasks by capturing the raw this pointer, which can lead to invalid memory access attempts due to race conditions between the thread owning the module and threads executing the asynchronous work defined in getMethods().

Microsoft Reviewers: Open in CodeFlow

vnext/Shared/Modules/WebSocketModule.cpp

… cxxmodule-holder

…-windows into cxxmodule-holder

vnext/Desktop.UnitTests/BaseWebSocketTests.cpp

vnext/Desktop.UnitTests/WinRTWebSocketResourceUnitTest.cpp

vnext/Shared/Modules/WebSocketModule.cpp

NikoAri

… cxxmodule-holder

vnext/Desktop/BeastWebSocketResource.cpp

vmoroz · 2021-12-09T23:29:50Z

m_stream->set_option(websocket::stream_base::decorator([options = std::move(options)](websocket::request_type &req) {

options are const& - we cannot move them

Refers to: vnext/Desktop/BeastWebSocketResource.cpp:410 in 3564788. [](commit_id = 3564788, deletion_comment = False)

vmoroz · 2021-12-09T23:31:24Z

m_connectHandler = handler;

The handler is passed as an r-value reference, but to use the move-semantic we must use the std::move() here and in the methods below.

Refers to: vnext/Desktop/BeastWebSocketResource.cpp:523 in 3564788. [](commit_id = 3564788, deletion_comment = False)

vnext/Shared/WinRTWebSocketResource.cpp

vmoroz

ghost · 2021-12-10T00:50:55Z

Hello @JunielKatarn!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

* 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>

* 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>

* 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 * Remove vnext/Shared/InspectorPackagerConnection.cpp * Empty Co-authored-by: Nick Gerleman <ngerlem@microsoft.com>

JunielKatarn added 4 commits December 8, 2021 00:57

Use implicit (brace) constructor for getMethods()

ddd6831

Define CxxModuleHolder

80baef1

Make m_asyncStorageManager shared_ptr

5a1c985

Change files

4b5eb21

JunielKatarn requested a review from a team as a code owner December 8, 2021 10:17

JunielKatarn requested a review from a team December 8, 2021 10:17

NikoAri reviewed Dec 8, 2021

View reviewed changes

vnext/Shared/Modules/WebSocketModule.cpp Outdated Show resolved Hide resolved

JunielKatarn added 2 commits December 8, 2021 15:55

Initialize m_holder

4331602

Implement WebSocketModule::SharedState

5577952

JunielKatarn changed the title ~~[DO NOT MERGE YET] Avoid capturing raw this pointer in WS and AsyncStorage~~ Avoid capturing raw this pointer in WS and AsyncStorage Dec 9, 2021

JunielKatarn changed the title ~~Avoid capturing raw this pointer in WS and AsyncStorage~~ Avoid capturing raw this pointer in WS and AsyncStorage modules. Dec 9, 2021

JunielKatarn changed the title ~~Avoid capturing raw this pointer in WS and AsyncStorage modules.~~ Avoid capturing raw this pointer in WS and AsyncStorage modules Dec 9, 2021

JunielKatarn requested review from vmoroz, a team and NikoAri December 9, 2021 03:44

JunielKatarn and others added 5 commits December 8, 2021 19:59

Move ResourceFactory into shared state

3e8383f

Merge branch 'main' into cxxmodule-holder

d5d8edf

Revert AsyncStorage changes

a65c967

Merge branch 'main' of github.com:microsoft/react-native-windows into…

add33c8

… cxxmodule-holder

Merge branch 'cxxmodule-holder' of github.com:jurocha-ms/react-native…

b8c9c31

…-windows into cxxmodule-holder

JunielKatarn changed the title ~~Avoid capturing raw this pointer in WS and AsyncStorage modules~~ Avoid capturing raw this pointer in WebSocket module Dec 9, 2021

JunielKatarn added 2 commits December 9, 2021 00:34

Add URL argument to IWebSocketResource::Connect

d691029

Drop URL from factory method

1bedb25

JunielKatarn requested review from a team as code owners December 9, 2021 09:49

JunielKatarn added 3 commits December 9, 2021 03:36

Update BeastWebSocketResource

7d7bdcd

Handle malformed Uris within ::Connect()

564a504

Remove urlString argument from IWebSocketResource::Make

8d2502d

NikoAri reviewed Dec 9, 2021

View reviewed changes

vnext/Desktop.UnitTests/BaseWebSocketTests.cpp Outdated Show resolved Hide resolved

NikoAri reviewed Dec 9, 2021

View reviewed changes

vnext/Desktop.UnitTests/WinRTWebSocketResourceUnitTest.cpp Outdated Show resolved Hide resolved

NikoAri reviewed Dec 9, 2021

View reviewed changes

vnext/Shared/Modules/WebSocketModule.cpp Show resolved Hide resolved

NikoAri approved these changes Dec 9, 2021

View reviewed changes

JunielKatarn added 2 commits December 9, 2021 14:56

Use constexpr for commont test URLs

ee1c2a1

Merge branch 'main' of github.com:microsoft/react-native-windows into…

3564788

… cxxmodule-holder

NikoAri approved these changes Dec 9, 2021

View reviewed changes

vmoroz reviewed Dec 9, 2021

View reviewed changes

vnext/Desktop/BeastWebSocketResource.cpp Show resolved Hide resolved

vmoroz reviewed Dec 9, 2021

View reviewed changes

vnext/Shared/WinRTWebSocketResource.cpp Show resolved Hide resolved

vmoroz reviewed Dec 9, 2021

View reviewed changes

vnext/Shared/WinRTWebSocketResource.cpp Outdated Show resolved Hide resolved

vmoroz approved these changes Dec 9, 2021

View reviewed changes

JunielKatarn added 2 commits December 9, 2021 16:36

Remove redundant move in to_hstring

699e445

Add/remove std::move where applicable

494666d

JunielKatarn added Area: Networking AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) labels Dec 10, 2021

ghost merged commit 02f8b9f into microsoft:main Dec 10, 2021

JunielKatarn deleted the cxxmodule-holder branch December 10, 2021 01:22

This was referenced Dec 17, 2021

[0.67] Avoid capturing raw this pointer in WebSocket module #9296

Merged

[0.64] Avoid capturing raw this pointer in WebSocket module #9297

Merged

This was referenced Dec 17, 2021

[0.66] Avoid capturing raw this pointer in WebSocket module #9298

Merged

[0.65] Avoid capturing raw this pointer in WebSocket module #9299

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Avoid capturing raw `this` pointer in WebSocket module #9247

Avoid capturing raw `this` pointer in WebSocket module #9247

Uh oh!

JunielKatarn commented Dec 8, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NikoAri left a comment

Uh oh!

Uh oh!

vmoroz commented Dec 9, 2021

Uh oh!

vmoroz commented Dec 9, 2021

Uh oh!

Uh oh!

Uh oh!

vmoroz left a comment

Uh oh!

ghost commented Dec 10, 2021

Uh oh!

Uh oh!

Avoid capturing raw this pointer in WebSocket module #9247

Avoid capturing raw this pointer in WebSocket module #9247

Uh oh!

Conversation

JunielKatarn commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Why

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NikoAri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vmoroz commented Dec 9, 2021

Uh oh!

vmoroz commented Dec 9, 2021

Uh oh!

Uh oh!

Uh oh!

vmoroz left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 10, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Uh oh!

Avoid capturing raw `this` pointer in WebSocket module #9247

Avoid capturing raw `this` pointer in WebSocket module #9247

JunielKatarn commented Dec 8, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.