Changeset 278413 in webkit

Timestamp:

Jun 3, 2021, 12:32:22 PM (5 years ago)

Author:

Patrick Angle

Message:

Web Inspector: [Cocoa] RemoteInspector won't connect to a new relay if it hasn't yet failed to communicate with a previously connected relay
​https://bugs.webkit.org/show_bug.cgi?id=226539

Reviewed by Devin Rousso.

RemoteInspector communicates with a relay daemon running on the same device in order to send updates like new
or removed inspectable targets and receive changes to settings like automatic debugging. The relay daemon then
communicates with a client that connects for debugging. Only one relay daemon should ever be running at a time,
and its lifecycle is managed separately from JavaScriptCore.

RemoteInspector holds a RefPtr to its connection to this relay, and only clears this pointer upon a failure to
communicate over the XPC connection or a known disconnection. However, it is possible, and in some cases likely
(for example the relay restarting from a brief client disconnection and reconnection), that we can be informed
of a newly launched relay being available while still thinking we are connected to the old relay, as we have not
yet sent a message and triggered a failure in the interim period of time.

To correct this we now send a simple message any time setupXPCConnectionIfNeeded is called if we have an
existing RefPtr to a relay connection in order to verify the connection is still functional. We now also retry
to connect to a relay upon failure in order to create a new connection to the current relay.

In order to prevent entering a retry loop where every subsequent retry's failure results in another retry
forever, a flag to retry connecting is set when a call to setupXPCConnectionIfNeeded is made while we already
have a RefPtr to a relay connection. On failure if we are in this special state we will retry once to connect
but subsequent failures will not automatically reattempt a connection.

• inspector/remote/RemoteInspector.h:
• inspector/remote/cocoa/RemoteInspectorCocoa.mm:

(Inspector::RemoteInspector::stopInternal):

• Clear the retry connection flag when stopping in an orderly fashion.

(Inspector::RemoteInspector::setupXPCConnectionIfNeeded):

• Set the retry connection flag and send a simple message if we already have a relay connection in order to make

sure the connection is either still valid or is torn down properly on failure.
(Inspector::RemoteInspector::xpcConnectionFailed):

• If the retry flag is set, schedule a retry and clear the retry flag.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• inspector/remote/RemoteInspector.h (modified) (1 diff)
• inspector/remote/cocoa/RemoteInspectorCocoa.mm (modified) (3 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2021-06-03  Patrick Angle  <pangle@apple.com>
+
+        Web Inspector: [Cocoa] `RemoteInspector` won't connect to a new relay if it hasn't yet failed to communicate with a previously connected relay
+        https://bugs.webkit.org/show_bug.cgi?id=226539
+
+        Reviewed by Devin Rousso.
+
+        `RemoteInspector` communicates with a relay daemon running on the same device in order to send updates like new
+        or removed inspectable targets and receive changes to settings like automatic debugging. The relay daemon then
+        communicates with a client that connects for debugging. Only one relay daemon should ever be running at a time,
+        and its lifecycle is managed separately from JavaScriptCore.
+
+        RemoteInspector holds a RefPtr to its connection to this relay, and only clears this pointer upon a failure to
+        communicate over the XPC connection or a known disconnection. However, it is possible, and in some cases likely
+        (for example the relay restarting from a brief client disconnection and reconnection), that we can be informed
+        of a newly launched relay being available while still thinking we are connected to the old relay, as we have not 
+        yet sent a message and triggered a failure in the interim period of time.
+
+        To correct this we now send a simple message any time `setupXPCConnectionIfNeeded` is called if we have an
+        existing RefPtr to a relay connection in order to verify the connection is still functional. We now also retry
+        to connect to a relay upon failure in order to create a new connection to the current relay.
+
+        In order to prevent entering a retry loop where every subsequent retry's failure results in another retry
+        forever, a flag to retry connecting is set when a call to setupXPCConnectionIfNeeded is made while we already
+        have a RefPtr to a relay connection. On failure if we are in this special state we will retry once to connect
+        but subsequent failures will not automatically reattempt a connection.
+
+        * inspector/remote/RemoteInspector.h:
+        * inspector/remote/cocoa/RemoteInspectorCocoa.mm:
+        (Inspector::RemoteInspector::stopInternal):
+        - Clear the retry connection flag when stopping in an orderly fashion.
+        (Inspector::RemoteInspector::setupXPCConnectionIfNeeded):
+        - Set the retry connection flag and send a simple message if we already have a relay connection in order to make
+        sure the connection is either still valid or is torn down properly on failure.
+        (Inspector::RemoteInspector::xpcConnectionFailed):
+        - If the retry flag is set, schedule a retry and clear the retry flag.
+
 2021-06-02  Robin Morisset  <rmorisset@apple.com>
 

trunk/Source/JavaScriptCore/inspector/remote/RemoteInspector.h

@@ -261 +261 @@
 #if PLATFORM(COCOA)
     RefPtr<RemoteInspectorXPCConnection> m_relayConnection;
+    bool m_shouldReconnectToRelayOnFailure { false };
 #endif
 #if USE(GLIB)

trunk/Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm

@@ -266 +266 @@
     }
 
+    m_shouldReconnectToRelayOnFailure = false;
+
     notify_cancel(m_notifyToken);
 }
@@ -273 +275 @@
     Locker locker { m_mutex };
 
-    if (m_relayConnection)
-        return;
+    if (m_relayConnection) {
+        m_shouldReconnectToRelayOnFailure = true;
+
+        // Send a simple message to make sure the connection is still open.
+        m_relayConnection->sendMessage(@"check", nil);
+        return;
+    }
 
     auto connection = adoptOSObject(xpc_connection_create_mach_service(WIRXPCMachPortName, m_xpcQueue, 0));
-    if (!connection)
-        return;
+    if (!connection) {
+        WTFLogAlways("RemoteInspector failed to create XPC connection.");
+        return;
+    }
 
     m_relayConnection = adoptRef(new RemoteInspectorXPCConnection(connection.get(), m_xpcQueue, this));
@@ -364 +373 @@
     // The XPC connection will close itself.
     m_relayConnection = nullptr;
+
+    if (!m_shouldReconnectToRelayOnFailure) {
+        WTFLogAlways("RemoteInspector XPC connection to relay failed.");
+        return;
+    }
+
+    m_shouldReconnectToRelayOnFailure = false;
+    WTFLogAlways("RemoteInspector XPC connection to relay failed, reconnecting in 1 second...");
+
+    // Schedule setting up a new connection, since we currently are holding a lock needed to create a new connection.
+    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 1 * NSEC_PER_SEC), dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+        RemoteInspector::singleton().setupXPCConnectionIfNeeded();
+    });
 }