Bug 226539 - Web Inspector: [Cocoa] `RemoteInspector` won't connect to a new relay if it hasn't yet failed to communicate with a previously connected relay
Summary: Web Inspector: [Cocoa] `RemoteInspector` won't connect to a new relay if it h...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Patrick Angle
URL:
Keywords: InRadar
Depends on:
Blocks: 228303
  Show dependency treegraph
 
Reported: 2021-06-02 09:46 PDT by Patrick Angle
Modified: 2021-07-26 15:00 PDT (History)
11 users (show)

See Also:


Attachments
Patch v1.0 (5.91 KB, patch)
2021-06-02 12:34 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.1 - Review nits (5.97 KB, patch)
2021-06-03 11:03 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.1.1 - Add reviewer to changelog (5.97 KB, patch)
2021-06-03 11:35 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].