| Summary: | [WebAuthN] Support CTAP HID authenticators on macOS | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||||||||||||
| Component: | WebCore Misc. | Assignee: | Jiewen Tan <jiewen_tan> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | alex.gaynor, bfulgham, cdumez, commit-queue, ews-watchlist, fred.wang, jiewen_tan, ryanhaddad, tsavell, webkit-bug-importer | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=192061 https://bugs.webkit.org/show_bug.cgi?id=196377 |
||||||||||||||||
| Bug Depends on: | |||||||||||||||||
| Bug Blocks: | 181943 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Jiewen Tan
2018-08-15 15:35:18 PDT
This task might import CTAPHID implementation from Chromium including: 1. FidoHidDevice for protocol structure, channels and arbitration. 2. FidoHidMessage for message packet structure. Created attachment 354538 [details]
Patch
Created attachment 354576 [details]
Patch
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 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
Created attachment 354601 [details]
Patch
Created attachment 354607 [details]
Patch
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 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 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 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. Created attachment 354750 [details]
Patch for Landing
Comment on attachment 354750 [details] Patch for Landing Clearing flags on attachment: 354750 Committed r238166: <https://trac.webkit.org/changeset/238166> 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. 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. (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. (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 ]. Test gardening: Committed r238243: <https://trac.webkit.org/changeset/238243> 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 (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. (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> Committed r238729: <https://trac.webkit.org/changeset/238729> 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 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 |