There are some cases we cannot reconnect to RemoteInspector Server once ungraceful disconnect happens before.
How to reproduce: 1. Open target's MiniBrowser and display something. (e.g. https://playstation.com/) 2. Open inspector's MiniBrowser and connect to target. A chooser window is opened. 3. Click [inspect]. A inspector window is opened. 4. Close chooser windows (2). Expected result: Can connect to the target again. Actual result: Cannot connect to the target until the target is restarted.
When socket is closed while RemoteInspector itself is still connecting status, the following dead lock happens: - RemoteInspector::didClose() is called. - It locks m_mutex. - It calls stopInternal() and it invokes target close() - RemoteInspector is informed to updateTarget - RemoteInspector::updateTarget tries to lock m_mutex again and stall.
Following Cocoa port implementation, it is better to target close on async timing, maybe using RunLoop. https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm#L202-L221
Created attachment 391780 [details] PATCH
Comment on attachment 391780 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=391780&action=review > Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.cpp:90 > + RunLoop::current().dispatch([this, protectThis = makeRef(*this), targetIdentifier = m_target->targetIdentifier()] { I believe that it's possible that `m_target` is `nullptr`. > Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.cpp:98 > + RemoteInspector::singleton().updateTargetListing(targetIdentifier); I'm not sure that we want to wait to do this. On macOS, this is used to control whether targets are able to be inspected/driven, so we ideally would want that info to be as "live" as possible (it also involves XPC messaging, so ideally the sooner the better). Perhaps we can split this into platform variants, so that only the platforms that can deadlock do it via the `RunLoop`? I'm not sure this is that big of a deal though, but I'd ask @JoePeck and @brrian too :)
Comment on attachment 391780 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=391780&action=review >> Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.cpp:90 >> + RunLoop::current().dispatch([this, protectThis = makeRef(*this), targetIdentifier = m_target->targetIdentifier()] { > > I believe that it's possible that `m_target` is `nullptr`. That's right. Original code checked and I missed that. >> Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.cpp:98 >> + RemoteInspector::singleton().updateTargetListing(targetIdentifier); > > I'm not sure that we want to wait to do this. On macOS, this is used to control whether targets are able to be inspected/driven, so we ideally would want that info to be as "live" as possible (it also involves XPC messaging, so ideally the sooner the better). > > Perhaps we can split this into platform variants, so that only the platforms that can deadlock do it via the `RunLoop`? > > I'm not sure this is that big of a deal though, but I'd ask @JoePeck and @brrian too :) But it seems macOS also call this async. https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm#L216 I agree with the importance of keeping the status as live as possible, but I think that should be treated in separate bug. I also see locking m_targetMutex isn't required while updateTargetListing in many functions.
Created attachment 391785 [details] PATCH
Comment on attachment 391780 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=391780&action=review >>> Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.cpp:98 >>> + RemoteInspector::singleton().updateTargetListing(targetIdentifier); >> >> I'm not sure that we want to wait to do this. On macOS, this is used to control whether targets are able to be inspected/driven, so we ideally would want that info to be as "live" as possible (it also involves XPC messaging, so ideally the sooner the better). >> >> Perhaps we can split this into platform variants, so that only the platforms that can deadlock do it via the `RunLoop`? >> >> I'm not sure this is that big of a deal though, but I'd ask @JoePeck and @brrian too :) > > But it seems macOS also call this async. https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm#L216 > > I agree with the importance of keeping the status as live as possible, but I think that should be treated in separate bug. > > I also see locking m_targetMutex isn't required while updateTargetListing in many functions. I totally forgot that this had a Cocoa specific variant 🤦♂️ My apologies. Where do you see `RemoteInspector::singleton().updateTargetListing` being called without `m_targetMutex` (or at least some form of locking)? I think it should have locking.
Comment on attachment 391780 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=391780&action=review >>>> Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.cpp:98 >>>> + RemoteInspector::singleton().updateTargetListing(targetIdentifier); >>> >>> I'm not sure that we want to wait to do this. On macOS, this is used to control whether targets are able to be inspected/driven, so we ideally would want that info to be as "live" as possible (it also involves XPC messaging, so ideally the sooner the better). >>> >>> Perhaps we can split this into platform variants, so that only the platforms that can deadlock do it via the `RunLoop`? >>> >>> I'm not sure this is that big of a deal though, but I'd ask @JoePeck and @brrian too :) >> >> But it seems macOS also call this async. https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm#L216 >> >> I agree with the importance of keeping the status as live as possible, but I think that should be treated in separate bug. >> >> I also see locking m_targetMutex isn't required while updateTargetListing in many functions. > > I totally forgot that this had a Cocoa specific variant 🤦♂️ My apologies. > > Where do you see `RemoteInspector::singleton().updateTargetListing` being called without `m_targetMutex` (or at least some form of locking)? I think it should have locking. I mean in the context of m_targetMutex. It shouldn't be protected by RemoteConnectionToTarget::m_targetMutex because it can be called from other places not related to RemoteConnectionTarget. It should has its own mutext and protected inside the method, maybe.
Comment on attachment 391785 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=391785&action=review r=me > Source/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.cpp:92 > + if (m_target) { Style: this could be an early return instead, so everything is indented one level less > Source/WTF/wtf/RunLoop.h:235 > + WTF::Function<void()> m_wakeUpCallback; I think you can drop the `WTF::`
Thanks!
Created attachment 391927 [details] PATCH
Comment on attachment 391927 [details] PATCH Clearing flags on attachment: 391927 Committed r257598: <https://trac.webkit.org/changeset/257598>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59867387>