WebAuthn Platform UI callbacks are not guaranteed to happen on the main thread

[Garrett Davidson]

WebAuthn Platform UI callbacks are not guaranteed to happen on the main thread

[Garrett Davidson]

Created attachment 441748 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/84420452>

[Brent Fulgham]

Comment on attachment 441748 [details]
Patch

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

> Source/WebKit/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=231963

Please add: <rdar://84420452>

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:268
> +        dispatch_async(dispatch_get_main_queue(), makeBlockPtr([weakThis, handler = WTFMove(handler), window = WTFMove(window), proxy = WTFMove(proxy), daemonEndpoint = retainPtr(daemonEndpoint), error = retainPtr(error)] () mutable {

This would be better as:

callOnMainThread([weakThis, handler = WTFMove(handler), window = WTFMove(window), proxy = WTFMove(proxy), daemonEndpoint = retainPtr(daemonEndpoint), error = retainPtr(error)] {
...
}

[Garrett Davidson]

Created attachment 441763 [details]
Patch

[Brent Fulgham]

Comment on attachment 441763 [details]
Patch

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

r=me, but please adjust the 'protectedThis' naming to reflect that this is a weak pointer, and does not hold the instance alive.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:270
> +            auto protectedThis = weakThis.get();

protectedThis is a bad name for a WeakPtr -- can we just use 'weakThis'? I don't think we need to pull a bare pointer out of the WeakPtr wrapper (it should cast properly). 'protectedThis' implies we have a RetainPtr that is holding the object alive -- which is misleading in this case.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:271
> +            if (!protectedThis || !daemonEndpoint) {

So this would just be "if (!weakThis || !daemonEndpoint) {"

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:277
> +            protectedThis->m_presenter = adoptNS([allocASCAuthorizationRemotePresenterInstance() init]);

Then these could just be "weakThis->m_presenter", etc.

[Garrett Davidson]

Created attachment 441770 [details]
Patch

[Brent Fulgham]

Comment on attachment 441770 [details]
Patch

Looks great! Thanks.

[EWS]

Committed r284489 (243243@main): <https://commits.webkit.org/243243@main>

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