Summary: | REGRESSION(r266890): [Cocoa] Fix InspectorDelegate / 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 | ||||||||
Attachments: |
|
Description
BJ Burg
2021-02-16 12:52:19 PST
Created attachment 420528 [details]
Patch v1.0
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 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. Waiting for mac-wk2 tests to run prior to landing. Created attachment 420532 [details]
Patch v1.1
Committed r272935: <https://commits.webkit.org/r272935> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420532 [details]. This didn't fix the problem entirely. I'm creating a new bug. |