Bug 227379 - [macOS] Add logging and clean up AppSSO flows
Summary: [macOS] Add logging and clean up AppSSO flows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-24 16:47 PDT by Brent Fulgham
Modified: 2021-06-28 10:45 PDT (History)
6 users (show)

See Also:


Attachments
Patch (38.99 KB, patch)
2021-06-25 14:01 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (41.29 KB, patch)
2021-06-25 15:24 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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