Bug 221988 - REGRESSION(r266890): [Cocoa] Fix InspectorDelegate / API::InspectorClient leak
Summary: REGRESSION(r266890): [Cocoa] Fix InspectorDelegate / API::InspectorClient leak
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: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-16 12:52 PST by BJ Burg
Modified: 2021-03-29 13:20 PDT (History)
6 users (show)

See Also:


Attachments
Patch v1.0 (4.83 KB, patch)
2021-02-16 13:00 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v1.1 (4.82 KB, patch)
2021-02-16 13:25 PST, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2021-02-16 12:52:19 PST
<rdar://73594555>
Comment 1 BJ Burg 2021-02-16 13:00:07 PST
Created attachment 420528 [details]
Patch v1.0
Comment 2 Devin Rousso 2021-02-16 13:14:45 PST
Comment on attachment 420528 [details]
Patch v1.0

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

r=me, nice fix :)

> Source/WebKit/ChangeLog:27
> +        via the constructor. If a nil delegate is passedâfor example, when
> +        closing the WKWebViewâthen don't create an API::InspectorClient

i think there's non-ascii here :(

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:61
> +    if (!delegate && !_delegate)
> +        return;

What about if `!delegate` but `_delegate`?  Should we `_delegate = nullptr;` in that scenario?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:63
> +    _delegate = makeUnique<WebKit::InspectorDelegate>(self, delegate);

I could be crazy, but I think you can drop the `<WebKit::InspectorDelegate>` as `makeUnique` might be able to deduce the type.
Comment 3 BJ Burg 2021-02-16 13:23:21 PST
Comment on attachment 420528 [details]
Patch v1.0

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

>> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:61
>> +        return;
> 
> What about if `!delegate` but `_delegate`?  Should we `_delegate = nullptr;` in that scenario?

No. If !delegate, then we recreate InspectorDelegate with nil ObjC delegate, which causes us to call WebInspectorProxy::setClient(nullptr) in the constructor. Otherwise we'd need to disconnect this ourselves which defeats the point of the refactoring herein.
Comment 4 BJ Burg 2021-02-16 13:23:55 PST
Waiting for mac-wk2 tests to run prior to landing.
Comment 5 BJ Burg 2021-02-16 13:25:31 PST
Created attachment 420532 [details]
Patch v1.1
Comment 6 EWS 2021-02-16 15:43:42 PST
Committed r272935: <https://commits.webkit.org/r272935>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420532 [details].
Comment 7 BJ Burg 2021-03-29 13:20:09 PDT
This didn't fix the problem entirely. I'm creating a new bug.