Bug 189277 – [WebAuthN] Polish AuthenticatorManager and rename it to AuthenticatorCoordinator

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

[Jiewen Tan]

This task aims to polish AuthenticatorManager such that:
1) it is no longer a singleton. Instead, it will live with Page. It was a singleton simply because static PublicKeyCredential::isUserVerifyingPlatformAuthenticatorAvailable() have to access it. However, the issue can be solved by adding an attribute [CallWith=Document]. Therefore, there is no such need. Also, the singleton is illy implemented as it also owns a single IPC proxy to UI Process which means different web pages will talk to the same web page proxy. Anyway, making it live with Page should solve the problem.
2) Since we are now planning to support external authenticators, the manager of all authenticators will then have to live in UI Process which makes this AuthenticatorManager obsolete. Instead, rename it to AuthenticatorCoordinator.
3) Rename CredentialsMessenger to AuthenticatorCoordinatorClient to tight it to WebAuthN. Also, simplify the message reply model as PublicKeyCredentialCreationOptions/PublicKeyCredentialRequestOptions => ExceptionData/PublicKeyCredential for makeCredential/getAssertion operations.

[Radar WebKit Bug Importer]

<rdar://problem/44115936>

[Jiewen Tan]

Created attachment 349266 [details]
Patch

[Jiewen Tan]

Created attachment 349274 [details]
Patch

[Jiewen Tan]

Created attachment 349318 [details]
Patch

[Chris Dumez]

Comment on attachment 349318 [details]
Patch

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

r=me with suggestions.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:98
> +    encoder.encodeFixedLengthData(reinterpret_cast<const uint8_t*>(user.id.data()), user.id.length(), 1);

Why do we need the reinterpret_cast here?

Also, it seems like this could construct a temporary IPC::DataReference and encode that.

> Source/WebCore/page/Page.h:910
> +    std::unique_ptr<AuthenticatorCoordinator> m_authenticatorCoordinator;

Could this be a UniqueRef instead of a unique_ptr? It seems it cannot be null.

[Jiewen Tan]

Comment on attachment 349318 [details]
Patch

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

Thanks Chris for r+ this patch.

>> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:98
>> +    encoder.encodeFixedLengthData(reinterpret_cast<const uint8_t*>(user.id.data()), user.id.length(), 1);
> 
> Why do we need the reinterpret_cast here?
> 
> Also, it seems like this could construct a temporary IPC::DataReference and encode that.

It doesn't need to. IPC::DataReference is in WebKit so I could't use it in WebCore.

>> Source/WebCore/page/Page.h:910
>> +    std::unique_ptr<AuthenticatorCoordinator> m_authenticatorCoordinator;
> 
> Could this be a UniqueRef instead of a unique_ptr? It seems it cannot be null.

Sure, I was not aware of UniqueRef.

[Jiewen Tan]

Created attachment 349336 [details]
Patch for landing

[Jiewen Tan]

Created attachment 349347 [details]
Patch for landing

[Jiewen Tan]

Comment on attachment 349347 [details]
Patch for landing

Added #include <JavaScriptCore/HeapInlines.h> to VRStageParameters.cpp to resolve the Win EWS bots linker error.

[WebKit Commit Bot]

Comment on attachment 349347 [details]
Patch for landing

Clearing flags on attachment: 349347

Committed r235888: <https://trac.webkit.org/changeset/235888>

[Jiewen Tan]

Build fix:
Committed r235900: <https://trac.webkit.org/changeset/235900>