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.
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 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.
Created attachment 350918 [details] Patch
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?
(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 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 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"
Created attachment 351257 [details] Patch Updated patch that addresses review feedback.
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.
Committed r236669: <https://trac.webkit.org/changeset/236669>
<rdar://problem/44913099>