[webkit-reviews] review denied: [Bug 191523] [WebAuthn] Implement AuthenticatorCancel : [Attachment 381169] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Jiewen Tan <jiewen_tan at apple.com>'s
request for review:
Bug 191523: [WebAuthn] Implement AuthenticatorCancel
https://bugs.webkit.org/show_bug.cgi?id=191523

Attachment 381169: Patch

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




--- Comment #3 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 381169
  --> https://bugs.webkit.org/attachment.cgi?id=381169
Patch

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

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:67
> +    if (!m_document || !m_document->frame() || !m_document->page()) {

Not needed. If you did not have a frame, you wouldn't have a page.

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:104
> +    if (!m_document || !m_document->frame() || !m_document->page()) {

Not needed. If you did not have a frame, you wouldn't have a page.

> Source/WebCore/Modules/webauthn/AuthenticatorCoordinator.cpp:143
> +    const auto& callerOrigin = document.securityOrigin();

we normally omit the "const" when we're using auto.

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:102
> +    memset(serialized.data() + serialized.size(), 0, kHidPacketSize -
serialized.size());

Looks to me like you're writing in memory that you don't own here? I get that
the vector capacity is kHidPacketSize, but it is not its size.
You should swap your too lines, first resize() and then memset.

Also, grow() here would be more efficient than resize() since serialized.size()
<= kHidPacketSize.

> Source/WebCore/Modules/webauthn/fido/FidoHidPacket.cpp:151
> +    memset(serialized.data() + serialized.size(), 0, kHidPacketSize -
serialized.size());

Same comment as above. ASAN would likely complain here.

> Source/WebKit/UIProcess/API/APIWebAuthenticationPanel.cpp:53
> +    if (!m_manager)

I think this can be:
if (m_manager)
    m_manager->cancelRequest(*this);

given how simple the method is.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:167
> +void AuthenticatorManager::cancelRequest(const WebPageProxy& page,
Optional<FrameIdentifier> subFrameId)

const Optional<FrameIdentifier>&

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:172
> +    if (subFrameId && (subFrameId != *(m_pendingRequestData.frameId)))

Too many parentheses.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:174
> +    invokePendingCompletionHandler(ExceptionData { NotAllowedError,
emptyString() });

Probably want a better error message than ""

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.h:68
> +    bool isInitialized() { return m_isInitialized; }

should be const.

> Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationRequestData.h:49
> +    WTF::Optional<WebCore::FrameIdentifier> frameId;

FrameIdentifier are not unique at UIProcess level, or even at WebPageProxy
level. So it looks very fragile to use this in UIProcess to identifier a
request. A { WebCore::PageIdentifier + WebCore::FrameIdentifier } is globally
unique though.

> Source/WebKit/UIProcess/WebAuthentication/fido/FidoAuthenticator.cpp:42
> +    if (!m_driver)

if (m_driver)
    m_driver->cancel();

> Source/WebKit/UIProcess/WebPageProxy.cpp:4119
> +    m_websiteDataStore->authenticatorManager().cancelRequest(*this,
frame->isMainFrame() ? WTF::nullopt : Optional<FrameIdentifier>(frameID));

could use makeOptional(frameID)

Seems odd to not be using a frameID for the main frame, since it has one.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:896
> +	   // WebPageProxy takes care of the main frame.

Why? Why can't we have a simple code path no matter if the frame is a main
frame or not?

> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:58
> +    auto messageId = setRequestCallback(WTFMove(handler));

Should this be using sendWithAsyncReply() ?

> Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:68
> +    auto messageId = setRequestCallback(WTFMove(handler));

Should this be using sendWithAsyncReply() ?