Avoid capturing raw `this` pointer in WebSocket module by JunielKatarn · Pull Request #9247 · microsoft/react-native-windows

Julio César Rocha (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

… cxxmodule-holder

…-windows into cxxmodule-holder

Nikolai Aristov (NikoAri)

… cxxmodule-holder

Vladimir Morozov (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)

Vladimir Morozov (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)

Vladimir Morozov (vmoroz)

Deleted user (ghost) · 2021-12-10T00:50:55Z

Hello Julio César Rocha (@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>

Julio César Rocha (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

Julio César Rocha (JunielKatarn) requested a review from a team as a code owner December 8, 2021 10:17

Julio César Rocha (JunielKatarn) requested a review from a team December 8, 2021 10:17

Nikolai Aristov (NikoAri) reviewed Dec 8, 2021

View reviewed changes

Comment thread vnext/Shared/Modules/WebSocketModule.cpp Outdated

Julio César Rocha (JunielKatarn) added 2 commits December 8, 2021 15:55

Initialize m_holder

4331602

Implement WebSocketModule::SharedState

5577952

Julio César Rocha (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

Julio César Rocha (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

Julio César Rocha (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

Julio César Rocha (JunielKatarn) requested review from a team, Nikolai Aristov (NikoAri) and Vladimir Morozov (vmoroz) December 9, 2021 03:44

Julio César Rocha (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

Julio César Rocha (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

Julio César Rocha (JunielKatarn) added 2 commits December 9, 2021 00:34

Add URL argument to IWebSocketResource::Connect

d691029

Drop URL from factory method

1bedb25

Julio César Rocha (JunielKatarn) requested review from a team as code owners December 9, 2021 09:49

Julio César Rocha (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

Nikolai Aristov (NikoAri) reviewed Dec 9, 2021

View reviewed changes

Comment thread vnext/Desktop.UnitTests/BaseWebSocketTests.cpp Outdated

Nikolai Aristov (NikoAri) reviewed Dec 9, 2021

View reviewed changes

Comment thread vnext/Desktop.UnitTests/WinRTWebSocketResourceUnitTest.cpp Outdated

Nikolai Aristov (NikoAri) reviewed Dec 9, 2021

View reviewed changes

Comment thread vnext/Shared/Modules/WebSocketModule.cpp

Nikolai Aristov (NikoAri) approved these changes Dec 9, 2021

View reviewed changes

Julio César Rocha (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

Nikolai Aristov (NikoAri) approved these changes Dec 9, 2021

View reviewed changes

Vladimir Morozov (vmoroz) reviewed Dec 9, 2021

View reviewed changes

Comment thread vnext/Desktop/BeastWebSocketResource.cpp

Vladimir Morozov (vmoroz) reviewed Dec 9, 2021

View reviewed changes

Comment thread vnext/Shared/WinRTWebSocketResource.cpp

Vladimir Morozov (vmoroz) reviewed Dec 9, 2021

View reviewed changes

Comment thread vnext/Shared/WinRTWebSocketResource.cpp Outdated

Vladimir Morozov (vmoroz) approved these changes Dec 9, 2021

View reviewed changes

Julio César Rocha (JunielKatarn) added 2 commits December 9, 2021 16:36

Remove redundant move in to_hstring

699e445

Add/remove std::move where applicable

494666d

Julio César Rocha (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

Julio César Rocha (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.

Navigation Menu

Uh oh!

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

Avoid capturing raw `this` pointer in WebSocket module#9247
20 commits merged into
microsoft:mainfrom
jurocha-ms:cxxmodule-holder

Julio César Rocha (JunielKatarn) commented Dec 8, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Nikolai Aristov (NikoAri) left a comment

Uh oh!

Uh oh!

Vladimir Morozov (vmoroz) commented Dec 9, 2021

Uh oh!

Vladimir Morozov (vmoroz) commented Dec 9, 2021

Uh oh!

Uh oh!

Uh oh!

Vladimir Morozov (vmoroz) left a comment

Uh oh!

Deleted user (ghost) commented Dec 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

Julio César Rocha (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!

Nikolai Aristov (NikoAri) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Vladimir Morozov (vmoroz) commented Dec 9, 2021

Uh oh!

Vladimir Morozov (vmoroz) commented Dec 9, 2021

Uh oh!

Uh oh!

Uh oh!

Vladimir Morozov (vmoroz) left a comment

Choose a reason for hiding this comment

Uh oh!

Deleted user (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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Julio César Rocha (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.