Bug 240143

Summary: [WebAuthn] Get rid of ASCAgentProxy instance after success/error/cancel
Product: WebKit Reporter: pascoe <pascoe>
Component: WebKit Misc.Assignee: pascoe <pascoe>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

pascoe@apple.com
Reported 2022-05-05 15:58:35 PDT
^
Attachments
Patch (2.98 KB, patch)
2022-05-05 16:05 PDT, pascoe@apple.com 		no flags
DetailsFormatted DiffDiff
Patch (3.99 KB, patch)
2022-05-05 16:50 PDT, pascoe@apple.com 		no flags
DetailsFormatted DiffDiff
Patch (3.99 KB, patch)
2022-05-05 17:08 PDT, pascoe@apple.com 		no flags
DetailsFormatted DiffDiff
Patch for landing (4.34 KB, patch)
2022-05-06 10:04 PDT, pascoe@apple.com 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2022-05-05 15:58:44 PDT
<rdar://problem/92825715>
pascoe@apple.com
Comment 2 2022-05-05 16:05:35 PDT
Created attachment 458921 [details] Patch
pascoe@apple.com
Comment 3 2022-05-05 16:50:32 PDT
Created attachment 458926 [details] Patch
pascoe@apple.com
Comment 4 2022-05-05 17:08:10 PDT
Created attachment 458927 [details] Patch
pascoe@apple.com
Comment 5 2022-05-05 17:08:24 PDT
rdar://91393607
Brent Fulgham
Comment 6 2022-05-06 09:42:16 PDT
Comment on attachment 458927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458927&action=review r=me > Source/WebKit/ChangeLog:12 > + * UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm: Nit: This file should probably be called WebAuthenticatorCoordinatorProxyCocoa.mm (but that's an issue for another day). > Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:450 > + weakThis->m_proxy.clear(); Do we need to clear m_proxy in in the case of 'ASCCredentialRequestTypeNone' on line 438? > Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:475 > + weakThis->m_proxy.clear(); What if we do have a weakThis, but we do not have a daemonEndPoint. Do we need to clear weakThis->m_proxy on line 465, above? Are they guaranteed to pass through the 'cancel' logic?
pascoe@apple.com
Comment 7 2022-05-06 09:49:18 PDT
Comment on attachment 458927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458927&action=review >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:450 >> + weakThis->m_proxy.clear(); > > Do we need to clear m_proxy in in the case of 'ASCCredentialRequestTypeNone' on line 438? We haven't set the m_proxy for the request yet here, so it shouldn't be necessary. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:475 >> + weakThis->m_proxy.clear(); > > What if we do have a weakThis, but we do not have a daemonEndPoint. Do we need to clear weakThis->m_proxy on line 465, above? Are they guaranteed to pass through the 'cancel' logic? If we don't get a daemonEndpoint it's likely something is wrong with the daemon. It does make sense to clear it here, will update.
pascoe@apple.com
Comment 8 2022-05-06 10:04:23 PDT
Created attachment 458960 [details] Patch for landing
EWS
Comment 9 2022-05-06 11:00:41 PDT
Committed r293907 (250362@main): <https://commits.webkit.org/250362@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 458960 [details].
Note You need to log in before you can comment on or make changes to this bug.