Bug 189992 - [iOS] Wrong key event may be sent to UIKit
Summary: [iOS] Wrong key event may be sent to UIKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 11
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks: 190571
  Show dependency treegraph
 
Reported: 2018-09-26 06:55 PDT by Daniel Bates
Modified: 2018-10-14 22:02 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.06 KB, patch)
2018-09-26 07:18 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (3.08 KB, patch)
2018-09-26 15:40 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (3.11 KB, patch)
2018-10-01 10:18 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land (4.03 KB, patch)
2018-10-01 10:40 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2018-09-26 06:55:42 PDT
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.
Comment 1 Daniel Bates 2018-09-26 07:18:04 PDT
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?
Comment 2 Daniel Bates 2018-09-26 11:45:56 PDT
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.
Comment 3 Daniel Bates 2018-09-26 15:40:37 PDT
Created attachment 350918 [details]
Patch
Comment 4 Simon Fraser (smfr) 2018-09-26 15:42:19 PDT
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?
Comment 5 Daniel Bates 2018-09-26 16:32:45 PDT
(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.
Comment 6 Joseph Pecoraro 2018-09-28 19:00:58 PDT
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.
Comment 7 Eric Carlson 2018-09-30 01:14:28 PDT
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"
Comment 8 Daniel Bates 2018-10-01 10:18:44 PDT
Created attachment 351257 [details]
Patch

Updated patch that addresses review feedback.
Comment 9 Daniel Bates 2018-10-01 10:40:36 PDT
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.
Comment 10 Daniel Bates 2018-10-01 10:41:39 PDT
Committed r236669: <https://trac.webkit.org/changeset/236669>
Comment 11 Radar WebKit Bug Importer 2018-10-01 10:42:25 PDT
<rdar://problem/44913099>