Bug 197917

Summary: REGRESSION (r245043) [Mac WK2 Debug] ASSERTION FAILED: m_services.isEmpty() && transports.size() <= maxTransportNumber seen with two http/wpt/webauthn/public-key-credential-* tests
Product: WebKit Reporter: Shawn Roberts <sroberts>
Component: Tools / TestsAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, jiewen_tan, jlewis3, 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=196377
https://bugs.webkit.org/show_bug.cgi?id=194780
Attachments:
Description Flags
Patch
none
Patch none

Description Shawn Roberts 2019-05-15 10:33:55 PDT
The following layout tests are flaky on Mac WK2 Debug

http/wpt/webauthn/public-key-credential-create-success-hid.https.html
http/wpt/webauthn/public-key-credential-get-success-hid.https.html

Probable cause:

After changes in https://trac.webkit.org/changeset/245043/webkit the above layout tests went from flaky failures in Mac WK2 Debug runs to crashes. 

Tests were marked as flaky in previous bugs 196377, 194780 for flaky failures in Mac WK2 Release and Debug builds. Now tests will crash instead of fail on Debug. 

Reproduced with:

run-webkit-tests http/wpt/webauthn/public-key-credential-get-success-hid.https.html --iter 100 -f --debug --exit-after-n-crashes=1

Will crash with builds including 245043. Previous builds will only flakily fail with 1500 iterations. 

Crash is the same for both tests

Flakiness Dashboard:

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

Crash Log: https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r245275%20(2606)/http/wpt/webauthn/public-key-credential-create-success-hid.https-crash-log.txt

0   com.apple.JavaScriptCore      	0x0000000104e80ae0 WTFCrash + 16 (Assertions.cpp:305)
1   com.apple.WebKit              	0x00000001116d4f0b WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebKit              	0x000000011255822b WebKit::AuthenticatorManager::startDiscovery(WTF::HashSet<WebCore::AuthenticatorTransport, WTF::IntHash<WebCore::AuthenticatorTransport>, WTF::StrongEnumHashTraits<WebCore::AuthenticatorTransport> > const&) + 171 (AuthenticatorManager.cpp:224)
3   com.apple.WebKit              	0x0000000112557a99 WebKit::AuthenticatorManager::makeCredential(WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialCreationOptions const&, WTF::CompletionHandler<void (WTF::Variant<WebCore::PublicKeyCredentialData, WebCore::ExceptionData>&&)>&&) + 649 (AuthenticatorManager.cpp:143)
4   com.apple.WebKit              	0x000000011255a38e WebKit::WebAuthenticatorCoordinatorProxy::makeCredential(unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialCreationOptions const&) + 142 (WebAuthenticatorCoordinatorProxy.cpp:69)
5   com.apple.WebKit              	0x000000011296e98f void IPC::callMemberFunctionImpl<WebKit::WebAuthenticatorCoordinatorProxy, void (WebKit::WebAuthenticatorCoordinatorProxy::*)(unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialCreationOptions const&), std::__1::tuple<unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::PublicKeyCredentialCreationOptions>, 0ul, 1ul, 2ul>(WebKit::WebAuthenticatorCoordinatorProxy*, void (WebKit::WebAuthenticatorCoordinatorProxy::*)(unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialCreationOptions const&), std::__1::tuple<unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::PublicKeyCredentialCreationOptions>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul>) + 207 (HandleMessage.h:42)
6   com.apple.WebKit              	0x00000001129669c0 void IPC::callMemberFunction<WebKit::WebAuthenticatorCoordinatorProxy, void (WebKit::WebAuthenticatorCoordinatorProxy::*)(unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialCreationOptions const&), std::__1::tuple<unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::PublicKeyCredentialCreationOptions>, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul> >(std::__1::tuple<unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::PublicKeyCredentialCreationOptions>&&, WebKit::WebAuthenticatorCoordinatorProxy*, void (WebKit::WebAuthenticatorCoordinatorProxy::*)(unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialCreationOptions const&)) + 112 (HandleMessage.h:48)
7   com.apple.WebKit              	0x0000000112966580 void IPC::handleMessage<Messages::WebAuthenticatorCoordinatorProxy::MakeCredential, WebKit::WebAuthenticatorCoordinatorProxy, void (WebKit::WebAuthenticatorCoordinatorProxy::*)(unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialCreationOptions const&)>(IPC::Decoder&, WebKit::WebAuthenticatorCoordinatorProxy*, void (WebKit::WebAuthenticatorCoordinatorProxy::*)(unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialCreationOptions const&)) + 336 (HandleMessage.h:121)
8   com.apple.WebKit              	0x00000001129662e5 WebKit::WebAuthenticatorCoordinatorProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 133 (WebAuthenticatorCoordinatorProxyMessageReceiver.cpp:46)
Comment 1 Radar WebKit Bug Importer 2019-05-15 10:34:35 PDT
<rdar://problem/50815987>
Comment 2 Ryan Haddad 2019-05-15 10:39:02 PDT
ASSERTION FAILED: m_services.isEmpty() && transports.size() <= maxTransportNumber
/Volumes/Data/slave/highsierra-debug/build/Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp(224) : void WebKit::AuthenticatorManager::startDiscovery(const WebKit::AuthenticatorManager::TransportSet &)
Comment 3 Shawn Roberts 2019-05-15 12:57:43 PDT
Skipped tests on Debug in https://trac.webkit.org/changeset/245340/webkit

Expectations for Release WK2 are still set to Pass / Fail
Comment 4 Brent Fulgham 2019-05-29 10:51:59 PDT
Created attachment 370865 [details]
Patch
Comment 5 Brent Fulgham 2019-05-29 10:53:11 PDT
The changes in Bug 198308 resolved the underlying issue that triggered this failure. Local testing confirms debug runs do not assert on these tests.
Comment 6 Brent Fulgham 2019-05-29 11:50:37 PDT
You can also hit this in GetAssertion:

    #0 0x107327b2d in WTFCrash Assertions.cpp:301
    #1 0x10cd9c01a in WTFCrashWithInfo(int, char const*, char const*, int) Assertions.h:568
    #2 0x10dc559f8 in WebKit::AuthenticatorManager::getAssertion(WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialRequestOptions const&, WTF::CompletionHandler<void (WTF::Variant<WebCore::PublicKeyCredentialData, WebCore::ExceptionData>&&)>&&) AuthenticatorManager.cpp:162
    #3 0x10dc5756d in WebKit::WebAuthenticatorCoordinatorProxy::getAssertion(unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialRequestOptions const&) WebAuthenticatorCoordinatorProxy.cpp:85
    #4 0x10e01d17e in void IPC::callMemberFunctionImpl<WebKit::WebAuthenticatorCoordinatorProxy, void (WebKit::WebAuthenticatorCoordinatorProxy::*)(unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialRequestOptions const&), std::__1::tuple<unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::PublicKeyCredentialRequestOptions>, 0ul, 1ul, 2ul>(WebKit::WebAuthenticatorCoordinatorProxy*, void (WebKit::WebAuthenticatorCoordinatorProxy::*)(unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialRequestOptions const&), std::__1::tuple<unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::PublicKeyCredentialRequestOptions>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul>) HandleMessage.h:41
    #5 0x10e0198cf in void IPC::callMemberFunction<WebKit::WebAuthenticatorCoordinatorProxy, void (WebKit::WebAuthenticatorCoordinatorProxy::*)(unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialRequestOptions const&), std::__1::tuple<unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::PublicKeyCredentialRequestOptions>, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul> >(std::__1::tuple<unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul>, WebCore::PublicKeyCredentialRequestOptions>&&, WebKit::WebAuthenticatorCoordinatorProxy*, void (WebKit::WebAuthenticatorCoordinatorProxy::*)(unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialRequestOptions const&)) HandleMessage.h:47
    #6 0x10e011277 in void IPC::handleMessage<Messages::WebAuthenticatorCoordinatorProxy::GetAssertion, WebKit::WebAuthenticatorCoordinatorProxy, void (WebKit::WebAuthenticatorCoordinatorProxy::*)(unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialRequestOptions const&)>(IPC::Decoder&, WebKit::WebAuthenticatorCoordinatorProxy*, void (WebKit::WebAuthenticatorCoordinatorProxy::*)(unsigned long long, WTF::Vector<unsigned char, 0ul, WTF::CrashOnOverflow, 16ul> const&, WebCore::PublicKeyCredentialRequestOptions const&)) HandleMessage.h:120
    #7 0x10e010eac in WebKit::WebAuthenticatorCoordinatorProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) WebAuthenticatorCoordinatorProxyMessageReceiver.cpp:49
    #8 0x10ce87d48 in IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) MessageReceiverMap.cpp:123
Comment 7 Brent Fulgham 2019-05-29 12:08:58 PDT
Actually, I can still hit a debug assertion if I run the tests enough times (1000x or so), so perhaps this isn't ready yet.
Comment 8 Jiewen Tan 2019-05-29 13:07:42 PDT
(In reply to Brent Fulgham from comment #7)
> Actually, I can still hit a debug assertion if I run the tests enough times
> (1000x or so), so perhaps this isn't ready yet.

I think this is a different issue. This is a race condition that when a new request comes in right between the previous one finishes and the clearStateAsync is queue after. Therefore, when the new request starts discovery, it will still see previous request's state.

You can assign this back to me, and I will investigate a fix.
Comment 9 Brent Fulgham 2019-05-29 13:26:26 PDT
*** Bug 194780 has been marked as a duplicate of this bug. ***
Comment 10 Brent Fulgham 2019-05-29 13:28:14 PDT
*** Bug 196377 has been marked as a duplicate of this bug. ***
Comment 11 Jiewen Tan 2019-06-11 20:57:04 PDT
Created attachment 371918 [details]
Patch
Comment 12 Brent Fulgham 2019-06-12 09:32:52 PDT
Comment on attachment 371918 [details]
Patch

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

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:178
> +        return;

I'm not sure I follow this new behavior. Does calling the completion handler (e.g., lines 132 and 152 above) cause the completion handler to be cleared? I guess it must.
Comment 13 Jiewen Tan 2019-06-12 11:23:40 PDT
Comment on attachment 371918 [details]
Patch

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

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:178
>> +        return;
> 
> I'm not sure I follow this new behavior. Does calling the completion handler (e.g., lines 132 and 152 above) cause the completion handler to be cleared? I guess it must.

Yes, it does. And m_pendingCompletionHandler should be called before every clearState(). Therefore, if clearState() is called with m_pendingCompletionHandler, that means clearState() is called from clearStateAsync() and there is currently an active request. Therefore, the method should just return.
Comment 14 Brent Fulgham 2019-06-12 11:28:55 PDT
Comment on attachment 371918 [details]
Patch

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

>>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:178
>>> +        return;
>> 
>> I'm not sure I follow this new behavior. Does calling the completion handler (e.g., lines 132 and 152 above) cause the completion handler to be cleared? I guess it must.
> 
> Yes, it does. And m_pendingCompletionHandler should be called before every clearState(). Therefore, if clearState() is called with m_pendingCompletionHandler, that means clearState() is called from clearStateAsync() and there is currently an active request. Therefore, the method should just return.

Okay -- makes sense.
Comment 15 Jiewen Tan 2019-06-12 11:35:15 PDT
Comment on attachment 371918 [details]
Patch

Thanks Brent for r+ this patch.
Comment 16 WebKit Commit Bot 2019-06-12 12:06:22 PDT
Comment on attachment 371918 [details]
Patch

Clearing flags on attachment: 371918

Committed r246369: <https://trac.webkit.org/changeset/246369>
Comment 17 WebKit Commit Bot 2019-06-12 12:06:23 PDT
All reviewed patches have been landed.  Closing bug.