Bug 189642 – [WebAuthN] Move time out control from WebProcess to UIProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jiewen Tan]

Move time out control from WebProcess to UIProcess. Otherwise, we will have a duplicate time out controller in AuthenticatorManager.

[Radar WebKit Bug Importer]

<rdar://problem/44476765>

[Jiewen Tan]

When we finished this task, we could add tests that:
1) In makeCredential, we could have a test that have exclude credentials that have the same credential id as existing one but not transport.
2) No authenticators can be discovered.

[Jiewen Tan]

3) Also, we could then add a test configuration to the discovery process to hide local authenticators.

[Jiewen Tan]

4) Re-enable time out tests in:
public-key-credential-create-failure.https.html,
public-key-credential-get-failure.https.html.

[Jiewen Tan]

Created attachment 351295 [details]
Patch

[Chris Dumez]

Comment on attachment 351295 [details]
Patch

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

> Source/WebCore/Modules/webauthn/PublicKeyCredentialRequestOptions.h:38
> +    std::optional<uint64_t> timeout;

Web IDL is using "unsigned long" so the correct C++ type is "unsigned".

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:193
> +    uint64_t timeOutInMsValue = 0;

unsigned timeOutInMsValue = std::min(maxTimeOutValue, timeOutInMs.value_or(maxTimeOutValue));

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

What guarantees there is always one timer needed at a time in the UIProcess? What prevents the AuthenticaticatorManager to process several requests in parallel?

[Jiewen Tan]

Comment on attachment 351295 [details]
Patch

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

Thanks for reviewing the patch, Chris.

>> Source/WebCore/Modules/webauthn/PublicKeyCredentialRequestOptions.h:38
>> +    std::optional<uint64_t> timeout;
> 
> Web IDL is using "unsigned long" so the correct C++ type is "unsigned".

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:193
>> +    uint64_t timeOutInMsValue = 0;
> 
> unsigned timeOutInMsValue = std::min(maxTimeOutValue, timeOutInMs.value_or(maxTimeOutValue));

Sure.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:199
>> +    m_requestTimeOutTimer = std::make_unique<Timer>([context = this]() mutable {
> 
> What guarantees there is always one timer needed at a time in the UIProcess? What prevents the AuthenticaticatorManager to process several requests in parallel?

In both AuthenticatorManager::makeCredential and AuthenticatorManager::getAssertion which are the only entrances that take requests will immediately invoke the request callback if there is an existing one as we enforce one request at a time.

[Jiewen Tan]

Created attachment 351572 [details]
Patch

[Chris Dumez]

Comment on attachment 351572 [details]
Patch

r=me

[Jiewen Tan]

(In reply to Chris Dumez from comment #9)
> Comment on attachment 351572 [details]
> Patch
> 
> r=me

Thanks, Chris.

[WebKit Commit Bot]

Comment on attachment 351572 [details]
Patch

Clearing flags on attachment: 351572

Committed r236842: <https://trac.webkit.org/changeset/236842>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.