[webkit-reviews] review denied: [Bug 181946] [WebAuthN] Revisit the whole async model of task dispatching, timeout and aborting : [Attachment 333145] Patch

Tue Feb 6 13:35:16 PST 2018

Chris Dumez <cdumez at apple.com> has denied Jiewen Tan <jiewen_tan at apple.com>'s
request for review:
Bug 181946: [WebAuthN] Revisit the whole async model of task dispatching,
timeout and aborting
https://bugs.webkit.org/show_bug.cgi?id=181946

Attachment 333145: Patch

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

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

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

> Source/WebCore/ChangeLog:12
> +	   threading model. To coperate that, a CredentialsMessenger class is
then created and

I don't think "coperate" is a word.

> Source/WebCore/ChangeLog:13
> +	   all task dispatching codes are moved thre.

typo: thre

also probably s/codes are/code is/

> Source/WebCore/ChangeLog:15
> +	   As an improvement over existing codes, static methods from
PublicKeyCredential are

s/codes/code
static functions

> Source/WebCore/ChangeLog:17
> +	   when static methods are called, they could reach the
CredentialsMessenger to interact

static functions

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:-140
> -    if (!options.publicKey) {

I liked it better is early return for error handling. This is a more usual
pattern.

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:88
> +    promise->reject(Exception { NotSupportedError });

It would be nice to provide a more detailed error message.

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:113
> +    if (options.publicKey) {

Liked it better with early return for error handling.

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:119
> +    promise->reject(Exception { NotSupportedError });

It would be nice to provide a more detailed error message.

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.h:43
> +    AssertionReturnBundle(Ref<ArrayBuffer>&& id, Ref<ArrayBuffer>&& data,
Ref<ArrayBuffer>&& sig, Ref<ArrayBuffer>&& handle)

What does sig mean? unless it is a well-established abbreviation in the spec, I
would avoid using abbreviations.

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.h:57
> +using CreationCompletionHandler =
Function<void(ExceptionOr<Vector<uint8_t>>&&)>;

Based on their name, you may want to use WTF::CompletionHandler instead of
WTF::Function.

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.h:58
> +using RequestCompletionHandler =
Function<void(ExceptionOr<AssertionReturnBundle>&&)>;

ditto.

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.h:90
> +    m_pendingCreationCompletionHandlers.add(m_accumulatedMessageId,
WTFMove(handler));

Would be nice to ASSERT that this is a new entry.

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.h:99
> +inline uint64_t
CredentialsMessenger::addRequestCompletionHandler(RequestCompletionHandler&&
handler)

Would be nice to ASSERT that this is a new entry.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:49
> +namespace AuthenticatorManagerInternal {

Why this namespace. I do not believe this is a common pattern in WebKit.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:82
> +static Vector<uint8_t> produceClientDataJsonHash(const Ref<ArrayBuffer>&
clientDataJson)

We may want to use const ArrayBuffer& as parameter type.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:102
> +static std::unique_ptr<Timer> initTimer(std::optional<unsigned long>
timeOutInMs, const Ref<DeferredPromise>& promise)

We prefer to use Seconds type. May be nice to use DeferredPromise& as parameter
too.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:104
> +    std::unique_ptr<Timer> timer = nullptr;

We prefer early return:
if (!timeOutInMs)
  return nullptr;
...

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:107
> +	       promise->reject(Exception { NotAllowedError });

It would be nice to provide a more detailed error message. Also, is
NotAllowedError the right exception type for a timeout?

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:114
> +static bool didTimerFire(const std::unique_ptr<Timer>& timer)

May be nicer to pass a Timer* in.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:116
> +    if (timer) {

if (!timer)
  return false;

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:128
> +    static NeverDestroyed<AuthenticatorManager> authenticator;

Can you please assert that: ASSERT(isMainThread()); ?

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:132
> +void AuthenticatorManager::setMessenger(WeakPtr<CredentialsMessenger>&&
messenger)

Can we pass in a CredentialsMessenger&, and have this method construct the
WeakPtr?

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:146
> +	   promise->reject(Exception { NotAllowedError });

It would be nice to provide a more detailed error message.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:157
> +    if (!options.rp.id.isEmpty() && !(callerOrigin.host() == options.rp.id))
{

Why isn't this using && callerOrigin.host() != options.rp.id ?

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:158
> +	   promise->reject(Exception { SecurityError });