Bug 208256 – [WinCairo] Fix RemoteInspector reconnect issue

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Basuke Suzuki]

There are some cases we cannot reconnect to RemoteInspector Server once ungraceful disconnect happens before.

[Basuke Suzuki]

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.

[Basuke Suzuki]

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.

[Basuke Suzuki]

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

[Basuke Suzuki]

Created attachment 391780 [details]
PATCH

[Devin Rousso]

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 :)

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 391785 [details]
PATCH

[Devin Rousso]

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.

[Basuke Suzuki]

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.

[Devin Rousso]

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::`

[Basuke Suzuki]

Thanks!

[Basuke Suzuki]

Created attachment 391927 [details]
PATCH

[WebKit Commit Bot]

Comment on attachment 391927 [details]
PATCH

Clearing flags on attachment: 391927

Committed r257598: <https://trac.webkit.org/changeset/257598>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59867387>