Skip to content

Commit 461edd2

Browse files
motiz88facebook-github-bot
motiz88
authored and
facebook-github-bot
committed
Allow ConnectFunc to reject connections by returning nullptr (#42387)
Summary: Pull Request resolved: #42387 Changelog: [Internal] Documents that it's legal for a Page's connection function to return null, and adds new logic to `InspectorPackagerConnection` (NOTE: to the C++ implementation *only*) to handle this case without crashing. The legacy RN CDP backend (`ConnectionDemux`) has a case similar to this that causes crashes depending on the timing of connection requests. Reviewed By: cortinico Differential Revision: D52905490 fbshipit-source-id: 2102adc859d1509647a31f92737a1e164781fadf
1 parent eb94727 commit 461edd2

File tree

3 files changed

+165
-3
lines changed

3 files changed

+165
-3
lines changed

‎packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.h‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,15 @@ class JSINSPECTOR_EXPORT IInspector : public IDestructible {
9393

9494
virtual ~IInspector() = 0;
9595

96-
/// addPage is called by the VM to add a page to the list of debuggable pages.
96+
/**
97+
* Add a page to the list of inspectable pages.
98+
* Callers are responsible for calling removePage when the page is no longer
99+
* expecting connections.
100+
* \param connectFunc a function that will be called to establish a
101+
* connection. \c connectFunc may return nullptr to reject the connection
102+
* (e.g. if the page is in the process of shutting down).
103+
* \returns the ID assigned to the new page.
104+
*/
97105
virtual int addPage(
98106
const std::string& title,
99107
const std::string& vm,
@@ -107,8 +115,12 @@ class JSINSPECTOR_EXPORT IInspector : public IDestructible {
107115
/// getPages is called by the client to list all debuggable pages.
108116
virtual std::vector<InspectorPageDescription> getPages() const = 0;
109117

110-
/// connect is called by the client to initiate a debugging session on the
111-
/// given page.
118+
/**
119+
* Called by InspectorPackagerConnection to initiate a debugging session with
120+
* the given page.
121+
* \returns an ILocalConnection that can be used to send messages to the
122+
* page, or nullptr if the connection has been rejected.
123+
*/
112124
virtual std::unique_ptr<ILocalConnection> connect(
113125
int pageId,
114126
std::unique_ptr<IRemoteConnection> remote) = 0;

‎packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.cpp‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,16 @@ void InspectorPackagerConnection::Impl::handleConnect(
101101
auto& inspector = getInspectorInstance();
102102
auto inspectorConnection =
103103
inspector.connect(pageIdInt, std::move(remoteConnection));
104+
if (!inspectorConnection) {
105+
LOG(INFO) << "Connection to page " << pageId << " rejected";
106+
107+
// RemoteConnection::onDisconnect(), if the connection even calls it, will
108+
// be a no op (because the session is not added to `inspectorSessions_`), so
109+
// let's always notify the remote client of the disconnection ourselves.
110+
sendToPackager(folly::dynamic::object("event", "disconnect")(
111+
"payload", folly::dynamic::object("pageId", pageId)));
112+
return;
113+
}
104114
inspectorSessions_.emplace(
105115
pageId,
106116
Session{

‎packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorPackagerConnectionTest.cpp‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1196,4 +1196,144 @@ TEST_F(
11961196
EXPECT_CALL(*localConnections_[1], disconnect()).RetiresOnSaturation();
11971197
getInspectorInstance().removePage(pageId);
11981198
}
1199+
1200+
TEST_F(InspectorPackagerConnectionTest, TestRejectedPageConnection) {
1201+
// Configure gmock to expect calls in a specific order.
1202+
InSequence mockCallsMustBeInSequence;
1203+
1204+
enum {
1205+
Accept,
1206+
RejectSilently,
1207+
RejectWithDisconnect
1208+
} mockNextConnectionBehavior;
1209+
1210+
auto pageId = getInspectorInstance().addPage(
1211+
"mock-title",
1212+
"mock-vm",
1213+
[&mockNextConnectionBehavior,
1214+
this](auto remoteConnection) -> std::unique_ptr<ILocalConnection> {
1215+
switch (mockNextConnectionBehavior) {
1216+
case Accept:
1217+
return localConnections_.make_unique(std::move(remoteConnection));
1218+
case RejectSilently:
1219+
return nullptr;
1220+
case RejectWithDisconnect:
1221+
remoteConnection->onDisconnect();
1222+
return nullptr;
1223+
}
1224+
});
1225+
1226+
packagerConnection_->connect();
1227+
1228+
ASSERT_TRUE(webSockets_[0]);
1229+
1230+
// Reject the connection by returning nullptr.
1231+
mockNextConnectionBehavior = RejectSilently;
1232+
1233+
EXPECT_CALL(
1234+
*webSockets_[0],
1235+
send(JsonParsed(AllOf(
1236+
AtJsonPtr("/event", Eq("disconnect")),
1237+
AtJsonPtr("/payload/pageId", Eq(std::to_string(pageId)))))))
1238+
.RetiresOnSaturation();
1239+
1240+
webSockets_[0]->getDelegate().didReceiveMessage(sformat(
1241+
R"({{
1242+
"event": "connect",
1243+
"payload": {{
1244+
"pageId": {0}
1245+
}}
1246+
}})",
1247+
toJson(std::to_string(pageId))));
1248+
1249+
webSockets_[0]->getDelegate().didReceiveMessage(sformat(
1250+
R"({{
1251+
"event": "wrappedEvent",
1252+
"payload": {{
1253+
"pageId": {0},
1254+
"wrappedEvent": {1}
1255+
}}
1256+
}})",
1257+
toJson(std::to_string(pageId)),
1258+
toJson(R"({
1259+
"method": "FakeDomain.fakeMethod",
1260+
"id": 1,
1261+
"params": ["arg1", "arg2"]
1262+
})")));
1263+
1264+
// Reject the connection by explicitly calling onDisconnect(), then returning
1265+
// nullptr.
1266+
mockNextConnectionBehavior = RejectWithDisconnect;
1267+
1268+
EXPECT_CALL(
1269+
*webSockets_[0],
1270+
send(JsonParsed(AllOf(
1271+
AtJsonPtr("/event", Eq("disconnect")),
1272+
AtJsonPtr("/payload/pageId", Eq(std::to_string(pageId)))))))
1273+
.RetiresOnSaturation();
1274+
1275+
webSockets_[0]->getDelegate().didReceiveMessage(sformat(
1276+
R"({{
1277+
"event": "connect",
1278+
"payload": {{
1279+
"pageId": {0}
1280+
}}
1281+
}})",
1282+
toJson(std::to_string(pageId))));
1283+
1284+
webSockets_[0]->getDelegate().didReceiveMessage(sformat(
1285+
R"({{
1286+
"event": "wrappedEvent",
1287+
"payload": {{
1288+
"pageId": {0},
1289+
"wrappedEvent": {1}
1290+
}}
1291+
}})",
1292+
toJson(std::to_string(pageId)),
1293+
toJson(R"({
1294+
"method": "FakeDomain.fakeMethod",
1295+
"id": 2,
1296+
"params": ["arg1", "arg2"]
1297+
})")));
1298+
1299+
// Accept a connection after previously rejecting connections to the same
1300+
// page.
1301+
mockNextConnectionBehavior = Accept;
1302+
1303+
webSockets_[0]->getDelegate().didReceiveMessage(sformat(
1304+
R"({{
1305+
"event": "connect",
1306+
"payload": {{
1307+
"pageId": {0}
1308+
}}
1309+
}})",
1310+
toJson(std::to_string(pageId))));
1311+
1312+
EXPECT_CALL(
1313+
*localConnections_[0],
1314+
sendMessage(JsonParsed(AllOf(
1315+
AtJsonPtr("/method", Eq("FakeDomain.fakeMethod")),
1316+
AtJsonPtr("/id", Eq(3)),
1317+
AtJsonPtr("/params", ElementsAre("arg1", "arg2"))))))
1318+
.RetiresOnSaturation();
1319+
1320+
webSockets_[0]->getDelegate().didReceiveMessage(sformat(
1321+
R"({{
1322+
"event": "wrappedEvent",
1323+
"payload": {{
1324+
"pageId": {0},
1325+
"wrappedEvent": {1}
1326+
}}
1327+
}})",
1328+
toJson(std::to_string(pageId)),
1329+
toJson(R"({
1330+
"method": "FakeDomain.fakeMethod",
1331+
"id": 3,
1332+
"params": ["arg1", "arg2"]
1333+
})")));
1334+
1335+
EXPECT_CALL(*localConnections_[0], disconnect()).RetiresOnSaturation();
1336+
getInspectorInstance().removePage(pageId);
1337+
}
1338+
11991339
} // namespace facebook::react::jsinspector_modern

0 commit comments

Comments
 (0)