Bug 223899 – v2: REGRESSION(r266890): [Cocoa] Fix API::InspectorClient leak

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

.

[Blaze Burg]

<rdar://problem/75249282>

[Blaze Burg]

Created attachment 424570 [details]
Patch v1.0

[Blaze Burg]

Created attachment 424598 [details]
Patch v2.0

[Blaze Burg]

I am attempting to repro the API test crash, so far no luck in a release build.

> Crashed
>    TestWebKitAPI.WKInspectorDelegate.ShowURLExternally

[Blaze Burg]

Created attachment 424806 [details]
Patch v2.1

[Devin Rousso]

Comment on attachment 424806 [details]
Patch v2.1

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

> Source/WebKit/UIProcess/API/APIInspectorClient.h:41
> -    virtual void openURLExternally(WebKit::WebInspectorUIProxy&, const WTF::String& url) { }
> +    virtual void openURLExternally(WebKit::WebInspectorUIProxy&, const WTF::String& url) { };

oops

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:64
> +    void openURLExternally(WebKit::WebInspectorUIProxy& inspector, const WTF::String& url)

NIT: I think we still add an `override` or `final` even if the overall `class` is marked as `final`.

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:208
> +    _inspector->~WebInspectorUIProxy();

Wait what?  This object isn't held elsewhere?  Does this mean that if Web Inspector is open and the app containing the `WKWebView` releases the `_WKInspector`, Web Inspector will close?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorInternal.h:-46
> -    std::unique_ptr<WebKit::InspectorDelegate> _delegate;

NIT: Please also remove the forward declare :)

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorTesting.mm:52
> +        _inspector->evaluateInFrontendForTesting(JavaScriptSnippetToOpenURLExternally([url copy]));

Why?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorTesting.mm:56
> +        [self.inspectorWebView loadRequest:[NSURLRequest requestWithURL:[url copy]]];

ditto (:+52)

[Blaze Burg]

(In reply to Devin Rousso from comment #6)
> Comment on attachment 424806 [details]
> Patch v2.1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424806&action=review
> 
> > Source/WebKit/UIProcess/API/APIInspectorClient.h:41
> > -    virtual void openURLExternally(WebKit::WebInspectorUIProxy&, const WTF::String& url) { }
> > +    virtual void openURLExternally(WebKit::WebInspectorUIProxy&, const WTF::String& url) { };
> 
> oops
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:64
> > +    void openURLExternally(WebKit::WebInspectorUIProxy& inspector, const WTF::String& url)
> 
> NIT: I think we still add an `override` or `final` even if the overall
> `class` is marked as `final`.

OK

> > Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:208
> > +    _inspector->~WebInspectorUIProxy();
> 
> Wait what?  This object isn't held elsewhere?  Does this mean that if Web
> Inspector is open and the app containing the `WKWebView` releases the
> `_WKInspector`, Web Inspector will close?

Hmm, good catch. I think this is only needed if WebInspectorUIProxy is allocated in the wrapper which it is not. It is owned by the inspected WebPageProxy (i.e, there is not -_apiObject or constructInWrapper() calls.) I'll remove it.


> > Source/WebKit/UIProcess/API/Cocoa/_WKInspectorInternal.h:-46
> > -    std::unique_ptr<WebKit::InspectorDelegate> _delegate;
> 
> NIT: Please also remove the forward declare :)
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKInspectorTesting.mm:52
> > +        _inspector->evaluateInFrontendForTesting(JavaScriptSnippetToOpenURLExternally([url copy]));
> 
> Why?
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKInspectorTesting.mm:56
> > +        [self.inspectorWebView loadRequest:[NSURLRequest requestWithURL:[url copy]]];
> 
> ditto (:+52)

`url` is not passed in with a +1 reference and its object lifetime is controlled by client code. Unless it's copied or autoreleased, it will be overreleased by the containing autoreleasepool. I prefer [url copy] as it is a more defensive approach.

[Blaze Burg]

Created attachment 424958 [details]
Patch v2.2 - address Devin feedback

[Devin Rousso]

Comment on attachment 424958 [details]
Patch v2.2 - address Devin feedback

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

r=me

> Source/WebKit/UIProcess/API/APIInspectorClient.h:41
> -    virtual void openURLExternally(WebKit::WebInspectorUIProxy&, const WTF::String& url) { }
> +    virtual void openURLExternally(WebKit::WebInspectorUIProxy&, const WTF::String& url) { };

oops

[Blaze Burg]

Committed r275393 (236057@main): <https://commits.webkit.org/236057@main>

[Joseph Pecoraro]

Comment on attachment 424958 [details]
Patch v2.2 - address Devin feedback

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

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:72
> +    WeakObjCPtr<id <_WKInspectorDelegate> > m_delegate;

`> >` => `>>` again I hope!

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:74
> +    bool m_respondsToInspectorOpenURLExternally : 1;

Probably not worth making this a bit right now instead of just a boolean. Unless you expect more of these in the future. The size shouldn't be any different, and maybe the generated code might be slower on some platforms.

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorInternal.h:42
> +    WeakObjCPtr<id <_WKInspectorDelegate> > _delegate;

I assume you can change `> >` to `>>` for all supported compilers now.

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorTesting.mm:52
> +        _inspector->evaluateInFrontendForTesting(JavaScriptSnippetToOpenURLExternally([url copy]));

Why do we need these copy calls? And if we do, how do these get released?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:224
> +    urlToOpen = adoptNS([NSURL URLWithString:@"https://www.webkit.org/"]);

You do not want to adopt an autoreleased NSURL, that would result in an overrelease. Just the regular assignment seems right, or adopt a `[[NSURL alloc] initWithString:...]`.

[Joseph Pecoraro]

> > > Source/WebKit/UIProcess/API/Cocoa/_WKInspectorTesting.mm:52
> > > +        _inspector->evaluateInFrontendForTesting(JavaScriptSnippetToOpenURLExternally([url copy]));
> > 
> > Why?
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/_WKInspectorTesting.mm:56
> > > +        [self.inspectorWebView loadRequest:[NSURLRequest requestWithURL:[url copy]]];
> > 
> > ditto (:+52)
> 
> `url` is not passed in with a +1 reference and its object lifetime is
> controlled by client code. Unless it's copied or autoreleased, it will be
> overreleased by the containing autoreleasepool. I prefer [url copy] as it is
> a more defensive approach.

Hmm, I don't follow this.

Given:

```
[NSURLRequest requestWithURL:url]
```

It would be the responsibility of NSURLRequest to copy the input if it needs to hold a reference to the url beyond the current run loop.

If you're getting an over-release somewhere, we should look around elsewhere. I'm afraid this copy would just be covering it up.

[Blaze Burg]

Reopening to attach new patch.

[Blaze Burg]

Created attachment 426835 [details]
Patch v2.2.1

[Blaze Burg]

Comment on attachment 426835 [details]
Patch v2.2.1

Submitting the leak fix separately for commit-queue integration.

[EWS]

Committed r276454 (236914@main): <https://commits.webkit.org/236914@main>

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