Hit this while running debug safaridriver. It may have been latent, but it's impossible to not hit now.
<rdar://problem/30643899>
Created attachment 302348 [details] Proposed Fix Still needs to be tested on Mac JSContext and iOS. Works fine in Safari. Has some protect(this) issues.
Comment on attachment 302348 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=302348&action=review > Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:468 > + std::lock_guard<Lock> lock(m_mutex); This is missing ref() and deref() like all other uses of dispatchAsyncOnTarget have. But I wasn't sure whether we need to do this for both RemoteInspector and the connection, or just the latter.
Attachment 302348 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:467: No space between ^ and block definition. [whitespace/brackets] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 302348 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=302348&action=review I haven't seen a backtrace but let me try to make sense of this. RemoteInspector::listingForAutomationTarget and RemoteInspector::listingForInspectionTarget both exist for getting listings. * One of them (listingForInspectionTarget) can seemingly be called from any thread. * The other (listingForAutomationTarget) should only be called from the Main Thread, presumably because one of those accessor calls is virtual and can do _anything_ in the UIProcess and so should be done when it is safe to do so. I don't actually see us calling some potentially wild virtual method, however I do see potential for issues if something is being modified at the same time that it is being fetched for a listing. I'm not sure this could be solved without a Lock per Target or a guaranteed access thread. I recall this worked differently a long time ago, but I don't remember how it changed differently. If updateTargetListing is asynchronous on a background thread, then it will be racing against pushListingsSoon on a different thread. I see no guarantee that the update happens before the push. So I don't think this patch as is can be correct. > Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:465 > + auto connectionToTarget = m_targetConnectionMap.get(targetIdentifier); > + if (!connectionToTarget) > return; This looks wrong. We should update the target listing regardless of whether or not there is a connection for it or not. It seems this would mean we never do any updates unless a debugger is connected. >> Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:468 >> + std::lock_guard<Lock> lock(m_mutex); > > This is missing ref() and deref() like all other uses of dispatchAsyncOnTarget have. But I wasn't sure whether we need to do this for both RemoteInspector and the connection, or just the latter. I don't think you need a ref/deref. RemoteInspector is a singleton that will always be alive. The connection is not used in the block so the connection could be destroyed and the block wouldn't have any problems when it executes.
Perhaps an alternative is to move the updateTargetListing + pushListingSoon combo to the end of RemoteConnectionToTarget::setup / RemoteConnectionToTarget::close which happens on the target queue. That should more closely match the other cases, but I don't really know if that is an improvement.
Created attachment 302366 [details] Patch v2
Comment on attachment 302366 [details] Patch v2 Okay this seems fine to me. I'll have to think about it a bit more tomorrow.
Comment on attachment 302366 [details] Patch v2 I haven't thought of any new concerns with this approach. So r=me
Committed r213228: <http://trac.webkit.org/changeset/213228>