Bug 189165 – [iOS] Multiple WKWebViewAutofillTests are flaky failures

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryan Haddad]

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

[Wenson Hsieh]

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.

[Wenson Hsieh]

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

[Ryan Haddad]

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

[Ryan Haddad]

*** Bug 193288 has been marked as a duplicate of this bug. ***

[Aakash Jain]

Can we prioritize this?
I am working on EWS for API tests, and this flaky failure is creating problem.

[Aakash Jain]

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]);

[Wenson Hsieh]

Sounds good! I will prioritize this.

[Wenson Hsieh]

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;

[Radar WebKit Bug Importer]

<rdar://problem/47433765>

[Wenson Hsieh]

Created attachment 359711 [details]
Patch

[Wenson Hsieh]

Created attachment 359713 [details]
Fix macOS builds

[Tim Horton]

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!

[Wenson Hsieh]

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

[Wenson Hsieh]

Created attachment 359777 [details]
Patch for landing (w/ typo fix)

[WebKit Commit Bot]

Comment on attachment 359777 [details]
Patch for landing (w/ typo fix)

Clearing flags on attachment: 359777

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