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

Description pascoe@apple.com 2022-05-05 15:58:35 PDT 
^
Comment 1 Radar WebKit Bug Importer 2022-05-05 15:58:44 PDT 
<rdar://problem/92825715>
Comment 2 pascoe@apple.com 2022-05-05 16:05:35 PDT 
Created attachment 458921 [details]
Patch
Comment 3 pascoe@apple.com 2022-05-05 16:50:32 PDT 
Created attachment 458926 [details]
Patch
Comment 4 pascoe@apple.com 2022-05-05 17:08:10 PDT 
Created attachment 458927 [details]
Patch
Comment 5 pascoe@apple.com 2022-05-05 17:08:24 PDT 
rdar://91393607
Comment 6 Brent Fulgham 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?
Comment 7 pascoe@apple.com 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.
Comment 8 pascoe@apple.com 2022-05-06 10:04:23 PDT 
Created attachment 458960 [details]
Patch for landing
Comment 9 EWS 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].