Bug 189992 – [iOS] Wrong key event may be sent to UIKit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Daniel Bates]

Currently the UIProcess retains the UIEvent associated with a keyboard event so as to defer notifying the UIKit keyboard code (via -_handleKeyUIEvent) about a received keyboard event until after the WebProcess has processed the raw key event. The UIProcess may invoke -_handleKeyUIEvent with the wrong UIEvent if new hardware keyboard events come in before the UIProcess has received notification that that the WebProcess has processed the last sent raw key event.

Additional remarks:

UIKit uses a singleton for all hardware-keyboard events. Currently WKWebView.uiEvent retains the UIEvent assigned to it. If this UIEvent is for a hardware keyboard event then it is not sufficient to retain (since we are retaining a singleton) and its value will be clobbered as new hardware keyboard events are received.

[Daniel Bates]

Created attachment 350860 [details]
Patch

I found this issue by code inspection. I am unclear how to test this change given that it effects UIProcess/UIKit behavior. Does anyone have any ideas how to test this?

[Daniel Bates]

Comment on attachment 350860 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3755
> +    webEvent.uiEvent = event._hidEvent ? [(UIPhysicalKeyboardEvent *)event _cloneEvent] : event;

Oops! This leaks the cloned event. Will update this line to call -autorelease before landing.

[Daniel Bates]

Created attachment 350918 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 350918 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        [iOS] Wrong key event may be told to UIKit

"told to UIKit" sounds weird. Maybe "sent"?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3755
> +    webEvent.uiEvent = event._hidEvent ? [[(UIPhysicalKeyboardEvent *)event _cloneEvent] autorelease] : event;

Does _cloneEvent not return an autoreleased object?

[Daniel Bates]

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 350918 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350918&action=review
> 
> > Source/WebKit/ChangeLog:3
> > +        [iOS] Wrong key event may be told to UIKit
> 
> "told to UIKit" sounds weird. Maybe "sent"?
> 

Will update ChangeLog to reflect updated bug title before landing.

> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3755
> > +    webEvent.uiEvent = event._hidEvent ? [[(UIPhysicalKeyboardEvent *)event _cloneEvent] autorelease] : event;
> 
> Does _cloneEvent not return an autoreleased object?

Correct. It does not return an autoreleased object.

[Joseph Pecoraro]

Comment on attachment 350918 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3755
>>> +    webEvent.uiEvent = event._hidEvent ? [[(UIPhysicalKeyboardEvent *)event _cloneEvent] autorelease] : event;
>> 
>> Does _cloneEvent not return an autoreleased object?
> 
> Correct. It does not return an autoreleased object.

If it does not return an autoreleased object it should probably be annotated with NS_RETURNS_RETAINED. Otherwise if we switched to ARC, our tools would look at this and think it is overreleasing.

[Eric Carlson]

Comment on attachment 350918 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        Retain a clone of a received UIEvent if its for a hardware key event so as to ensure that we

Grammar nit: "it's" or "it is"

[Daniel Bates]

Created attachment 351257 [details]
Patch

Updated patch that addresses review feedback.

[Daniel Bates]

Created attachment 351264 [details]
To Land

Patch for landing. I extracted the check for whether the event is a hardware keyboard from two places in -handleKeyEvent: into one conditional at the top of the function.

[Daniel Bates]

Committed r236669: <https://trac.webkit.org/changeset/236669>

[Radar WebKit Bug Importer]

<rdar://problem/44913099>