Bug 223899 - v2: REGRESSION(r266890): [Cocoa] Fix API::InspectorClient leak
Summary: v2: REGRESSION(r266890): [Cocoa] Fix 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: 225815
  Show dependency treegraph
 
Reported: 2021-03-29 13:20 PDT by BJ Burg
Modified: 2021-05-14 10:42 PDT (History)
6 users (show)

See Also:


Attachments
Patch v1.0 (2.77 KB, patch)
2021-03-29 13:25 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v2.0 (18.38 KB, patch)
2021-03-29 15:54 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v2.1 (20.12 KB, patch)
2021-03-31 12:53 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v2.2 - address Devin feedback (20.30 KB, patch)
2021-04-01 16:08 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v2.2.1 (3.33 KB, patch)
2021-04-22 12:04 PDT, 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-03-29 13:20:42 PDT
.
Comment 1 BJ Burg 2021-03-29 13:21:08 PDT
<rdar://problem/75249282>
Comment 2 BJ Burg 2021-03-29 13:25:35 PDT
Created attachment 424570 [details]
Patch v1.0
Comment 3 BJ Burg 2021-03-29 15:54:01 PDT
Created attachment 424598 [details]
Patch v2.0
Comment 4 BJ Burg 2021-03-30 10:31:06 PDT
I am attempting to repro the API test crash, so far no luck in a release build.

> Crashed
>    TestWebKitAPI.WKInspectorDelegate.ShowURLExternally
Comment 5 BJ Burg 2021-03-31 12:53:08 PDT
Created attachment 424806 [details]
Patch v2.1
Comment 6 Devin Rousso 2021-04-01 13:47:46 PDT
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)
Comment 7 BJ Burg 2021-04-01 14:52:51 PDT
(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.
Comment 8 BJ Burg 2021-04-01 16:08:11 PDT
Created attachment 424958 [details]
Patch v2.2 - address Devin feedback
Comment 9 Devin Rousso 2021-04-01 16:13:21 PDT
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
Comment 10 BJ Burg 2021-04-01 16:32:31 PDT
Committed r275393 (236057@main): <https://commits.webkit.org/236057@main>
Comment 11 Joseph Pecoraro 2021-04-08 10:57:31 PDT
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:...]`.
Comment 12 Joseph Pecoraro 2021-04-08 11:05:44 PDT
> > > 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.
Comment 13 BJ Burg 2021-04-22 12:04:30 PDT
Reopening to attach new patch.
Comment 14 BJ Burg 2021-04-22 12:04:31 PDT
Created attachment 426835 [details]
Patch v2.2.1
Comment 15 BJ Burg 2021-04-22 12:04:55 PDT
Comment on attachment 426835 [details]
Patch v2.2.1

Submitting the leak fix separately for commit-queue integration.
Comment 16 EWS 2021-04-22 12:48:25 PDT
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].