Bug 189992

Summary: [iOS] Wrong key event may be sent to UIKit
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebKit Misc.Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: enrica, eric.carlson, joepeck, megan_gardner, mitz, simon.fraser, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: iPhone / iPad   
OS: iOS 11   
Bug Depends on:    
Bug Blocks: 190571    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
To Land none

Daniel Bates
Reported 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.
Attachments
Patch (3.06 KB, patch)
2018-09-26 07:18 PDT, Daniel Bates 		no flags
DetailsFormatted DiffDiff
Patch (3.08 KB, patch)
2018-09-26 15:40 PDT, Daniel Bates 		no flags
DetailsFormatted DiffDiff
Patch (3.11 KB, patch)
2018-10-01 10:18 PDT, Daniel Bates 		no flags
DetailsFormatted DiffDiff
To Land (4.03 KB, patch)
2018-10-01 10:40 PDT, Daniel Bates 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Daniel Bates
Comment 1 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?
Daniel Bates
Comment 2 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.
Daniel Bates
Comment 3 2018-09-26 15:40:37 PDT
Created attachment 350918 [details] Patch
Simon Fraser (smfr)
Comment 4 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?
Daniel Bates
Comment 5 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.
Joseph Pecoraro
Comment 6 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.
Eric Carlson
Comment 7 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"
Daniel Bates
Comment 8 2018-10-01 10:18:44 PDT
Created attachment 351257 [details] Patch Updated patch that addresses review feedback.
Daniel Bates
Comment 9 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.
Daniel Bates
Comment 10 2018-10-01 10:41:39 PDT
Committed r236669: <https://trac.webkit.org/changeset/236669>
Radar WebKit Bug Importer
Comment 11 2018-10-01 10:42:25 PDT
<rdar://problem/44913099>
Note You need to log in before you can comment on or make changes to this bug.