Bug 181946 – [WebAuthN] Revisit the whole async model of task dispatching, timeout and aborting

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jiewen Tan]

Revisit the whole async model of task dispatching, timeout and aborting once a first prototype is finished and all requirements are clear.

[Jiewen Tan]

For WebAuthN operations.

[Radar WebKit Bug Importer]

<rdar://problem/37258262>

[Jiewen Tan]

Created attachment 333145 [details]
Patch

[EWS Watchlist]

Comment on attachment 333145 [details]
Patch

Attachment 333145 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6375291

New failing tests:
media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html

[EWS Watchlist]

Created attachment 333151 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Jiewen Tan]

(In reply to Build Bot from comment #4)
> Comment on attachment 333145 [details]
> Patch
> 
> Attachment 333145 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/6375291
> 
> New failing tests:
> media/modern-media-controls/tracks-support/tracks-support-show-panel-after-
> dragging-controls.html

This test failure should not blame my patch.

[Chris Dumez]

Comment on attachment 333145 [details]
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 });

Would be nice to provide a more detailed error message.

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

Would be nice to provide a more detailed error message.

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

Would be nice to provide a more detailed error message. Should this be an InvalidStateError?

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

Would be nice to provide a more detailed error message.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:197
> +        auto attestationObject = result.returnValue();

This copies attestationObject unnecessarily. Either:
- Get rid of this local variable
or
- use auto&

You can also releaseReturnValue() if it is beneficial to transfer ownership.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:200
> +    // Async operation are dispatched and handled in the messenger.

operations

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

Would be nice to provide a more detailed error message.

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

Why not != ?

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

Should this be an InvalidStateError?

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

Would be nice to provide a more detailed error message.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:252
> +            promise->reject(result.exception());

Would be nice to provide a more detailed error message.

> Source/WebCore/testing/Internals.cpp:536
> +    m_mockCredentialsMessenger = new MockCredentialsMessenger();

Isn't this leaking? Should we use a unique_ptr?

> Source/WebCore/testing/MockCredentialsMessenger.cpp:50
> +    auto messageId = addCreationCompletionHandler(WTFMove(handler));

Isn't the handler left in the map when m_timeOut is true or when m_attestationObject.isEmpty()?

> Source/WebCore/testing/MockCredentialsMessenger.cpp:57
> +        didMakeCredential(messageId, Exception { NotAllowedError });

Please provide a better exception message.

> Source/WebCore/testing/MockCredentialsMessenger.cpp:69
> +    auto messageId = addRequestCompletionHandler(WTFMove(handler));

Isn't the handler left in the map when m_timeOut is true or when !m_bundle?

> Source/WebCore/testing/MockCredentialsMessenger.h:40
> +    void setTimeOut() { m_timeOut = true; }

Bad naming

> Source/WebCore/testing/MockCredentialsMessenger.h:41
> +    void setUserCancel() { m_userCancel = true; }

Bad naming

> Source/WebCore/testing/MockCredentialsMessenger.h:45
> +    void ref() const { }

This looks really weird.

> Source/WebCore/testing/MockCredentialsMessenger.h:46
> +    void deref() const { }

ditto.

> Source/WebCore/testing/MockCredentialsMessenger.h:55
> +    bool m_timeOut { false };

m_timeOut is very confusingly named and not as per coding style. It needs a prefix like "m_didTimeOut".

> Source/WebCore/testing/MockCredentialsMessenger.h:56
> +    bool m_userCancel { false };

Bad naming as well.

> Source/WebCore/testing/MockCredentialsMessenger.idl:30
> +    void setTimeOut();

bad naming

> Source/WebCore/testing/MockCredentialsMessenger.idl:31
> +    void setUserCancel();

bad naming

[Jiewen Tan]

Comment on attachment 333145 [details]
Patch

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

Thanks Chris for reviewing this patch. I will upload one with a dummy IPC channel hooked up in WebProcess and UIProcess.

>> Source/WebCore/ChangeLog:12
>> +        threading model. To coperate that, a CredentialsMessenger class is then created and
> 
> I don't think "coperate" is a word.

Fixed.

>> Source/WebCore/ChangeLog:13
>> +        all task dispatching codes are moved thre.
> 
> typo: thre
> 
> also probably s/codes are/code is/

Fixed.

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

Fixed.

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

Fixed.

>> 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.

Fixed.

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:88
>> +    promise->reject(Exception { NotSupportedError });
> 
> It would be nice to provide a more detailed error message.

Sure, I filed Bug 182657 to cooperate that later.

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:113
>> +    if (options.publicKey) {
> 
> Liked it better with early return for error handling.

Fixed.

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:119
>> +    promise->reject(Exception { NotSupportedError });
> 
> It would be nice to provide a more detailed error message.

Bug 182657.

>> 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.

Fixed.

>> 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.

Thanks for the hints. Were not aware of this type.

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

Fixed.

>> 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.

Fixed.

>> 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.

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:49
>> +namespace AuthenticatorManagerInternal {
> 
> Why this namespace. I do not believe this is a common pattern in WebKit.

I believe this is something new introduced by the UnifiedSource to avoid naming collision when unify multiple .cpp into a single one.

>> 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.

Fixed.

>> 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.

timeOutInMs is from an IDL type. It will then be converted to Seconds below. I have to copy the promise below, hence DeferredPromise& might not be sufficient.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:104
>> +    std::unique_ptr<Timer> timer = nullptr;
> 
> We prefer early return:
> if (!timeOutInMs)
>   return nullptr;
> ...

Fixed.

>> 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?

Bug 182657. From the spec, it should be NotAllowedError if any 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.

Fixed.

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

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:128
>> +    static NeverDestroyed<AuthenticatorManager> authenticator;
> 
> Can you please assert that: ASSERT(isMainThread()); ?

Fixed.

>> 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?

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:146
>> +        promise->reject(Exception { NotAllowedError });
> 
> It would be nice to provide a more detailed error message.

Bug 182657.

>> 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 ?

Oops. Fixed. I should not have an EE background...

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:158
>> +        promise->reject(Exception { SecurityError });
> 
> Would be nice to provide a more detailed error message.

Bug 182657.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:168
>> +        promise->reject(Exception { NotSupportedError });
> 
> Would be nice to provide a more detailed error message.

Bug 182657.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:181
>> +        promise->reject(Exception { UnknownError });
> 
> Would be nice to provide a more detailed error message. Should this be an InvalidStateError?

Bug 182657. I am not sure since this is not part of the spec.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:189
>> +            promise->reject(Exception { AbortError });
> 
> Would be nice to provide a more detailed error message.

Bug 182657.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:197
>> +        auto attestationObject = result.returnValue();
> 
> This copies attestationObject unnecessarily. Either:
> - Get rid of this local variable
> or
> - use auto&
> 
> You can also releaseReturnValue() if it is beneficial to transfer ownership.

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:200
>> +    // Async operation are dispatched and handled in the messenger.
> 
> operations

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:213
>> +        promise->reject(Exception { NotAllowedError });
> 
> Would be nice to provide a more detailed error message.

Bug 182657.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:224
>> +    if (!options.rpId.isEmpty() && !(callerOrigin.host() == options.rpId)) {
> 
> Why not != ?

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:240
>> +        promise->reject(Exception { UnknownError });
> 
> Should this be an InvalidStateError?

Ditto.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:248
>> +            promise->reject(Exception { AbortError });
> 
> Would be nice to provide a more detailed error message.

Bug 182657.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:252
>> +            promise->reject(result.exception());
> 
> Would be nice to provide a more detailed error message.

Bug 182657.

>> Source/WebCore/testing/Internals.cpp:536
>> +    m_mockCredentialsMessenger = new MockCredentialsMessenger();
> 
> Isn't this leaking? Should we use a unique_ptr?

We probably should but I saw the MockPaymentCoordinator doesn't care about it.

>> Source/WebCore/testing/MockCredentialsMessenger.cpp:50
>> +    auto messageId = addCreationCompletionHandler(WTFMove(handler));
> 
> Isn't the handler left in the map when m_timeOut is true or when m_attestationObject.isEmpty()?

That's fair. However, I don't see a way to fix it in this mocking environment. It should not happen in a real environment.

>> Source/WebCore/testing/MockCredentialsMessenger.cpp:57
>> +        didMakeCredential(messageId, Exception { NotAllowedError });
> 
> Please provide a better exception message.

Bug 182657.

>> Source/WebCore/testing/MockCredentialsMessenger.cpp:69
>> +    auto messageId = addRequestCompletionHandler(WTFMove(handler));
> 
> Isn't the handler left in the map when m_timeOut is true or when !m_bundle?

Ditto.

>> Source/WebCore/testing/MockCredentialsMessenger.h:40
>> +    void setTimeOut() { m_timeOut = true; }
> 
> Bad naming

Fixed.

>> Source/WebCore/testing/MockCredentialsMessenger.h:41
>> +    void setUserCancel() { m_userCancel = true; }
> 
> Bad naming

Fixed.

>> Source/WebCore/testing/MockCredentialsMessenger.h:45
>> +    void ref() const { }
> 
> This looks really weird.

In order to hock up with IDL, it requires some ref/deref things. Maybe there is an attribute to suggest not require ref/deref?

>> Source/WebCore/testing/MockCredentialsMessenger.h:46
>> +    void deref() const { }
> 
> ditto.

Ditto.

>> Source/WebCore/testing/MockCredentialsMessenger.h:55
>> +    bool m_timeOut { false };
> 
> m_timeOut is very confusingly named and not as per coding style. It needs a prefix like "m_didTimeOut".

Fixed

>> Source/WebCore/testing/MockCredentialsMessenger.h:56
>> +    bool m_userCancel { false };
> 
> Bad naming as well.

Fixed

>> Source/WebCore/testing/MockCredentialsMessenger.idl:30
>> +    void setTimeOut();
> 
> bad naming

Fixed

>> Source/WebCore/testing/MockCredentialsMessenger.idl:31
>> +    void setUserCancel();
> 
> bad naming

Fixed

[Jiewen Tan]

Created attachment 333534 [details]
Patch

[Jiewen Tan]

Created attachment 333538 [details]
Patch

[Jiewen Tan]

Created attachment 333544 [details]
Patch

[Jiewen Tan]

Created attachment 333546 [details]
Patch

[Jiewen Tan]

Created attachment 333547 [details]
Patch

[Chris Dumez]

Comment on attachment 333547 [details]
Patch

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

r=me with comments.

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:35
> +static constexpr uint64_t MaxMessageId = 0xFFFFFFFFFFFFFF; // 56 bits

const maxMessageId = 0xFFFFFFFFFFFFFF; // 56 bits

No need for static since this is a global const variable. Also, I do not believe we usually start global variable names with a capital letter.

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:36
> +static constexpr size_t CallBackClassifierOffset = 56;

ditto.

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:41
> +static constexpr size_t CredentialIdLengthOffset = 43;

ditto

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:42
> +static constexpr size_t CredentialIdLengthLength = 2;

ditto.

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:51
> +        auto handler = m_pendingCreationCompletionHandlers.take(messageId);

takeCreationCompletionHandler()?

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:56
> +        auto handler = m_pendingRequestCompletionHandlers.take(messageId);

takeCreationCompletionHandler()?

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.h:72
> +class CredentialsMessenger {

WTF_MAKE_FAST_ALLOCATED;

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

initTimeoutTimer()?

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

Please provide an exception message (second parameter to Exception constructor.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:94
> +    timer->startOneShot(Seconds(timeOutInMs.value() / 1000.0));

Seconds::fromMilliseconds(*timeOutInMs);

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:98
> +static bool didTimerFire(Timer* timer)

didTimeoutTimerFire()?

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

Please provide an exception message (second parameter to Exception constructor.

> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:136
> +    std::unique_ptr<Timer> timer = initTimer(options.timeout, promise);

timeoutTimer?

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

Please provide an exception message (second parameter to Exception constructor.

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

Please provide an exception message (second parameter to Exception constructor.

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

Please provide an exception message (second parameter to Exception constructor.

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

Please provide an exception message (second parameter to Exception constructor.

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

Please provide an exception message (second parameter to Exception constructor.

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

Please provide an exception message (second parameter to Exception constructor.

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

Please provide an exception message (second parameter to Exception constructor.

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

Please provide an exception message (second parameter to Exception constructor.

> Source/WebCore/testing/MockCredentialsMessenger.cpp:60
> +        exceptionReply(messageId, ExceptionData { NotAllowedError, emptyString()});

Please provide a useful exception message instead of the empty string.

> Source/WebCore/testing/MockCredentialsMessenger.cpp:79
> +        exceptionReply(messageId, ExceptionData { NotAllowedError, emptyString()});

Please provide a useful exception message instead of the empty string.

> Source/WebCore/testing/MockCredentialsMessenger.h:40
> +    void timeOut() { m_didTimeOut = true; }

Confusing naming. I would suggest: markAsTimedOut(), setDidTimeOut().

> Source/WebCore/testing/MockCredentialsMessenger.h:41
> +    void userCancel() { m_didUserCancel = true; }

Confusing naming. I would suggest markAsCancelledByUser() or setDidUserCancel().

> Source/WebCore/testing/MockCredentialsMessenger.h:45
> +    void ref() const { }

We usually handle this by ref'ing / derefing the owner, so:
1. Pass a Internals& to the constructor
2. Store it in a Internals m_internals; data member
3. Have ref() / deref() forward the calls to m_internals (i.e. m_internals.ref() / m_internals.deref())

Note that you need to keep MockCredentialsMessenger as a unique_ptr inside Internals.

[Jiewen Tan]

Comment on attachment 333547 [details]
Patch

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

Thanks Chris for r+ the patch.

>> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:35
>> +static constexpr uint64_t MaxMessageId = 0xFFFFFFFFFFFFFF; // 56 bits
> 
> const maxMessageId = 0xFFFFFFFFFFFFFF; // 56 bits
> 
> No need for static since this is a global const variable. Also, I do not believe we usually start global variable names with a capital letter.

Fixed.

>> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:36
>> +static constexpr size_t CallBackClassifierOffset = 56;
> 
> ditto.

Fixed.

>> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:41
>> +static constexpr size_t CredentialIdLengthOffset = 43;
> 
> ditto

Fixed.

>> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:42
>> +static constexpr size_t CredentialIdLengthLength = 2;
> 
> ditto.

Fixed.

>> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:51
>> +        auto handler = m_pendingCreationCompletionHandlers.take(messageId);
> 
> takeCreationCompletionHandler()?

Fixed.

>> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.cpp:56
>> +        auto handler = m_pendingRequestCompletionHandlers.take(messageId);
> 
> takeCreationCompletionHandler()?

Fixed.

>> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.h:72
>> +class CredentialsMessenger {
> 
> WTF_MAKE_FAST_ALLOCATED;

May I ask what's the usage of this?

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

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:92
>> +        promise->reject(Exception { NotAllowedError });
> 
> Please provide an exception message (second parameter to Exception constructor.

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:94
>> +    timer->startOneShot(Seconds(timeOutInMs.value() / 1000.0));
> 
> Seconds::fromMilliseconds(*timeOutInMs);

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:98
>> +static bool didTimerFire(Timer* timer)
> 
> didTimeoutTimerFire()?

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:131
>> +        promise->reject(Exception { NotAllowedError });
> 
> Please provide an exception message (second parameter to Exception constructor.

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:136
>> +    std::unique_ptr<Timer> timer = initTimer(options.timeout, promise);
> 
> timeoutTimer?

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:143
>> +        promise->reject(Exception { SecurityError });
> 
> Please provide an exception message (second parameter to Exception constructor.

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:153
>> +        promise->reject(Exception { NotSupportedError });
> 
> Please provide an exception message (second parameter to Exception constructor.

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:166
>> +        promise->reject(Exception { UnknownError });
> 
> Please provide an exception message (second parameter to Exception constructor.

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:174
>> +            promise->reject(Exception { AbortError });
> 
> Please provide an exception message (second parameter to Exception constructor.

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:198
>> +        promise->reject(Exception { NotAllowedError });
> 
> Please provide an exception message (second parameter to Exception constructor.

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:210
>> +        promise->reject(Exception { SecurityError });
> 
> Please provide an exception message (second parameter to Exception constructor.

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:225
>> +        promise->reject(Exception { UnknownError });
> 
> Please provide an exception message (second parameter to Exception constructor.

Fixed.

>> Source/WebCore/Modules/webauthn/AuthenticatorManager.cpp:233
>> +            promise->reject(Exception { AbortError });
> 
> Please provide an exception message (second parameter to Exception constructor.

Fixed.

>> Source/WebCore/testing/MockCredentialsMessenger.cpp:60
>> +        exceptionReply(messageId, ExceptionData { NotAllowedError, emptyString()});
> 
> Please provide a useful exception message instead of the empty string.

Fixed.

>> Source/WebCore/testing/MockCredentialsMessenger.cpp:79
>> +        exceptionReply(messageId, ExceptionData { NotAllowedError, emptyString()});
> 
> Please provide a useful exception message instead of the empty string.

Fixed.

>> Source/WebCore/testing/MockCredentialsMessenger.h:40
>> +    void timeOut() { m_didTimeOut = true; }
> 
> Confusing naming. I would suggest: markAsTimedOut(), setDidTimeOut().

Fixed.

>> Source/WebCore/testing/MockCredentialsMessenger.h:41
>> +    void userCancel() { m_didUserCancel = true; }
> 
> Confusing naming. I would suggest markAsCancelledByUser() or setDidUserCancel().

Fixed.

>> Source/WebCore/testing/MockCredentialsMessenger.h:45
>> +    void ref() const { }
> 
> We usually handle this by ref'ing / derefing the owner, so:
> 1. Pass a Internals& to the constructor
> 2. Store it in a Internals m_internals; data member
> 3. Have ref() / deref() forward the calls to m_internals (i.e. m_internals.ref() / m_internals.deref())
> 
> Note that you need to keep MockCredentialsMessenger as a unique_ptr inside Internals.

Fixed.

[Jiewen Tan]

Created attachment 333723 [details]
Patch for landing

[Jiewen Tan]

Created attachment 333726 [details]
Patch for landing

[Jiewen Tan]

Created attachment 333732 [details]
Patch for landing

[Jiewen Tan]

Created attachment 333735 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 333735 [details]
Patch for landing

Clearing flags on attachment: 333735

Committed r228444: <https://trac.webkit.org/changeset/228444>

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #20)
> Comment on attachment 333735 [details]
> Patch for landing
> 
> Clearing flags on attachment: 333735
> 
> Committed r228444: <https://trac.webkit.org/changeset/228444>
This change introduced 10 API test failures on macOS and 5 on iOS.

macOS:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK2%20%28Tests%29/builds/2052/steps/run-api-tests/logs/stdio

ASSERTION FAILED: !m_messageReceiverMapCount
/Volumes/Data/slave/highsierra-debug/build/Source/WebKit/Platform/IPC/MessageReceiver.h(40) : virtual IPC::MessageReceiver::~MessageReceiver()
1   0x10eae462d WTFCrash
2   0x111cb01e4 IPC::MessageReceiver::~MessageReceiver()
3   0x1124fb35c WebKit::WebCredentialsMessengerProxy::~WebCredentialsMessengerProxy()
4   0x1124fb385 WebKit::WebCredentialsMessengerProxy::~WebCredentialsMessengerProxy()
5   0x1124fb3a9 WebKit::WebCredentialsMessengerProxy::~WebCredentialsMessengerProxy()
6   0x11270d19c WebKit::WebPageProxy::reattachToWebProcess()
7   0x1127112ad WebKit::WebPageProxy::loadRequest(WebCore::ResourceRequest&&, WebCore::ShouldOpenExternalURLsPolicy, API::Object*)
8   0x112b69a1b WKPageLoadURL
9   0x10cb06c69 TestWebKitAPI::WebKit_ResizeWindowAfterCrash_Test::TestBody()
10  0x10ccdb48a testing::Test::Run()
11  0x10ccdc2bd testing::internal::TestInfoImpl::Run()
12  0x10ccdd30d testing::TestCase::Run()
13  0x10cce3d7b testing::internal::UnitTestImpl::RunAllTests()
14  0x10cce39f9 testing::UnitTest::Run()
15  0x10cb6044c TestWebKitAPI::TestsController::run(int, char**)
16  0x10ccaa69f main
17  0x7fff5a285115 start
18  0x2

iOS:
https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/builds/2699/steps/run-api-tests/logs/stdio

UNEXPECTEDLY EXITED WebKit.InteractionDeadlockAfterCrash
ASSERTION FAILED: !m_messageReceiverMapCount
/Volumes/Data/slave/ios-simulator-11-debug/build/Source/WebKit/Platform/IPC/MessageReceiver.h(40) : virtual IPC::MessageReceiver::~MessageReceiver()
1   0x10bf3f0cd WTFCrash
2   0x10e4293a4 IPC::MessageReceiver::~MessageReceiver()
3   0x10eb1872c WebKit::WebCredentialsMessengerProxy::~WebCredentialsMessengerProxy()
4   0x10eb18755 WebKit::WebCredentialsMessengerProxy::~WebCredentialsMessengerProxy()
5   0x10eb18779 WebKit::WebCredentialsMessengerProxy::~WebCredentialsMessengerProxy()
6   0x10ed2bb50 WebKit::WebPageProxy::reattachToWebProcess()
7   0x10ed30ccf WebKit::WebPageProxy::loadData(API::Data*, WTF::String const&, WTF::String const&, WTF::String const&, API::Object*)
8   0x10f20a2b7 -[WKWebView loadData:MIMEType:characterEncodingName:baseURL:]
9   0x10f20a1a1 -[WKWebView loadHTMLString:baseURL:]
10  0x10a51ae52 WebKit_InteractionDeadlockAfterCrash_Test::TestBody()
11  0x10a6f39da testing::Test::Run()
12  0x10a6f449d testing::internal::TestInfoImpl::Run()
13  0x10a6f54ed testing::TestCase::Run()
14  0x10a6fb56b testing::internal::UnitTestImpl::RunAllTests()
15  0x10a6fb1e9 testing::UnitTest::Run()
16  0x10a5caaec TestWebKitAPI::TestsController::run(int, char**)
17  0x10a6c8cc0 main
18  0x124ab8d81 start

[Ryan Haddad]

Also, an assertion failure on macOS Debug WK2 seen with http/wpt/webauthn tests

ASSERTION FAILED: Completion handler should always be called
!m_function
/Volumes/Data/slave/highsierra-debug/build/WebKitBuild/Debug/usr/local/include/wtf/CompletionHandler.h(51) : WTF::CompletionHandler<void (WebCore::ExceptionOr<WebCore::CreationReturnBundle> &&)>::~CompletionHandler()

https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r228453%20(2055)/results.html
https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r228454%20(5245)/results.html

[Ryan Haddad]

Reverted r228444 for reason:

Introduced API and Layout test failures.

Committed r228470: <https://trac.webkit.org/changeset/228470>

[Jiewen Tan]

Fixed all API and Layout test failures.

For API tests, the failure is due to the lack of nullifying m_credentialsMessenger in  WebPageProxy::resetState. When WebPageProxy::reattachToWebProcess is called, a newly created CredentialsMessenger will overwrite the value which is the m_credentialsMessenger in MessageReceiverMap as they share the same name before m_credentialsMessenger is destructed. Later on when m_credentialsMessenger is destructed, it will then remove the newly created CredentialsMessenger from the MessageReceiverMap and decrement its m_messageReceiverMapCount instead of itself. Thus, we hit the assertion.

For the Layout tests, as Chris pointed out early that completionHandlers are not called when mocking timeouts. Then we hit the assertion when those are being destructed. To resolve that, I added a way in MockCredentialsMessenger to remember all those timeout handlers and call them when ~MockCredentialsMessenger is invoked.

[Jiewen Tan]

Created attachment 333867 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 333867 [details]
Patch for landing

Clearing flags on attachment: 333867

Committed r228523: <https://trac.webkit.org/changeset/228523>