Summary: | v2: REGRESSION(r266890): [Cocoa] Fix API::InspectorClient leak | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | BJ Burg <bburg> | ||||||||||||
Component: | Web Inspector | Assignee: | BJ Burg <bburg> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bburg, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 225815 | ||||||||||||||
Attachments: |
|
Description
BJ Burg
2021-03-29 13:20:42 PDT
Created attachment 424570 [details]
Patch v1.0
Created attachment 424598 [details]
Patch v2.0
I am attempting to repro the API test crash, so far no luck in a release build.
> Crashed
> TestWebKitAPI.WKInspectorDelegate.ShowURLExternally
Created attachment 424806 [details]
Patch v2.1
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) (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. Created attachment 424958 [details]
Patch v2.2 - address Devin feedback
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 Committed r275393 (236057@main): <https://commits.webkit.org/236057@main> 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:...]`. > > > 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.
Reopening to attach new patch. Created attachment 426835 [details]
Patch v2.2.1
Comment on attachment 426835 [details]
Patch v2.2.1
Submitting the leak fix separately for commit-queue integration.
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]. |