Bug 238962 - [WebAuthn] Ensure requestPin callback on main thread
Summary: [WebAuthn] Ensure requestPin callback on main thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: pascoe@apple.com
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-07 15:00 PDT by pascoe@apple.com
Modified: 2022-04-11 09:20 PDT (History)
2 users (show)

See Also:


Attachments
Patch (2.38 KB, patch)
2022-04-07 15:07 PDT, pascoe@apple.com
no flags Details | Formatted Diff | Diff
Patch for landing (5.06 KB, patch)
2022-04-08 10:54 PDT, pascoe@apple.com
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description pascoe@apple.com 2022-04-07 15:00:47 PDT
When performing requests via ASA, this callback causes asserts to fail in debug builds.
Comment 1 Radar WebKit Bug Importer 2022-04-07 15:00:57 PDT
<rdar://problem/91446051>
Comment 2 pascoe@apple.com 2022-04-07 15:07:05 PDT
Created attachment 456974 [details]
Patch
Comment 3 Brent Fulgham 2022-04-08 10:38:49 PDT
Comment on attachment 456974 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:138
> +            completionHandler(pin.get());

I recommend adding a RELEASE_ASSERT(isMainThread()) to the logic of the completion handler to avoid future mistakes.
Comment 4 Brent Fulgham 2022-04-08 10:44:51 PDT
Comment on attachment 456974 [details]
Patch

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

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm:138
>> +            completionHandler(pin.get());
> 
> I recommend adding a RELEASE_ASSERT(isMainThread()) to the logic of the completion handler to avoid future mistakes.

Actually, I see you are using the more-correct RunLoop::isMain() in the caller, so no other action needed (except perhaps making it a RELEASE_ASSERT, which might be important to prevent UIKit code from ever running on a background thread.
Comment 5 pascoe@apple.com 2022-04-08 10:54:48 PDT
Created attachment 457094 [details]
Patch for landing
Comment 6 EWS 2022-04-11 09:20:07 PDT
Committed r292710 (249501@main): <https://commits.webkit.org/249501@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457094 [details].