[webkit-reviews] review denied: [Bug 232635] [WebAuthn] Implement add/remove_virtual_authenticator for transport=internal : [Attachment 443230] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied j_pascoe at apple.com
<j_pascoe at apple.com>'s request for review:
Bug 232635: [WebAuthn] Implement add/remove_virtual_authenticator for
transport=internal
https://bugs.webkit.org/show_bug.cgi?id=232635

Attachment 443230: Patch

https://bugs.webkit.org/attachment.cgi?id=443230&action=review




--- Comment #7 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 443230
  --> https://bugs.webkit.org/attachment.cgi?id=443230
Patch

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

I think this patch looks really good, but a couple of improvements could be
made. r- to correct the pass-by-value things (or help me understand why I'm
wrong), and to confirm we don't have any racy behavior with objects going out
of scope.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1560
> +    WebPageProxy* page = webPageProxyForHandle(browsingContextHandle);

Could be 'auto'

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1563
> +    VirtualAuthenticatorManager& authenticatorManager =
page->websiteDataStore().virtualAuthenticatorManager();

Couldn't this be "const auto&"?

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1605
> +    VirtualAuthenticatorManager& authenticatorManager =
page->websiteDataStore().virtualAuthenticatorManager();

Ditto the auto and "const auto&" comments above.

>
Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualAuthenticatorManager.c
pp:41
> +String
VirtualAuthenticatorManager::createAuthenticator(VirtualAuthenticatorConfigurat
ion config)

Seems like the config argument should be a const & or a Move operation.

> Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualLocalConnection.h:41
> +    void verifyUser(const String&, WebCore::ClientDataType,
SecAccessControlRef, WebCore::UserVerificationRequirement, 
UserVerificationCallback&&) final;

Extra space before UserVerificationCallback&&

>
Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualLocalConnection.mm:55
> +    RunLoop::main().dispatch([configuration = m_configuration, callback =
WTFMove(callback)]() mutable {

Do we copy the configuration here because we think the main object may go out
of scope? If so, we should hold a weakPtr to 'this' and bail out early if it is
nullptr.

If we don't think it's possible for 'this' to be deleted before this code is
called, maybe pass a reference or just access the object through this?

> Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualService.h:39
> +    static UniqueRef<AuthenticatorTransportService>
createVirtual(WebCore::AuthenticatorTransport, Observer&, const
Vector<VirtualAuthenticatorConfiguration> configs);

Do you really want to pass a vector of VirtualAuthenticatorConfiguration by
copy? Seems like you should move them, or pass a const ref.

> Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualService.mm:54
> +	      
observer()->authenticatorAdded(LocalAuthenticator::create(makeUniqueRef<Virtual
LocalConnection>(config)));

Is it possible for 'authenticatorAdded' to call anything that could cause
observer() to return nullptr?