Add timeout for ensurePositionInformationIsUpToDate

[Megan Gardner]

Add timout for ensurePositionInformationIsUpToDate

[Megan Gardner]

Created attachment 337857 [details]
Patch

[Megan Gardner]

<rdar://problem/38729231>

[Alexey Proskuryakov]

Comment on attachment 337857 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        so as to not hang the UI.

What kinds of correctness bugs will this introduce?

[Megan Gardner]

Basically sometimes longPresses will fail to bring up action sheets, or text will fail to be selected. But only if the process would have taken over one second past the already lengthy time for a long press. We believe that it's a better user experience to fail on some long presses that are already taking a long time, but allow the UI to still function rather than waiting all the time, no matter what, for positionInformation to be gathered and returned.

[Wenson Hsieh]

Comment on attachment 337857 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:-1442
> -    _hasValidPositionInformation = YES;

_hasValidPositionInformation needs to be set to YES before calling -_invokeAndRemovePendingHandlersValidForCurrentPositionInformation, otherwise -_currentPositionInformationIsValidForRequest: will return NO, and we'll never invoke the pending handlers in -_invokeAndRemovePendingHandlersValidForCurrentPositionInformation, even in the case where the IPC message came back in a timely manner.

[Megan Gardner]

Created attachment 337860 [details]
Patch

[EWS Watchlist]

Comment on attachment 337860 [details]
Patch

Attachment 337860 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7301781

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 337869 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Wenson Hsieh]

Comment on attachment 337860 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1437
> +        return connection->waitForAndDispatchImmediately<Messages::WebPageProxy::DidReceivePositionInformation>(_page->pageID(), Seconds(1), IPC::WaitForOption::InterruptWaitingIfSyncMessageArrives);

Nit - this can be 1_s

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1439
> +    _page->process().sendSync(Messages::WebPage::GetPositionInformation(request), Messages::WebPage::GetPositionInformation::Reply(_positionInformation), _page->pageID(), Seconds(1));

Nit - 1_s here too.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1444
> +        _hasValidPositionInformation = YES;

It seems like we should set `_hasValidPositionInformation = NO;` when the request times out...perhaps something like:

_hasValidPositionInformation = _page->process().sendSync(Messages::WebPage::GetPositionInformation(request), Messages::WebPage::GetPositionInformation::Reply(_positionInformation), _page->pageID(), 1_s);

...and then you don't need the isValidForRequest() check down here, because we have valid position info iff. the IPC message came back without timing out.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1448
> +    // FIXME: We need to clean up these handlers in the event that we are not able to collect data, or if the WebProcess crashes.

This is something we should really fix :(

I filed https://bugs.webkit.org/show_bug.cgi?id=184622 to track this.

> Tools/TestWebKitAPI/Tests/ios/SynchronousTimeoutTests.mm:45
> +    [webView _simulateLongPressActionAtLocation:CGPointMake(100, 100)];

Does this test time out before your change? The implementation of -_simulateLongPressActionAtLocation: is this:

- (void)_simulateLongPressActionAtLocation:(CGPoint)location
{
    RetainPtr<WKContentView> protectedSelf = self;
    [self doAfterPositionInformationUpdate:[protectedSelf] (InteractionInformationAtPosition) {
        if (SEL action = [protectedSelf _actionForLongPress])
            [protectedSelf performSelector:action];
    } forRequest:InteractionInformationRequest(roundedIntPoint(location))];
}

...which would suggest that even with an IPC timeout of ∞ (before your change), we'd still finish the test after calling -_simulateLongPressActionAtLocation:, because -doAfterPositionInformationUpdate: does not block. Instead, I think we need to call into one of the private UITextInput methods that will then call into -ensurePositionInformationIsUpToDate:. One such codepath is -hasSelectablePositionAtPoint:. An example of an existing iOS API test that exercises this is WebKit.InteractionDeadlockAfterCrash.

[Megan Gardner]

Created attachment 338922 [details]
Patch

[Megan Gardner]

Created attachment 338924 [details]
Patch

[EWS Watchlist]

Attachment 338924 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/ios/SynchronousTimeoutTests.mm:29:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Comment on attachment 338924 [details]
Patch

r=me too

[WebKit Commit Bot]

Comment on attachment 338924 [details]
Patch

Clearing flags on attachment: 338924

Committed r231080: <https://trac.webkit.org/changeset/231080>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.