Bug 182032 – [WebAuthN] Implement PublicKeyCredential’s [[DiscoverFromExternalSource]] with a dummy authenticator

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

[Jiewen Tan]

Implement PublicKeyCredential’s [[DiscoverFromExternalSource]] with a dummy authenticator. Here is the spec: https://www.w3.org/TR/webauthn/#getAssertion. It implements the spec as of 5 December 2017.

[Jiewen Tan]

<rdar://problem/36459922>

[Jiewen Tan]

Created attachment 332114 [details]
Patch

[EWS Watchlist]

Comment on attachment 332114 [details]
Patch

Attachment 332114 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6189945

New failing tests:
http/wpt/webauthn/public-key-credential-get-failure.https.html

[EWS Watchlist]

Created attachment 332118 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332114 [details]
Patch

Attachment 332114 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6189969

New failing tests:
http/wpt/webauthn/public-key-credential-get-failure.https.html

[EWS Watchlist]

Created attachment 332119 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Jiewen Tan]

The failure of tests is due to the flaky natures of time out operations. There is no guarantees in the code that a timeout = 0 can fire immediately and before a result is returned. Maybe some test wizards could help?

[Jiewen Tan]

(In reply to Jiewen Tan from comment #7)
> The failure of tests is due to the flaky natures of time out operations.
> There is no guarantees in the code that a timeout = 0 can fire immediately
> and before a result is returned. Maybe some test wizards could help?

I could add some noops to make Authenticator::getAssertion since it is dummy anyway.

[Jiewen Tan]

Created attachment 332126 [details]
Patch

[Jiewen Tan]

(In reply to Jiewen Tan from comment #8)
> (In reply to Jiewen Tan from comment #7)
> > The failure of tests is due to the flaky natures of time out operations.
> > There is no guarantees in the code that a timeout = 0 can fire immediately
> > and before a result is returned. Maybe some test wizards could help?
> 
I could add some noops to make Authenticator::getAssertion slower since it is dummy
anyway.

[Brent Fulgham]

Comment on attachment 332126 [details]
Patch

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

All of this looks good, except the 'getAssertion' implementation. I am very uneasy by the "do bogus work to get the tests to run properly". If you run on different hardware, your loop of 10000 may not be enough. Let's try to do this properly. For that reason, r- of this patch (just for the 'getAssertion' chunk).  Let's talk when you have a minute.

> Source/WebCore/Modules/webauthn/Authenticator.cpp:71
> +    }

This is pretty sketchy. What if you just used a 'sleep' here? I know you added this due to test flakiness, but I don't understand why the timing here matters. What's wrong with an instantaneous 'getAssertion' happening?

In general, we want to support asynchronous actions, and need to use promises or some other mechanism to allow for variation in response time from these methods. Artificially slowing this code path down to satisfy the test system seems like we aren't addressing the real problem.

Do we need to use some kind of async coordination primitive (semaphore, completion handler, etc.) to address this properly? Maybe you should just do that now when things are as simple as they can be.

> Source/WebCore/Modules/webauthn/Authenticator.h:38
> +// https://bugs.webkit.org/show_bug.cgi?id=181946

We usually just write: "FIXME(181946):" for this kind of thing.

> Source/WebCore/Modules/webauthn/PublicKeyCredential.cpp:146
> +    // FIXME: Got some crazy compile error when I was trying to return RHS directly.

Can you show the error to JF? He might be able to help figure out what's going on.

[Jiewen Tan]

Comment on attachment 332126 [details]
Patch

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

Thanks Brent for reviewing the patch.

>> Source/WebCore/Modules/webauthn/Authenticator.cpp:71
>> +    }
> 
> This is pretty sketchy. What if you just used a 'sleep' here? I know you added this due to test flakiness, but I don't understand why the timing here matters. What's wrong with an instantaneous 'getAssertion' happening?
> 
> In general, we want to support asynchronous actions, and need to use promises or some other mechanism to allow for variation in response time from these methods. Artificially slowing this code path down to satisfy the test system seems like we aren't addressing the real problem.
> 
> Do we need to use some kind of async coordination primitive (semaphore, completion handler, etc.) to address this properly? Maybe you should just do that now when things are as simple as they can be.

Sure. Let's talk.

>> Source/WebCore/Modules/webauthn/Authenticator.h:38
>> +// https://bugs.webkit.org/show_bug.cgi?id=181946
> 
> We usually just write: "FIXME(181946):" for this kind of thing.

Got it. I am going to fix all other places as well.

>> Source/WebCore/Modules/webauthn/PublicKeyCredential.cpp:146
>> +    // FIXME: Got some crazy compile error when I was trying to return RHS directly.
> 
> Can you show the error to JF? He might be able to help figure out what's going on.

Sure. Glad to show him this compile error. My bet is that there are too many autos to confuse the compiler to deduce a right type.

[Brent Fulgham]

Comment on attachment 332126 [details]
Patch

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

r=me, now that I understand that this was test code. Please create a bugzilla to document moving the test code to a proper "Internals" piece, in a follow-up step.

> Source/WebCore/Modules/webauthn/Authenticator.cpp:66
> +    // Do nothing, trying to extend the lifetime of this operation

// FIXME(): Delay processing for 5 seconds to simulate a timeout condition. This code will be removed when the full test infrastructure is set up.

>>> Source/WebCore/Modules/webauthn/Authenticator.cpp:71
>>> +    }
>> 
>> This is pretty sketchy. What if you just used a 'sleep' here? I know you added this due to test flakiness, but I don't understand why the timing here matters. What's wrong with an instantaneous 'getAssertion' happening?
>> 
>> In general, we want to support asynchronous actions, and need to use promises or some other mechanism to allow for variation in response time from these methods. Artificially slowing this code path down to satisfy the test system seems like we aren't addressing the real problem.
>> 
>> Do we need to use some kind of async coordination primitive (semaphore, completion handler, etc.) to address this properly? Maybe you should just do that now when things are as simple as they can be.
> 
> Sure. Let's talk.

I misunderstood -- you are trying to create a test case for a timeout.

[Jiewen Tan]

Created attachment 332211 [details]
Patch for landing

[Jiewen Tan]

Thanks Brent for r+ the patch.

[WebKit Commit Bot]

Comment on attachment 332211 [details]
Patch for landing

Clearing flags on attachment: 332211

Committed r227589: <https://trac.webkit.org/changeset/227589>