Changeset 206875 in webkit

Timestamp:

Oct 6, 2016 12:45:52 PM (8 years ago)

Author:

BJ Burg

Message:

Web Inspector: RemoteInspector should cache client capabilities for off-main thread usage
​https://bugs.webkit.org/show_bug.cgi?id=163039
<rdar://problem/28571460>

Reviewed by Timothy Hatcher.

The fix in r206797 was incorrect because listings are always pushed out on the XPC connection queue.
Instead of delaying the listing needlessly, RemoteInspector should cache the capabilities of its
client while on the main thread, then use the cached struct data on the XPC connection queue rather
than directly accessing m_client. This is similar to how RemoteConnectionToTarget marshalls listing
information from arbitrary queues into m_targetListingMap, which can then be read from any queue.

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

(Inspector::RemoteInspector::updateClientCapabilities): Cache the capabilities.
(Inspector::RemoteInspector::setRemoteInspectorClient):
Re-cache the capabilities. Scope the lock to avoid reentrant locking.

(Inspector::RemoteInspector::clientCapabilitiesDidChange): Cache the capabilities.
(Inspector::RemoteInspector::pushListingsNow): Use cached client capabilities.
(Inspector::RemoteInspector::receivedGetListingMessage): Revert the change in r206797.
(Inspector::RemoteInspector::receivedAutomationSessionRequestMessage):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• inspector/remote/RemoteInspector.h (modified) (4 diffs)
• inspector/remote/RemoteInspector.mm (modified) (6 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2016-10-06  Brian Burg  <bburg@apple.com>
+
+        Web Inspector: RemoteInspector should cache client capabilities for off-main thread usage
+        https://bugs.webkit.org/show_bug.cgi?id=163039
+        <rdar://problem/28571460>
+
+        Reviewed by Timothy Hatcher.
+
+        The fix in r206797 was incorrect because listings are always pushed out on the XPC connection queue.
+        Instead of delaying the listing needlessly, RemoteInspector should cache the capabilities of its
+        client while on the main thread, then use the cached struct data on the XPC connection queue rather
+        than directly accessing m_client. This is similar to how RemoteConnectionToTarget marshalls listing
+        information from arbitrary queues into m_targetListingMap, which can then be read from any queue.
+
+        * inspector/remote/RemoteInspector.h:
+        * inspector/remote/RemoteInspector.mm:
+        (Inspector::RemoteInspector::updateClientCapabilities): Cache the capabilities.
+        (Inspector::RemoteInspector::setRemoteInspectorClient):
+        Re-cache the capabilities. Scope the lock to avoid reentrant locking.
+
+        (Inspector::RemoteInspector::clientCapabilitiesDidChange): Cache the capabilities.
+        (Inspector::RemoteInspector::pushListingsNow): Use cached client capabilities.
+        (Inspector::RemoteInspector::receivedGetListingMessage): Revert the change in r206797.
+        (Inspector::RemoteInspector::receivedAutomationSessionRequestMessage):
+
 2016-10-06  Yusuke Suzuki  <utatane.tea@gmail.com>
 

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

@@ -49 +49 @@
     class Client {
     public:
+        struct Capabilities {
+            bool remoteAutomationAllowed : 1;
+        };
+
         virtual ~Client() { }
         virtual bool remoteAutomationAllowed() const = 0;
@@ -63 +67 @@
     void sendMessageToRemote(unsigned targetIdentifier, const String& message);
 
-    void updateAutomaticInspectionCandidate(RemoteInspectionTarget*);
     void setRemoteInspectorClient(RemoteInspector::Client*);
+    void clientCapabilitiesDidChange();
 
     void setupFailed(unsigned targetIdentifier);
     void setupCompleted(unsigned targetIdentifier);
     bool waitingForAutomaticInspection(unsigned targetIdentifier);
-    void clientCapabilitiesDidChange() { pushListingsSoon(); }
+    void updateAutomaticInspectionCandidate(RemoteInspectionTarget*);
 
     bool enabled() const { return m_enabled; }
@@ -100 +104 @@
 
     void updateHasActiveDebugSession();
+    void updateClientCapabilities();
 
     void sendAutomaticInspectionCandidateMessage();
@@ -133 +138 @@
 
     RemoteInspector::Client* m_client { nullptr };
+    Optional<RemoteInspector::Client::Capabilities> m_clientCapabilities;
 
     dispatch_queue_t m_xpcQueue;

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

@@ -250 +250 @@
 }
 
+void RemoteInspector::updateClientCapabilities()
+{
+    ASSERT(isMainThread());
+
+    std::lock_guard<Lock> lock(m_mutex);
+
+    if (!m_client)
+        m_clientCapabilities = Nullopt;
+    else {
+        RemoteInspector::Client::Capabilities updatedCapabilities = {
+            m_client->remoteAutomationAllowed() // remoteAutomationAllowed
+        };
+
+        m_clientCapabilities = updatedCapabilities;
+    }
+}
+
 void RemoteInspector::setRemoteInspectorClient(RemoteInspector::Client* client)
 {
@@ -255 +272 @@
     ASSERT(!m_client);
 
-    std::lock_guard<Lock> lock(m_mutex);
-    m_client = client;
+    {
+        std::lock_guard<Lock> lock(m_mutex);
+        m_client = client;
+    }
 
     // Send an updated listing that includes whether the client allows remote automation.
+    updateClientCapabilities();
     pushListingsSoon();
 }
@@ -320 +340 @@
     // We don't take the lock to check this because we assume it will be checked repeatedly.
     return m_automaticInspectionPaused;
+}
+
+void RemoteInspector::clientCapabilitiesDidChange()
+{
+    updateClientCapabilities();
+    pushListingsSoon();
 }
 
@@ -568 +594 @@
     [message setObject:listings.get() forKey:WIRListingKey];
 
-    BOOL isAllowed = m_client && m_client->remoteAutomationAllowed();
+    BOOL isAllowed = m_clientCapabilities && m_clientCapabilities->remoteAutomationAllowed;
     [message setObject:@(isAllowed) forKey:WIRRemoteAutomationEnabledKey];
 
@@ -697 +723 @@
 void RemoteInspector::receivedGetListingMessage(NSDictionary *)
 {
-    pushListingsSoon();
+    pushListingsNow();
 }
 
@@ -795 +821 @@
         return;
 
-    if (!m_client->remoteAutomationAllowed())
+    if (!m_clientCapabilities || !m_clientCapabilities->remoteAutomationAllowed)
         return;