Bug 221988 – REGRESSION(r266890): [Cocoa] Fix InspectorDelegate / API::InspectorClient leak

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

<rdar://73594555>

[Blaze Burg]

Created attachment 420528 [details]
Patch v1.0

[Devin Rousso]

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.

[Blaze Burg]

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.

[Blaze Burg]

Waiting for mac-wk2 tests to run prior to landing.

[Blaze Burg]

Created attachment 420532 [details]
Patch v1.1

[EWS]

Committed r272935: <https://commits.webkit.org/r272935>

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

[Blaze Burg]

This didn't fix the problem entirely. I'm creating a new bug.