Bug 181946 - [WebAuthN] Revisit the whole async model of task dispatching, timeout and aborting
Summary: [WebAuthN] Revisit the whole async model of task dispatching, timeout and abo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 181943
  Show dependency treegraph
 
Reported: 2018-01-22 13:43 PST by Jiewen Tan
Modified: 2018-03-09 14:55 PST (History)
11 users (show)

See Also:


Attachments
Patch (94.49 KB, patch)
2018-02-05 19:01 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.43 MB, application/zip)
2018-02-05 19:43 PST, Build Bot
no flags Details
Patch (133.87 KB, patch)
2018-02-09 16:55 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (131.03 KB, patch)
2018-02-09 17:07 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (131.04 KB, patch)
2018-02-09 18:12 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (131.10 KB, patch)
2018-02-09 18:21 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (131.09 KB, patch)
2018-02-09 18:28 PST, Jiewen Tan
cdumez: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (140.81 KB, patch)
2018-02-13 14:21 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch for landing (137.94 KB, patch)
2018-02-13 14:38 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch for landing (140.90 KB, patch)
2018-02-13 15:14 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch for landing (138.04 KB, patch)
2018-02-13 15:29 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch for landing (139.16 KB, patch)
2018-02-14 18:51 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2018-01-22 13:43:02 PST
Revisit the whole async model of task dispatching, timeout and aborting once a first prototype is finished and all requirements are clear.
Comment 1 Jiewen Tan 2018-01-22 14:48:32 PST
For WebAuthN operations.
Comment 2 Radar WebKit Bug Importer 2018-02-05 18:01:20 PST
<rdar://problem/37258262>
Comment 3 Jiewen Tan 2018-02-05 19:01:08 PST
Created attachment 333145 [details]
Patch
Comment 4 Build Bot 2018-02-05 19:43:35 PST
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
Comment 5 Build Bot 2018-02-05 19:43:36 PST
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
Comment 6 Jiewen Tan 2018-02-06 12:53:27 PST
(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.
Comment 7 Chris Dumez 2018-02-06 13:35:16 PST
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
Comment 8 Jiewen Tan 2018-02-09 16:22:56 PST
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
Comment 9 Jiewen Tan 2018-02-09 16:55:33 PST
Created attachment 333534 [details]
Patch
Comment 10 Jiewen Tan 2018-02-09 17:07:34 PST
Created attachment 333538 [details]
Patch
Comment 11 Jiewen Tan 2018-02-09 18:12:31 PST
Created attachment 333544 [details]
Patch
Comment 12 Jiewen Tan 2018-02-09 18:21:25 PST
Created attachment 333546 [details]
Patch
Comment 13 Jiewen Tan 2018-02-09 18:28:34 PST
Created attachment 333547 [details]
Patch
Comment 14 Chris Dumez 2018-02-12 09:21:24 PST
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.
Comment 15 Jiewen Tan 2018-02-13 14:13:08 PST
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.
Comment 16 Jiewen Tan 2018-02-13 14:21:56 PST
Created attachment 333723 [details]
Patch for landing
Comment 17 Jiewen Tan 2018-02-13 14:38:32 PST
Created attachment 333726 [details]
Patch for landing
Comment 18 Jiewen Tan 2018-02-13 15:14:59 PST
Created attachment 333732 [details]
Patch for landing
Comment 19 Jiewen Tan 2018-02-13 15:29:15 PST
Created attachment 333735 [details]
Patch for landing
Comment 20 WebKit Commit Bot 2018-02-13 17:34:26 PST
Comment on attachment 333735 [details]
Patch for landing

Clearing flags on attachment: 333735

Committed r228444: <https://trac.webkit.org/changeset/228444>
Comment 21 Ryan Haddad 2018-02-14 09:21:20 PST
(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
Comment 22 Ryan Haddad 2018-02-14 09:26:06 PST
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
Comment 23 Ryan Haddad 2018-02-14 10:34:47 PST
Reverted r228444 for reason:

Introduced API and Layout test failures.

Committed r228470: <https://trac.webkit.org/changeset/228470>
Comment 24 Jiewen Tan 2018-02-14 18:48:11 PST
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.
Comment 25 Jiewen Tan 2018-02-14 18:51:29 PST
Created attachment 333867 [details]
Patch for landing
Comment 26 WebKit Commit Bot 2018-02-15 11:02:55 PST
Comment on attachment 333867 [details]
Patch for landing

Clearing flags on attachment: 333867

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