Bug 204664 – [iOS] WKWebView touch event gesture recognition should not block the application process main thread when possible

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Wenson Hsieh]

<rdar://problem/38670692>

[Wenson Hsieh]

...the hope is to eliminate all hangs under -[WKContentView(WKInteraction) _webTouchEventsRecognized:].

[Wenson Hsieh]

Created attachment 384949 [details]
First pass

[Wenson Hsieh]

Created attachment 385027 [details]
Fix iOS tests on non-internal builds

[Tim Horton]

Comment on attachment 385027 [details]
Fix iOS tests on non-internal builds

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:2818
> +            --m_handlingPreventableTouchStartCount;

This needs some sort of care in the underflow case. Also, it's possible we don't want to do all this work in the error case (at least on crash?)? I'm not really sure.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:168
> +#define USE_DEFERRING_GESTURE_RECOGNIZERS ENABLE(IOS_TOUCH_EVENTS)

This is fairly surprising to me. I don't usually like it when a USE() or ENABLE() is in a header that you can possibly not have (so prefix header or Platform.h/FeatureDefines.h and friends are OK, but here would not be), but 🤷‍♂️

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4211
> +    [_twoFingerDoubleTapGestureRecognizer setEnabled:NO];
> +    [_twoFingerDoubleTapGestureRecognizer setEnabled:YES];

Wonder if we should just add a _wk_cancel category method on UIGR :) this comes up enough

[Tim Horton]

Prediction: this will not be the last patch in this series :)

[Wenson Hsieh]

(In reply to Tim Horton from comment #5)
> Prediction: this will not be the last patch in this series :)

Quite likely :(

[Wenson Hsieh]

Comment on attachment 385027 [details]
Fix iOS tests on non-internal builds

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:2818
>> +            --m_handlingPreventableTouchStartCount;
> 
> This needs some sort of care in the underflow case. Also, it's possible we don't want to do all this work in the error case (at least on crash?)? I'm not really sure.

I’ll assert that it should never be zero here, and also avoid incrementing past 0 for the underflow case.

It also looks like the work here is not necessary in the case where the web process terminates, since the gestures should be reset anyways by -cleanupInteraction in WKContentView. I’ll double check by forcing the web process to crash while handling a touchstart.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.h:168
>> +#define USE_DEFERRING_GESTURE_RECOGNIZERS ENABLE(IOS_TOUCH_EVENTS)
> 
> This is fairly surprising to me. I don't usually like it when a USE() or ENABLE() is in a header that you can possibly not have (so prefix header or Platform.h/FeatureDefines.h and friends are OK, but here would not be), but 🤷‍♂️

Good point! I’ll change these to just be ENABLE(IOS_TOUCH_EVENTS) instead.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4211
>> +    [_twoFingerDoubleTapGestureRecognizer setEnabled:YES];
> 
> Wonder if we should just add a _wk_cancel category method on UIGR :) this comes up enough

Sounds good — I’ll add a -_wk_cancel category method on UIGestureRecognizer in this file.

[Wenson Hsieh]

Created attachment 385109 [details]
For EWS

[WebKit Commit Bot]

Comment on attachment 385109 [details]
For EWS

Clearing flags on attachment: 385109

Committed r253267: <https://trac.webkit.org/changeset/253267>

[Antoine Quint]

This caused bug 211179.

[Antoine Quint]

This also caused bug 211521.