Revisit the whole async model of task dispatching, timeout and aborting once a first prototype is finished and all requirements are clear.
For WebAuthN operations.
<rdar://problem/37258262>
Created attachment 333145 [details] Patch
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
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
(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 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 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
Created attachment 333534 [details] Patch
Created attachment 333538 [details] Patch
Created attachment 333544 [details] Patch
Created attachment 333546 [details] Patch
Created attachment 333547 [details] Patch
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 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.
Created attachment 333723 [details] Patch for landing
Created attachment 333726 [details] Patch for landing
Created attachment 333732 [details] Patch for landing
Created attachment 333735 [details] Patch for landing
Comment on attachment 333735 [details] Patch for landing Clearing flags on attachment: 333735 Committed r228444: <https://trac.webkit.org/changeset/228444>
(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
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
Reverted r228444 for reason: Introduced API and Layout test failures. Committed r228470: <https://trac.webkit.org/changeset/228470>
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.
Created attachment 333867 [details] Patch for landing
Comment on attachment 333867 [details] Patch for landing Clearing flags on attachment: 333867 Committed r228523: <https://trac.webkit.org/changeset/228523>