<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[162910] trunk/Source/JavaScriptCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/162910">162910</a></dd>
<dt>Author</dt> <dd>commit-queue@webkit.org</dd>
<dt>Date</dt> <dd>2014-01-27 20:55:55 -0800 (Mon, 27 Jan 2014)</dd>
</dl>
<h3>Log Message</h3>
<pre>Web Inspector: CRASH when debugger closes remote inspecting JSContext
https://bugs.webkit.org/show_bug.cgi?id=127738
Patch by Joseph Pecoraro <pecoraro@apple.com> on 2014-01-27
Reviewed by Timothy Hatcher.
RemoteInspectorXPCConnection could be accessed in a background dispatch
queue, while being deallocated on the main thread when a connection
was suddenly terminated.
Make RemoteInspectorXPCConnection a ThreadSafeRefCounted object. Always
keep the connection object ref'd until the main thread calls close()
and removes its reference. At that point we can close the connection,
queue, and deref safely on the background queue.
* inspector/remote/RemoteInspector.h:
* inspector/remote/RemoteInspector.mm:
(Inspector::RemoteInspector::setupXPCConnectionIfNeeded):
(Inspector::RemoteInspector::xpcConnectionFailed):
For simplicity RemoteInspectorXPCConnections's don't have any threading
primatives to prevent client callbacks after they are closed. RemoteInspector
does, so it just ignores possible callbacks from connections it no longer
cares about.
* inspector/remote/RemoteInspectorXPCConnection.h:
* inspector/remote/RemoteInspectorXPCConnection.mm:
(Inspector::RemoteInspectorXPCConnection::RemoteInspectorXPCConnection):
(Inspector::RemoteInspectorXPCConnection::~RemoteInspectorXPCConnection):
(Inspector::RemoteInspectorXPCConnection::close):
Keep the connection alive as long as the queue it can be used on
is alive. Clean up everything on the queue when close() is called.
(Inspector::RemoteInspectorXPCConnection::handleEvent):
Checking if closed here is not thread safe so it is meaningless.
Remove the check.
(Inspector::RemoteInspectorXPCConnection::sendMessage):
Bail based on the m_closed state.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreinspectorremoteRemoteInspectorh">trunk/Source/JavaScriptCore/inspector/remote/RemoteInspector.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreinspectorremoteRemoteInspectormm">trunk/Source/JavaScriptCore/inspector/remote/RemoteInspector.mm</a></li>
<li><a href="#trunkSourceJavaScriptCoreinspectorremoteRemoteInspectorXPCConnectionh">trunk/Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreinspectorremoteRemoteInspectorXPCConnectionmm">trunk/Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.mm</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (162909 => 162910)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2014-01-28 04:04:56 UTC (rev 162909)
+++ trunk/Source/JavaScriptCore/ChangeLog 2014-01-28 04:55:55 UTC (rev 162910)
</span><span class="lines">@@ -1,5 +1,45 @@
</span><span class="cx"> 2014-01-27 Joseph Pecoraro <pecoraro@apple.com>
</span><span class="cx">
</span><ins>+ Web Inspector: CRASH when debugger closes remote inspecting JSContext
+ https://bugs.webkit.org/show_bug.cgi?id=127738
+
+ Reviewed by Timothy Hatcher.
+
+ RemoteInspectorXPCConnection could be accessed in a background dispatch
+ queue, while being deallocated on the main thread when a connection
+ was suddenly terminated.
+
+ Make RemoteInspectorXPCConnection a ThreadSafeRefCounted object. Always
+ keep the connection object ref'd until the main thread calls close()
+ and removes its reference. At that point we can close the connection,
+ queue, and deref safely on the background queue.
+
+ * inspector/remote/RemoteInspector.h:
+ * inspector/remote/RemoteInspector.mm:
+ (Inspector::RemoteInspector::setupXPCConnectionIfNeeded):
+ (Inspector::RemoteInspector::xpcConnectionFailed):
+ For simplicity RemoteInspectorXPCConnections's don't have any threading
+ primatives to prevent client callbacks after they are closed. RemoteInspector
+ does, so it just ignores possible callbacks from connections it no longer
+ cares about.
+
+ * inspector/remote/RemoteInspectorXPCConnection.h:
+ * inspector/remote/RemoteInspectorXPCConnection.mm:
+ (Inspector::RemoteInspectorXPCConnection::RemoteInspectorXPCConnection):
+ (Inspector::RemoteInspectorXPCConnection::~RemoteInspectorXPCConnection):
+ (Inspector::RemoteInspectorXPCConnection::close):
+ Keep the connection alive as long as the queue it can be used on
+ is alive. Clean up everything on the queue when close() is called.
+
+ (Inspector::RemoteInspectorXPCConnection::handleEvent):
+ Checking if closed here is not thread safe so it is meaningless.
+ Remove the check.
+
+ (Inspector::RemoteInspectorXPCConnection::sendMessage):
+ Bail based on the m_closed state.
+
+2014-01-27 Joseph Pecoraro <pecoraro@apple.com>
+
</ins><span class="cx"> JavaScriptCore: Enable -Wimplicit-fallthrough and add FALLTHROUGH annotation where needed
</span><span class="cx"> https://bugs.webkit.org/show_bug.cgi?id=127647
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreinspectorremoteRemoteInspectorh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/inspector/remote/RemoteInspector.h (162909 => 162910)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/inspector/remote/RemoteInspector.h 2014-01-28 04:04:56 UTC (rev 162909)
+++ trunk/Source/JavaScriptCore/inspector/remote/RemoteInspector.h 2014-01-28 04:55:55 UTC (rev 162910)
</span><span class="lines">@@ -94,7 +94,7 @@
</span><span class="cx">
</span><span class="cx"> HashMap<unsigned, std::pair<RemoteInspectorDebuggable*, RemoteInspectorDebuggableInfo>> m_debuggableMap;
</span><span class="cx"> HashMap<unsigned, RefPtr<RemoteInspectorDebuggableConnection>> m_connectionMap;
</span><del>- std::unique_ptr<RemoteInspectorXPCConnection> m_xpcConnection;
</del><ins>+ RefPtr<RemoteInspectorXPCConnection> m_xpcConnection;
</ins><span class="cx"> unsigned m_nextAvailableIdentifier;
</span><span class="cx"> int m_notifyToken;
</span><span class="cx"> bool m_enabled;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreinspectorremoteRemoteInspectormm"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/inspector/remote/RemoteInspector.mm (162909 => 162910)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/inspector/remote/RemoteInspector.mm 2014-01-28 04:04:56 UTC (rev 162909)
+++ trunk/Source/JavaScriptCore/inspector/remote/RemoteInspector.mm 2014-01-28 04:55:55 UTC (rev 162910)
</span><span class="lines">@@ -215,7 +215,7 @@
</span><span class="cx"> if (!connection)
</span><span class="cx"> return;
</span><span class="cx">
</span><del>- m_xpcConnection = std::make_unique<RemoteInspectorXPCConnection>(connection, this);
</del><ins>+ m_xpcConnection = adoptRef(new RemoteInspectorXPCConnection(connection, this));
</ins><span class="cx"> m_xpcConnection->sendMessage(@"syn", nil); // Send a simple message to initialize the XPC connection.
</span><span class="cx"> xpc_release(connection);
</span><span class="cx">
</span><span class="lines">@@ -249,9 +249,11 @@
</span><span class="cx"> NSLog(@"Unrecognized RemoteInspector XPC Message: %@", messageName);
</span><span class="cx"> }
</span><span class="cx">
</span><del>-void RemoteInspector::xpcConnectionFailed(RemoteInspectorXPCConnection*)
</del><ins>+void RemoteInspector::xpcConnectionFailed(RemoteInspectorXPCConnection* connection)
</ins><span class="cx"> {
</span><span class="cx"> MutexLocker locker(m_lock);
</span><ins>+ if (connection != m_xpcConnection)
+ return;
</ins><span class="cx">
</span><span class="cx"> m_pushScheduled = false;
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreinspectorremoteRemoteInspectorXPCConnectionh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.h (162909 => 162910)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.h 2014-01-28 04:04:56 UTC (rev 162909)
+++ trunk/Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.h 2014-01-28 04:55:55 UTC (rev 162910)
</span><span class="lines">@@ -29,7 +29,7 @@
</span><span class="cx"> #define RemoteInspectorXPCConnection_h
</span><span class="cx">
</span><span class="cx"> #import <dispatch/dispatch.h>
</span><del>-#import <wtf/Noncopyable.h>
</del><ins>+#import <wtf/ThreadSafeRefCounted.h>
</ins><span class="cx"> #import <xpc/xpc.h>
</span><span class="cx">
</span><span class="cx"> OBJC_CLASS NSDictionary;
</span><span class="lines">@@ -37,9 +37,7 @@
</span><span class="cx">
</span><span class="cx"> namespace Inspector {
</span><span class="cx">
</span><del>-class RemoteInspectorXPCConnection {
- WTF_MAKE_NONCOPYABLE(RemoteInspectorXPCConnection);
-
</del><ins>+class RemoteInspectorXPCConnection : public ThreadSafeRefCounted<RemoteInspectorXPCConnection> {
</ins><span class="cx"> public:
</span><span class="cx"> class Client {
</span><span class="cx"> public:
</span><span class="lines">@@ -62,6 +60,7 @@
</span><span class="cx"> xpc_connection_t m_connection;
</span><span class="cx"> dispatch_queue_t m_queue;
</span><span class="cx"> Client* m_client;
</span><ins>+ bool m_closed;
</ins><span class="cx"> };
</span><span class="cx">
</span><span class="cx"> } // namespace Inspector
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreinspectorremoteRemoteInspectorXPCConnectionmm"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.mm (162909 => 162910)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.mm 2014-01-28 04:04:56 UTC (rev 162909)
+++ trunk/Source/JavaScriptCore/inspector/remote/RemoteInspectorXPCConnection.mm 2014-01-28 04:55:55 UTC (rev 162910)
</span><span class="lines">@@ -49,13 +49,17 @@
</span><span class="cx"> : m_connection(connection)
</span><span class="cx"> , m_queue(dispatch_queue_create("com.apple.JavaScriptCore.remote-inspector-xpc-connection", DISPATCH_QUEUE_SERIAL))
</span><span class="cx"> , m_client(client)
</span><ins>+ , m_closed(false)
</ins><span class="cx"> {
</span><span class="cx"> xpc_retain(m_connection);
</span><span class="cx"> xpc_connection_set_target_queue(m_connection, m_queue);
</span><del>- RemoteInspectorXPCConnection* weakThis = this;
</del><span class="cx"> xpc_connection_set_event_handler(m_connection, ^(xpc_object_t object) {
</span><del>- weakThis->handleEvent(object);
</del><ins>+ handleEvent(object);
</ins><span class="cx"> });
</span><ins>+
+ // Balanced by deref in close.
+ ref();
+
</ins><span class="cx"> xpc_connection_resume(m_connection);
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -63,21 +67,29 @@
</span><span class="cx"> {
</span><span class="cx"> ASSERT(!m_client);
</span><span class="cx"> ASSERT(!m_connection);
</span><ins>+ ASSERT(m_closed);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void RemoteInspectorXPCConnection::close()
</span><span class="cx"> {
</span><del>- if (!m_connection)
</del><ins>+ if (m_closed)
</ins><span class="cx"> return;
</span><span class="cx">
</span><del>- xpc_connection_cancel(m_connection);
- xpc_release(m_connection);
- m_connection = NULL;
</del><ins>+ m_closed = true;
</ins><span class="cx">
</span><del>- dispatch_release(m_queue);
- m_queue = NULL;
</del><ins>+ dispatch_async(m_queue, ^{
+ xpc_connection_cancel(m_connection);
+ xpc_release(m_connection);
+ m_connection = NULL;
</ins><span class="cx">
</span><del>- m_client = nullptr;
</del><ins>+ dispatch_release(m_queue);
+ m_queue = NULL;
+
+ m_client = nullptr;
+
+ // Balance the ref in our constructor.
+ deref();
+ });
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> NSDictionary *RemoteInspectorXPCConnection::deserializeMessage(xpc_object_t object)
</span><span class="lines">@@ -99,9 +111,6 @@
</span><span class="cx">
</span><span class="cx"> void RemoteInspectorXPCConnection::handleEvent(xpc_object_t object)
</span><span class="cx"> {
</span><del>- if (!m_connection)
- return;
-
</del><span class="cx"> if (xpc_get_type(object) == XPC_TYPE_ERROR) {
</span><span class="cx"> if (m_client)
</span><span class="cx"> m_client->xpcConnectionFailed(this);
</span><span class="lines">@@ -120,7 +129,7 @@
</span><span class="cx">
</span><span class="cx"> void RemoteInspectorXPCConnection::sendMessage(NSString *messageName, NSDictionary *userInfo)
</span><span class="cx"> {
</span><del>- if (!m_connection)
</del><ins>+ if (m_closed)
</ins><span class="cx"> return;
</span><span class="cx">
</span><span class="cx"> NSMutableDictionary *dictionary = [NSMutableDictionary dictionaryWithObject:messageName forKey:RemoteInspectorXPCConnectionMessageNameKey];
</span></span></pre>
</div>
</div>
</body>
</html>