Bug 188623 – [WebAuthN] Support CTAP HID authenticators on macOS

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

Support USB authenticators on macOS.

[Radar WebKit Bug Importer]

<rdar://problem/43353777>

[Jiewen Tan]

This task might import CTAPHID implementation from Chromium including:
1. FidoHidDevice for protocol structure, channels and arbitration.
2. FidoHidMessage for message packet structure.

[Jiewen Tan]

Created attachment 354538 [details]
Patch

[Jiewen Tan]

Created attachment 354576 [details]
Patch

[EWS Watchlist]

Comment on attachment 354576 [details]
Patch

Attachment 354576 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9962366

New failing tests:
http/wpt/webauthn/ctap-hid-failure.https.html
http/wpt/webauthn/ctap-hid-success.https.html
http/wpt/webauthn/public-key-credential-create-success-hid.https.html
http/wpt/webauthn/idl.https.html
http/wpt/webauthn/public-key-credential-create-failure-local-silent.https.html
http/wpt/webauthn/public-key-credential-get-failure-hid.https.html
http/wpt/webauthn/public-key-credential-get-failure-local-silent.https.html
http/wpt/webauthn/public-key-credential-create-failure-hid.https.html
http/wpt/webauthn/public-key-credential-create-failure.https.html
http/wpt/webauthn/public-key-credential-create-failure-hid-silent.https.html
http/wpt/webauthn/public-key-credential-create-failure-local.https.html
http/wpt/webauthn/public-key-credential-get-success-local.https.html
http/wpt/webauthn/public-key-credential-get-failure.https.html
http/wpt/webauthn/public-key-credential-get-failure-hid-silent.https.html
http/wpt/webauthn/public-key-credential-get-success-hid.https.html
http/wpt/webauthn/public-key-credential-create-success-local.https.html

[EWS Watchlist]

Created attachment 354596 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Jiewen Tan]

Created attachment 354601 [details]
Patch

[Jiewen Tan]

Created attachment 354607 [details]
Patch

[Brent Fulgham]

Comment on attachment 354607 [details]
Patch

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

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:598
> +    if (hidRef) {

I think this would be better as:

if (auto hidRef = .... stuff...) {

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:613
> +

Should probably just be one blank line here.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:629
> +        if (payloadBase64)

if (auto payloadBase64 = ... stuff ... ) {

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:633
> +        if (keepAlive)

Ditto one-line format

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:637
> +        if (fastDataArrival)

Ditto

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:641
> +        if (continueAfterErrorData)

Ditto

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:58
> +        ASSERT_UNUSED(addResult, addResult.isNewEntry);

Is there any reason this is macOS only? Seems like gtk or Windows might be able to use this code if they had relevant HID code.

Maybe we should have a "#if USE(HID)" or something to represent this, rather than OS-specific checks?

This would be fine to do as a separate patch.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:65
>      void clearState();

Should clearState ever be called directly? Maybe it should be private.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.mm:75
> +    m_initialized = true;

Seems like we could have a PAL layer for HID handling, then this code could be shared across ports.

Note: Not for the current patch though -- just an idea for a future cleanup.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.mm:89
> +    auto task = BlockPtr<void()>::fromCallable([device = m_device, data = WTFMove(data), callback = WTFMove(callback)]() mutable {

Do we want device to retain m_device? Or should it be a weakptr?

> LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-hid-silent.https-expected.txt:5
> +PASS PublicKeyCredential's [[create]] with mixed options in a mock hid authenticator. 

Nice. Web Platform Tests for this!

[Jiewen Tan]

Comment on attachment 354607 [details]
Patch

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

Thanks Brent for r+ this patch.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:598
>> +    if (hidRef) {
> 
> I think this would be better as:
> 
> if (auto hidRef = .... stuff...) {

Fixed.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:613
>> +
> 
> Should probably just be one blank line here.

Fixed.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:629
>> +        if (payloadBase64)
> 
> if (auto payloadBase64 = ... stuff ... ) {

Fixed.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:633
>> +        if (keepAlive)
> 
> Ditto one-line format

Fixed.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:637
>> +        if (fastDataArrival)
> 
> Ditto

Fixed.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:641
>> +        if (continueAfterErrorData)
> 
> Ditto

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:58
>> +        ASSERT_UNUSED(addResult, addResult.isNewEntry);
> 
> Is there any reason this is macOS only? Seems like gtk or Windows might be able to use this code if they had relevant HID code.
> 
> Maybe we should have a "#if USE(HID)" or something to represent this, rather than OS-specific checks?
> 
> This would be fine to do as a separate patch.

Sure, let's do this on demand. Just don't want things become even more complicated for the first patch.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:65
>>      void clearState();
> 
> Should clearState ever be called directly? Maybe it should be private.

Good question. Now, it is only called by the timeout timer. Since the timer fires in a later runloop cycle which has already broken the cyclic dependency, it doesn't need to call the async version. However, I don't see why it can't call the async version. Maybe it is good to unify them.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.mm:75
>> +    m_initialized = true;
> 
> Seems like we could have a PAL layer for HID handling, then this code could be shared across ports.
> 
> Note: Not for the current patch though -- just an idea for a future cleanup.

Yup, if GTK+ or WPE wants WebAuthN.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.mm:89
>> +    auto task = BlockPtr<void()>::fromCallable([device = m_device, data = WTFMove(data), callback = WTFMove(callback)]() mutable {
> 
> Do we want device to retain m_device? Or should it be a weakptr?

Not sure if there is a weak version for RetainPtr. We could use that if there is one.

>> LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-hid-silent.https-expected.txt:5
>> +PASS PublicKeyCredential's [[create]] with mixed options in a mock hid authenticator. 
> 
> Nice. Web Platform Tests for this!

Yup.

[Chris Dumez]

Comment on attachment 354607 [details]
Patch

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

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:196
>          m_requestTimeOutTimer->stop();

I think I missed this during an earlier review but since this code runs in the UIProcess, you are not allowed to use WebCore::Timer (It would break apps using both WK1 and WK2). You need to use RunLoop::Timer in the UIProcess.

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:229
> +    m_requestTimeOutTimer = std::make_unique<Timer>([weakThis = makeWeakPtr(*this)]() mutable {

Not sure why we're making this change, I think we should just capture |this|. |this| owns the Timer.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.mm:41
> +    HidConnection* connection = static_cast<HidConnection*>(context);

auto*

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.mm:54
> +    reportData.reserveInitialCapacity(reportLength);

I do not think this is helpful in this case since you're doing a single append with the whole length. Vector::append(data, length) is already optimized.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm:46
> +    HidService* listener = static_cast<HidService*>(context);

auto*

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm:112
> +        observer()->authenticatorAdded(CtapHidAuthenticator::create(m_drivers.take(ptr)));

This is unnecessarily doing a second lookup of ptr in m_drivers (see call to contains above). It should be a single lookup.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:141
> +            const auto it = requestMap->getMap().find(CBORValue(CtapMakeCredentialRequestOptionsKey)); // Find options.

no need for the "const", we prefer to omit it whenever possible.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:143
> +                const auto& optionMap = it->second.getMap();

ditto.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:156
> +            const auto it = requestMap->getMap().find(CBORValue(CtapGetAssertionRequestOptionsKey)); // Find options.

ditto.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:158
> +                const auto& optionMap = it->second.getMap();

ditto.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:182
> +        payload.grow(kHidInitResponseSize);

I don't understand why you need reserveInitialCapacity(kHidInitResponseSize) and / or grow(kHidInitResponseSize) here? appendVector() already knows how many entries will be appended. AFAICT, there is a single append call.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:194
> +        auto infoData = encodeAsCBOR(AuthenticatorGetInfoResponse({ ProtocolVersion::kCtap }, Vector<uint8_t>(kAaguidLength)));

Isn't this vector (Vector<uint8_t>(kAaguidLength)) containing uninitialized data?

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:195
> +        infoData.insert(0, static_cast<uint8_t>(CtapDeviceResponseCode::kSuccess)); // Prepend status code.

Maybe this is initializing it?

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.h:46
> +    explicit MockHidConnection(IOHIDDeviceRef, const MockWebAuthenticationConfiguration&);

There are 2 parameters, not sure this needs to be explicit.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.h:65
> +    uint32_t m_currentChannel;

uninitialized?

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.h:66
> +    bool m_requireResidentKey;

uninitialized?

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.h:67
> +    bool m_requireUserVerification;

uninitialized?

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockWebAuthenticationConfiguration.h:44
> +        enum class Stage {

You may want to consider using a smaller underlying type, e.g.
enum class Stage : bool { }

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockWebAuthenticationConfiguration.h:49
> +        enum class SubStage {

ditto

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockWebAuthenticationConfiguration.h:54
> +        enum class Error {

ditto but maybe a uint8_t.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockWebAuthenticationConfiguration.h:63
> +        Stage stage;

This struct is currently a lot larger than it needs to be. Please consider switching these enums to tighter underlying types and moving them after the String data member for better packing.

Also please provide default initialization values for ALL data members.

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp:64
> +        receiveRespond(ExceptionData { UnknownError, makeString("Unknown internal error. Error code: ", String::number(data.size() == 1 ? data[0] : -1)) });

Please do not use String::number() with makeString(), just include StringConcatenateNumbers.h.

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.h:50
> +    enum class State {

Please consider using a tighter underlying type, e.g. uint8_t.

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.h:72
> +        enum class State {

Please consider using a tighter underlying type, e.g. uint8_t.

[Jiewen Tan]

Comment on attachment 354607 [details]
Patch

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

Thanks Chris for reviewing the patch.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:196
>>          m_requestTimeOutTimer->stop();
> 
> I think I missed this during an earlier review but since this code runs in the UIProcess, you are not allowed to use WebCore::Timer (It would break apps using both WK1 and WK2). You need to use RunLoop::Timer in the UIProcess.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:229
>> +    m_requestTimeOutTimer = std::make_unique<Timer>([weakThis = makeWeakPtr(*this)]() mutable {
> 
> Not sure why we're making this change, I think we should just capture |this|. |this| owns the Timer.

|This| is not an issue anymore.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.mm:41
>> +    HidConnection* connection = static_cast<HidConnection*>(context);
> 
> auto*

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.mm:54
>> +    reportData.reserveInitialCapacity(reportLength);
> 
> I do not think this is helpful in this case since you're doing a single append with the whole length. Vector::append(data, length) is already optimized.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm:46
>> +    HidService* listener = static_cast<HidService*>(context);
> 
> auto*

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm:112
>> +        observer()->authenticatorAdded(CtapHidAuthenticator::create(m_drivers.take(ptr)));
> 
> This is unnecessarily doing a second lookup of ptr in m_drivers (see call to contains above). It should be a single lookup.

I rewrote it as:
    std::unique_ptr<CtapHidDriver> driver = m_drivers.take(ptr);
    if (!driver || !observer())
        return;

    auto info = readCTAPGetInfoResponse(response);
    if (info && info->versions().find(ProtocolVersion::kCtap) != info->versions().end()) {
        observer()->authenticatorAdded(CtapHidAuthenticator::create(WTFMove(driver), WTFMove(*info)));
        return;
    }
    // FIXME(191535): Support U2F authenticators.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:141
>> +            const auto it = requestMap->getMap().find(CBORValue(CtapMakeCredentialRequestOptionsKey)); // Find options.
> 
> no need for the "const", we prefer to omit it whenever possible.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:143
>> +                const auto& optionMap = it->second.getMap();
> 
> ditto.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:156
>> +            const auto it = requestMap->getMap().find(CBORValue(CtapGetAssertionRequestOptionsKey)); // Find options.
> 
> ditto.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:158
>> +                const auto& optionMap = it->second.getMap();
> 
> ditto.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:182
>> +        payload.grow(kHidInitResponseSize);
> 
> I don't understand why you need reserveInitialCapacity(kHidInitResponseSize) and / or grow(kHidInitResponseSize) here? appendVector() already knows how many entries will be appended. AFAICT, there is a single append call.

Because the vector appended is not the reserve size. It makes more sense if I rearranged the above to:
        Vector<uint8_t> payload;
        payload.reserveInitialCapacity(kHidInitResponseSize);
        // FIXME(191533): Use a real nonce.
        payload.appendVector(Vector<uint8_t>({ 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07 }));
        payload.grow(kHidInitResponseSize);
        cryptographicallyRandomValues(payload.data() + payload.size(), CtapChannelIdSize);

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:194
>> +        auto infoData = encodeAsCBOR(AuthenticatorGetInfoResponse({ ProtocolVersion::kCtap }, Vector<uint8_t>(kAaguidLength)));
> 
> Isn't this vector (Vector<uint8_t>(kAaguidLength)) containing uninitialized data?

I guess it is initialized to all 0s?

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:195
>> +        infoData.insert(0, static_cast<uint8_t>(CtapDeviceResponseCode::kSuccess)); // Prepend status code.
> 
> Maybe this is initializing it?

No, this is not.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.h:46
>> +    explicit MockHidConnection(IOHIDDeviceRef, const MockWebAuthenticationConfiguration&);
> 
> There are 2 parameters, not sure this needs to be explicit.

It doesn't.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.h:65
>> +    uint32_t m_currentChannel;
> 
> uninitialized?

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.h:66
>> +    bool m_requireResidentKey;
> 
> uninitialized?

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.h:67
>> +    bool m_requireUserVerification;
> 
> uninitialized?

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockWebAuthenticationConfiguration.h:44
>> +        enum class Stage {
> 
> You may want to consider using a smaller underlying type, e.g.
> enum class Stage : bool { }

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockWebAuthenticationConfiguration.h:49
>> +        enum class SubStage {
> 
> ditto

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockWebAuthenticationConfiguration.h:54
>> +        enum class Error {
> 
> ditto but maybe a uint8_t.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockWebAuthenticationConfiguration.h:63
>> +        Stage stage;
> 
> This struct is currently a lot larger than it needs to be. Please consider switching these enums to tighter underlying types and moving them after the String data member for better packing.
> 
> Also please provide default initialization values for ALL data members.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp:64
>> +        receiveRespond(ExceptionData { UnknownError, makeString("Unknown internal error. Error code: ", String::number(data.size() == 1 ? data[0] : -1)) });
> 
> Please do not use String::number() with makeString(), just include StringConcatenateNumbers.h.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.h:50
>> +    enum class State {
> 
> Please consider using a tighter underlying type, e.g. uint8_t.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidDriver.h:72
>> +        enum class State {
> 
> Please consider using a tighter underlying type, e.g. uint8_t.

Fixed.

[Jiewen Tan]

Created attachment 354750 [details]
Patch for Landing

[WebKit Commit Bot]

Comment on attachment 354750 [details]
Patch for Landing

Clearing flags on attachment: 354750

Committed r238166: <https://trac.webkit.org/changeset/238166>

[Jiewen Tan]

Comment on attachment 354607 [details]
Patch

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

>>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:194
>>> +        auto infoData = encodeAsCBOR(AuthenticatorGetInfoResponse({ ProtocolVersion::kCtap }, Vector<uint8_t>(kAaguidLength)));
>> 
>> Isn't this vector (Vector<uint8_t>(kAaguidLength)) containing uninitialized data?
> 
> I guess it is initialized to all 0s?

Alright, our Vector doesn't like std's behavior. Will fix it in Bug 191533.

[Truitt Savell]

Looks like the new test http/wpt/webauthn/public-key-credential-get-success-hid.https.html added in https://trac.webkit.org/changeset/238166/webkit is a flakey failure. 

Looks like this is having a harness timeout. 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-success-hid.https.html

Diff:
--- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-get-success-hid.https-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-get-success-hid.https-actual.txt
@@ -1,7 +1,9 @@
+
+Harness Error (TIMEOUT), message = null
 
 PASS PublicKeyCredential's [[get]] with minimum options in a mock hid authenticator. 
 PASS PublicKeyCredential's [[get]] with matched allow credentials in a mock hid authenticator. 
-PASS PublicKeyCredential's [[get]] with userVerification { preferred } in a mock hid authenticator. 
-PASS PublicKeyCredential's [[get]] with userVerification { discouraged } in a mock hid authenticator. 
-PASS PublicKeyCredential's [[get]] with mixed options in a mock hid authenticator. 
+TIMEOUT PublicKeyCredential's [[get]] with userVerification { preferred } in a mock hid authenticator. Test timed out
+NOTRUN PublicKeyCredential's [[get]] with userVerification { discouraged } in a mock hid authenticator. 
+NOTRUN PublicKeyCredential's [[get]] with mixed options in a mock hid authenticator.

[Jiewen Tan]

(In reply to Truitt Savell from comment #16)
> Looks like the new test
> http/wpt/webauthn/public-key-credential-get-success-hid.https.html added in
> https://trac.webkit.org/changeset/238166/webkit is a flakey failure. 
> 
> Looks like this is having a harness timeout. 
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-
> get-success-hid.https.html
> 
> Diff:
> ---
> /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/http/
> wpt/webauthn/public-key-credential-get-success-hid.https-expected.txt
> +++
> /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/http/
> wpt/webauthn/public-key-credential-get-success-hid.https-actual.txt
> @@ -1,7 +1,9 @@
> +
> +Harness Error (TIMEOUT), message = null
>  
>  PASS PublicKeyCredential's [[get]] with minimum options in a mock hid
> authenticator. 
>  PASS PublicKeyCredential's [[get]] with matched allow credentials in a mock
> hid authenticator. 
> -PASS PublicKeyCredential's [[get]] with userVerification { preferred } in a
> mock hid authenticator. 
> -PASS PublicKeyCredential's [[get]] with userVerification { discouraged } in
> a mock hid authenticator. 
> -PASS PublicKeyCredential's [[get]] with mixed options in a mock hid
> authenticator. 
> +TIMEOUT PublicKeyCredential's [[get]] with userVerification { preferred }
> in a mock hid authenticator. Test timed out
> +NOTRUN PublicKeyCredential's [[get]] with userVerification { discouraged }
> in a mock hid authenticator. 
> +NOTRUN PublicKeyCredential's [[get]] with mixed options in a mock hid
> authenticator.

It is not flaky in my iMac Pro. I suspect we should make it long. I will do the gardening. Thanks for the head up.

[Jiewen Tan]

(In reply to Jiewen Tan from comment #17)
> (In reply to Truitt Savell from comment #16)
> > Looks like the new test
> > http/wpt/webauthn/public-key-credential-get-success-hid.https.html added in
> > https://trac.webkit.org/changeset/238166/webkit is a flakey failure. 
> > 
> > Looks like this is having a harness timeout. 
> > 
> > History:
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-
> > get-success-hid.https.html
> > 
> > Diff:
> > ---
> > /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/http/
> > wpt/webauthn/public-key-credential-get-success-hid.https-expected.txt
> > +++
> > /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/http/
> > wpt/webauthn/public-key-credential-get-success-hid.https-actual.txt
> > @@ -1,7 +1,9 @@
> > +
> > +Harness Error (TIMEOUT), message = null
> >  
> >  PASS PublicKeyCredential's [[get]] with minimum options in a mock hid
> > authenticator. 
> >  PASS PublicKeyCredential's [[get]] with matched allow credentials in a mock
> > hid authenticator. 
> > -PASS PublicKeyCredential's [[get]] with userVerification { preferred } in a
> > mock hid authenticator. 
> > -PASS PublicKeyCredential's [[get]] with userVerification { discouraged } in
> > a mock hid authenticator. 
> > -PASS PublicKeyCredential's [[get]] with mixed options in a mock hid
> > authenticator. 
> > +TIMEOUT PublicKeyCredential's [[get]] with userVerification { preferred }
> > in a mock hid authenticator. Test timed out
> > +NOTRUN PublicKeyCredential's [[get]] with userVerification { discouraged }
> > in a mock hid authenticator. 
> > +NOTRUN PublicKeyCredential's [[get]] with mixed options in a mock hid
> > authenticator.
> 
> It is not flaky in my iMac Pro. I suspect we should make it long. I will do
> the gardening. Thanks for the head up.

By saying long, I mean [ Slow ].

[Jiewen Tan]

Test gardening:
Committed r238243: <https://trac.webkit.org/changeset/238243>

[Truitt Savell]

Most of the tests added in https://trac.webkit.org/changeset/238166/webkit

are flakey failures and timeouts. 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fctap-hid-success.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-create-failure-hid-silent.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-create-failure-hid.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-create-success-hid.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-failure-hid-silent.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-failure-hid.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-success-hid.https.html

[Jiewen Tan]

(In reply to Truitt Savell from comment #20)
> Most of the tests added in https://trac.webkit.org/changeset/238166/webkit
> 
> are flakey failures and timeouts. 
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fctap-hid-success.https.
> html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-create-failure-hid-
> silent.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-create-
> failure-hid.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-
> create-success-hid.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-
> credential-get-failure-hid-silent.https.
> html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-failure-hid.https.
> html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-success-hid.https.
> html

Thanks for monitoring all WebAuthN tests. It looks like that only http/wpt/webauthn/public-key-credential-create-success-hid.https.html and http/wpt/webauthn/public-key-credential-get-success-hid.https.html are flaky. Other failures are results of these two. I have filed Bug 192061 to tackle those tests as it is not trivial for me to determine why. I can't reproduce any failures in my local machines.

I will make an unreviewed test gardening commit to isolate the above two tests.

[Jiewen Tan]

(In reply to Jiewen Tan from comment #21)
> (In reply to Truitt Savell from comment #20)
> > Most of the tests added in https://trac.webkit.org/changeset/238166/webkit
> > 
> > are flakey failures and timeouts. 
> > 
> > History:
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fctap-hid-success.https.
> > html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-create-failure-hid-
> > silent.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-create-
> > failure-hid.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-
> > create-success-hid.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-
> > credential-get-failure-hid-silent.https.
> > html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-failure-hid.https.
> > html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-success-hid.https.
> > html
> 
> Thanks for monitoring all WebAuthN tests. It looks like that only
> http/wpt/webauthn/public-key-credential-create-success-hid.https.html and
> http/wpt/webauthn/public-key-credential-get-success-hid.https.html are
> flaky. Other failures are results of these two. I have filed Bug 192061 to
> tackle those tests as it is not trivial for me to determine why. I can't
> reproduce any failures in my local machines.
> 
> I will make an unreviewed test gardening commit to isolate the above two
> tests.

Committed r238598: <https://trac.webkit.org/changeset/238598>

[Frédéric Wang (:fredw)]

Committed r238729: <https://trac.webkit.org/changeset/238729>

[Frédéric Wang (:fredw)]

Comment on attachment 354750 [details]
Patch for Landing

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

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.mm:40
> +    ASSERT(RunLoop::isMain());

This requires RunLoop.h.

I landed https://trac.webkit.org/changeset/238729/webkit

[Frédéric Wang (:fredw)]

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

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidConnection.mm:40
> +    ASSERT(RunLoop::isMain());

This requires RunLoop.h.

I landed https://trac.webkit.org/changeset/238729/webkit