Bug 182032

<rdar://problem/36459922>

Created attachment 332114 [details] Patch

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

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 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

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

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?

Created attachment 332126 [details] Patch

(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 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 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 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.

Created attachment 332211 [details] Patch for landing

Thanks Brent for r+ the patch.

Comment on attachment 332211 [details] Patch for landing Clearing flags on attachment: 332211 Committed r227589: <https://trac.webkit.org/changeset/227589>