Bug 188623 - [WebAuthN] Support CTAP HID authenticators on macOS
Summary: [WebAuthN] Support CTAP HID authenticators on macOS
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-08-15 15:35 PDT by Jiewen Tan
Modified: 2019-03-28 15:49 PDT (History)
10 users (show)

See Also:


Attachments
Patch (164.35 KB, patch)
2018-11-12 01:51 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (167.43 KB, patch)
2018-11-12 12:59 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.45 MB, application/zip)
2018-11-12 14:46 PST, Build Bot
no flags Details
Patch (168.61 KB, patch)
2018-11-12 15:35 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (169.14 KB, patch)
2018-11-12 16:30 PST, Jiewen Tan
bfulgham: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff
Patch for Landing (173.26 KB, patch)
2018-11-13 18:58 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-08-15 15:35:18 PDT
Support USB authenticators on macOS.
Comment 1 Radar WebKit Bug Importer 2018-08-15 15:35:48 PDT
<rdar://problem/43353777>
Comment 2 Jiewen Tan 2018-09-04 17:45:33 PDT
This task might import CTAPHID implementation from Chromium including:
1. FidoHidDevice for protocol structure, channels and arbitration.
2. FidoHidMessage for message packet structure.
Comment 3 Jiewen Tan 2018-11-12 01:51:03 PST
Created attachment 354538 [details]
Patch
Comment 4 Jiewen Tan 2018-11-12 12:59:55 PST
Created attachment 354576 [details]
Patch
Comment 5 Build Bot 2018-11-12 14:46:43 PST Comment hidden (obsolete)
Comment 6 Build Bot 2018-11-12 14:46:45 PST Comment hidden (obsolete)
Comment 7 Jiewen Tan 2018-11-12 15:35:52 PST
Created attachment 354601 [details]
Patch
Comment 8 Jiewen Tan 2018-11-12 16:30:23 PST
Created attachment 354607 [details]
Patch
Comment 9 Brent Fulgham 2018-11-13 08:59:45 PST
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!
Comment 10 Jiewen Tan 2018-11-13 11:16:53 PST
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.
Comment 11 Chris Dumez 2018-11-13 12:14:30 PST
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.
Comment 12 Jiewen Tan 2018-11-13 18:46:58 PST
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.
Comment 13 Jiewen Tan 2018-11-13 18:58:48 PST
Created attachment 354750 [details]
Patch for Landing
Comment 14 WebKit Commit Bot 2018-11-13 22:54:40 PST
Comment on attachment 354750 [details]
Patch for Landing

Clearing flags on attachment: 354750

Committed r238166: <https://trac.webkit.org/changeset/238166>
Comment 15 Jiewen Tan 2018-11-14 00:47:11 PST
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.
Comment 16 Truitt Savell 2018-11-14 08:52:11 PST
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.
Comment 17 Jiewen Tan 2018-11-15 11:56:14 PST
(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.
Comment 18 Jiewen Tan 2018-11-15 12:59:45 PST
(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 ].
Comment 19 Jiewen Tan 2018-11-15 13:04:37 PST
Test gardening:
Committed r238243: <https://trac.webkit.org/changeset/238243>
Comment 21 Jiewen Tan 2018-11-27 18:07:27 PST
(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.
Comment 22 Jiewen Tan 2018-11-27 18:17:07 PST
(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>
Comment 23 Frédéric Wang (:fredw) 2018-11-30 04:28:07 PST
Committed r238729: <https://trac.webkit.org/changeset/238729>
Comment 24 Frédéric Wang (:fredw) 2018-11-30 04:31:06 PST
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
Comment 25 Frédéric Wang (:fredw) 2018-11-30 04:31:13 PST
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