Bug 226539

Summary: Web Inspector: [Cocoa] `RemoteInspector` won't connect to a new relay if it hasn't yet failed to communicate with a previously connected relay
Product: WebKit Reporter: Patrick Angle <pangle>
Component: Web InspectorAssignee: Patrick Angle <pangle>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, pangle, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 228303    
Attachments:
Description Flags
Patch v1.0
none
Patch v1.1 - Review nits
none
Patch v1.1.1 - Add reviewer to changelog none

Description Patrick Angle 2021-06-02 09:46:20 PDT
<rdar://78237764>
Comment 1 Radar WebKit Bug Importer 2021-06-02 09:46:39 PDT
<rdar://problem/78768633>
Comment 2 Patrick Angle 2021-06-02 12:34:53 PDT
Created attachment 430392 [details]
Patch v1.0
Comment 3 Patrick Angle 2021-06-02 12:43:33 PDT
Correct radar is <rdar://78237764>
Comment 4 Ryan Haddad 2021-06-02 12:49:54 PDT
<rdar://78237764>
Comment 5 Devin Rousso 2021-06-02 16:34:14 PDT
Comment on attachment 430392 [details]
Patch v1.0

View in context: https://bugs.webkit.org/attachment.cgi?id=430392&action=review

r=me, nice fix with a great explanation :)

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:263
> +    bool m_allowRetryRelayConnection { false };

NIT: How about something a bit more specific/explanatory like `m_shouldReconnectToRelayOnFailure`?

> Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:281
> +        m_relayConnection->sendMessage(@"syn", nil);

NIT: I wonder if maybe we should give this a different name so it shows up differently in logging.  If we do keep the same name, perhaps we should create a static constant instead of hardcoding in both spots?

> Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:376
> +    if (m_allowRetryRelayConnection) {

Style: I'd make this into an early return

> Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:377
> +        WTFLogAlways("RemoteInspector XPC connection to relay failed, reattempting connection...");

NIT: s/reattempting connection/reconnecting/

> Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:380
> +        dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{

Should we maybe wait a second or two before attempting to reconnect?

> Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:384
> +        m_allowRetryRelayConnection = false;

NIT: I'd put this as the first thing in the `if` so that if someone adds an early return in the future it's less likely to be missed/forgotten
Comment 6 Patrick Angle 2021-06-03 11:03:23 PDT
Created attachment 430484 [details]
Patch v1.1 - Review nits
Comment 7 Patrick Angle 2021-06-03 11:35:41 PDT
Created attachment 430491 [details]
Patch v1.1.1 - Add reviewer to changelog
Comment 8 EWS 2021-06-03 12:32:29 PDT
Committed r278413 (238438@main): <https://commits.webkit.org/238438@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430491 [details].