Bug 189165

Summary: [iOS] Multiple WKWebViewAutofillTests are flaky failures
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: Tools / TestsAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, bdakin, commit-queue, jbedard, jlewis3, lforschler, megan_gardner, realdawei, thorton, tsavell, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Fix macOS builds
thorton: review+
Patch for landing (w/ typo fix) none

Description Ryan Haddad 2018-08-30 11:28:32 PDT
WKWebViewAutofillTests.StandalonePasswordField is a flaky failure on iOS Simulator bots

    TestWebKitAPI.WKWebViewAutofillTests.StandalonePasswordField
        LEAK: 1 WebProcessPool
        LEAK: 1 WebPageProxy
        
        /Volumes/Data/slave/ios-simulator-11-debug/build/Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:142
        Value of: [webView textInputHasAutofillContext]
          Actual: true
        Expected: false

https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/builds/6188/steps/run-api-tests/logs/stdio
Comment 1 Wenson Hsieh 2018-08-30 11:41:25 PDT
I suspect we need to wait for the input session to actually begin in these tests, since that can be deferred until the next post-layout EditorState arrives in the UI process.
Comment 2 Wenson Hsieh 2018-08-30 11:46:05 PDT
(In reply to Wenson Hsieh from comment #1)
> I suspect we need to wait for the input session to actually begin in these
> tests, since that can be deferred until the next post-layout EditorState
> arrives in the UI process.

Never mind, textInputHasAutofillContext already waits for the next presentation update (which should include an up-to-date EditorState). Need to keep looking...
Comment 3 Ryan Haddad 2018-09-07 17:30:54 PDT
It looks like there are more flaky autofill tests:

    TestWebKitAPI.WKWebViewAutofillTests.StandalonePasswordField
        LEAK: 1 WebProcessPool
        LEAK: 1 WebPageProxy
        
        /Volumes/Data/slave/ios-simulator-11-debug/build/Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:142
        Value of: [webView textInputHasAutofillContext]
          Actual: true
        Expected: false

https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/builds/6339/steps/run-api-tests/logs/stdio


    TestWebKitAPI.WKWebViewAutofillTests.UsernameAndPasswordFieldSeparatedByRadioButton
        
        /Volumes/Data/slave/ios-simulator-11-release/build/Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:115
        Value of: [webView textInputHasAutofillContext]
          Actual: true
        Expected: false

https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/builds/7303/steps/run-api-tests/logs/stdio
Comment 4 Ryan Haddad 2019-01-09 10:27:54 PST
*** Bug 193288 has been marked as a duplicate of this bug. ***
Comment 5 Aakash Jain 2019-01-18 11:30:07 PST
Can we prioritize this?
I am working on EWS for API tests, and this flaky failure is creating problem.
Comment 6 Aakash Jain 2019-01-18 11:34:14 PST
One more related flaky test: TestWebKitAPI.WKWebViewAutofillTests.UsernameAndPasswordField

Noticed in: https://ews-build.webkit-uat.org/#/builders/20/builds/215

It seems like all these flaky tests are failing at similar place:
EXPECT_FALSE([webView textInputHasAutofillContext]);
Comment 7 Wenson Hsieh 2019-01-18 11:35:19 PST
Sounds good! I will prioritize this.
Comment 8 Wenson Hsieh 2019-01-21 15:37:50 PST
I can reproduce this somewhat regularly by running the function body of WKWebViewAutofillTests.StandalonePasswordField 50 times in a for loop.

In the implementation of the testing helper method -textInputHasAutofillContext, we first attempt to wait until the next presentation update before consulting -_autofillContext; the idea is that by bouncing to the web process and back, we allow any focused element changes (due to elements blurring or receiving focus) to finish before checking for state that depends on the UI-side input session.

However, in the case where an element is being blurred, this doesn't (always) work because the IPC message when blurring an element is dispatched asynchronously on the main thread (see: WebPage::elementDidBlur). This means that we're racing the element blur IPC message dispatch against the remote layer tree commit. In most cases (at least, from what I observe on my machine), we win the race because there isn't already a layer tree flush scheduled in the web process when we try to wait for the next presentation update, and so we get a sequence of events like this:

(UI process): evaluate JS ("document.activeElement.blur()")
(UI process): do after next presentation update
(Web process): WebPage::elementDidBlur()
(Web process): RemoteLayerTreeDrawingArea::flushLayers()
(Web process): WebPage::elementDidBlur() - actually send the message
(Web process): BackingStoreFlusher::flush()
(UI process): WebPageProxy::elementDidBlur()
(UI process): call do after next presentation update completion handler

...but if we lose the race, it looks something like:

(Web process): RemoteLayerTreeDrawingArea::flushLayers()
  .
  .
  .
(UI process): evaluate JS ("document.activeElement.blur()")
(UI process): do after next presentation update
(Web process): WebPage::elementDidBlur()
(Web process): BackingStoreFlusher::flush()
(Web process): WebPage::elementDidBlur() - actually send the message
(UI process): call do after next presentation update completion handler
(UI process): WebPageProxy::elementDidBlur()

...and so in this case, we subsequently continue the test before the input session is actually dismissed in the UI process. The fix is probably to avoid this race by using these testing hooks to figure out when we've focused or blurred, rather than waiting for presentation updates:

- (void)didStartFormControlInteraction;
- (void)didEndFormControlInteraction;
Comment 9 Radar WebKit Bug Importer 2019-01-21 16:15:27 PST
<rdar://problem/47433765>
Comment 10 Wenson Hsieh 2019-01-21 16:29:34 PST
Created attachment 359711 [details]
Patch
Comment 11 Wenson Hsieh 2019-01-21 17:19:53 PST
Created attachment 359713 [details]
Fix macOS builds
Comment 12 Tim Horton 2019-01-22 13:23:20 PST
Comment on attachment 359713 [details]
Fix macOS builds

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

> Tools/TestWebKitAPI/Tests/ios/WKWebViewAutofillTests.mm:83
> -    [webView stringByEvaluatingJavaScript:@"user.focus()"];
> +    [webView evaluateJavaScriptAndWaitForInputSessionToChange:@"user.focus()"];

Nice!
Comment 13 Wenson Hsieh 2019-01-22 14:04:40 PST
Comment on attachment 359713 [details]
Fix macOS builds

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

Thanks for the review!

> Tools/ChangeLog:38
> +        Monotonically increasing identifier that's incremeneted whenever an input session is started in the UI process.

Oops, "incremeneted" :|
Comment 14 Wenson Hsieh 2019-01-22 14:43:32 PST
Created attachment 359777 [details]
Patch for landing (w/ typo fix)
Comment 15 WebKit Commit Bot 2019-01-22 15:21:54 PST
Comment on attachment 359777 [details]
Patch for landing (w/ typo fix)

Clearing flags on attachment: 359777

Committed r240301: <https://trac.webkit.org/changeset/240301>