Bug 182032 - [WebAuthN] Implement PublicKeyCredential’s [[DiscoverFromExternalSource]] with a dummy authenticator
Summary: [WebAuthN] Implement PublicKeyCredential’s [[DiscoverFromExternalSource]] wit...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 181943
  Show dependency treegraph
 
Reported: 2018-01-23 18:15 PST by Jiewen Tan
Modified: 2018-01-24 17:35 PST (History)
6 users (show)

See Also:


Attachments
Patch (43.68 KB, patch)
2018-01-23 18:27 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.15 MB, application/zip)
2018-01-23 19:27 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (2.91 MB, application/zip)
2018-01-23 19:58 PST, EWS Watchlist
no flags Details
Patch (43.84 KB, patch)
2018-01-23 23:08 PST, Jiewen Tan
bfulgham: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (48.55 KB, patch)
2018-01-24 16:21 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2018-01-23 18:15:45 PST
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.
Comment 1 Jiewen Tan 2018-01-23 18:16:19 PST
<rdar://problem/36459922>
Comment 2 Jiewen Tan 2018-01-23 18:27:11 PST
Created attachment 332114 [details]
Patch
Comment 3 EWS Watchlist 2018-01-23 19:27:42 PST
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
Comment 4 EWS Watchlist 2018-01-23 19:27:43 PST
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
Comment 5 EWS Watchlist 2018-01-23 19:58:47 PST
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
Comment 6 EWS Watchlist 2018-01-23 19:58:49 PST
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
Comment 7 Jiewen Tan 2018-01-23 21:14:54 PST
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?
Comment 8 Jiewen Tan 2018-01-23 23:07:41 PST Comment hidden (obsolete)
Comment 9 Jiewen Tan 2018-01-23 23:08:51 PST
Created attachment 332126 [details]
Patch
Comment 10 Jiewen Tan 2018-01-23 23:09:30 PST
(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.
Comment 11 Brent Fulgham 2018-01-24 08:56:34 PST
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.
Comment 12 Jiewen Tan 2018-01-24 14:19:32 PST
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.
Comment 13 Brent Fulgham 2018-01-24 14:35:22 PST
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.
Comment 14 Jiewen Tan 2018-01-24 16:21:27 PST
Created attachment 332211 [details]
Patch for landing
Comment 15 Jiewen Tan 2018-01-24 16:23:11 PST
Thanks Brent for r+ the patch.
Comment 16 WebKit Commit Bot 2018-01-24 17:28:52 PST
Comment on attachment 332211 [details]
Patch for landing

Clearing flags on attachment: 332211

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