Bug 189165 - [iOS] Multiple WKWebViewAutofillTests are flaky failures
Summary: [iOS] Multiple WKWebViewAutofillTests are flaky failures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
: 193288 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-08-30 11:28 PDT by Ryan Haddad
Modified: 2019-01-22 15:24 PST (History)
13 users (show)

See Also:


Attachments
Patch (14.03 KB, patch)
2019-01-21 16:29 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Fix macOS builds (14.07 KB, patch)
2019-01-21 17:19 PST, Wenson Hsieh
thorton: review+
Details | Formatted Diff | Diff
Patch for landing (w/ typo fix) (14.02 KB, patch)
2019-01-22 14:43 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>