Bug 227379

Summary: [macOS] Add logging and clean up AppSSO flows
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit2Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: ehutchison, ews-watchlist, jiewen_tan, katherine_cheney, kkinnunen, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Brent Fulgham 2021-06-24 16:47:19 PDT 
We continue to experience some hangs when using AppSSO to authenticate users. This patch adds copious logging to understand the flows through this code, and adds clean-up code in more places to ensure that the modal sheet used by AppSSO is dismissed in cases where the authentication has completed (or failed).
Comment 1 Brent Fulgham 2021-06-25 14:01:41 PDT 
Created attachment 432290 [details]
Patch
Comment 2 Kate Cheney 2021-06-25 14:46:05 PDT 
Comment on attachment 432290 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432290&action=review

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:153
> +    AUTHORIZATIONSESSION_RELEASE_LOG("shouldStart: m_page=%p", m_page.get());

If m_page is a weakPtr, could that cause issues trying to log if it is destroyed?

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:172
> +            WTFLogAlways("SOAuthorizationSession::start (getAuthorizationHintsWithURL completion handler): Returning early because weakThis is now null.");

Why WTFLogAlways instead of AUTHORIZATIONSESSION_RELEASE_LOG?
Comment 3 Brent Fulgham 2021-06-25 15:24:44 PDT 
Created attachment 432297 [details]
Patch
Comment 4 Kate Cheney 2021-06-25 15:36:51 PDT 
Comment on attachment 432297 [details]
Patch

r=me
Comment 5 EWS 2021-06-25 18:47:34 PDT 
Committed r279305 (239181@main): <https://commits.webkit.org/239181@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432297 [details].
Comment 6 Radar WebKit Bug Importer 2021-06-25 18:48:16 PDT 
<rdar://problem/79805243>
Comment 7 Eric Hutchison 2021-06-28 10:45:04 PDT 
It looks like the assert added in https://trac.webkit.org/changeset/279305/webkit

caused 4 API test crashes. Tracking in: https://bugs.webkit.org/show_bug.cgi?id=227454