Bug 192839 – UI process crash when focusing an editable image

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

UI process crash when focusing an editable image

[Tim Horton]

Created attachment 357641 [details]
Patch

[Tim Horton]

<rdar://problem/46786670>

[Wenson Hsieh]

Comment on attachment 357641 [details]
Patch

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

r=me

This seems that it's not-trivial-but-also-not-terribly-difficult to write a simple test for this. Simulating a pencil-tap by triggering `_stylusSingleTapRecognized:` seems like a bit much, but maybe it's something that we'll want to be able to simulate in layout tests anyways. Or instead, we could just programmatically focus an editable image when tapping a button (or something similar), and then add a UIScriptController hook to fetch the bounds of the ink picker view.

> Source/WebKit/UIProcess/ios/WKInkPickerView.h:34
> +- (instancetype)initWithDrawingView:(WKDrawingView *)drawingView;

Maybe make this the designated initializer and make -initWithFrame:/-init unavailable?

> Source/WebKit/UIProcess/ios/WKInkPickerView.mm:32
> +

Are we putting all SoftLink headers in a different section now? Or should this be #import "PencilKitSoftLink.h" and then #import "WKDrawingView.h"?

[Tim Horton]

(In reply to Wenson Hsieh from comment #3)
> Comment on attachment 357641 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357641&action=review
> 
> r=me
> 
> This seems that it's not-trivial-but-also-not-terribly-difficult to write a
> simple test for this. Simulating a pencil-tap by triggering
> `_stylusSingleTapRecognized:` seems like a bit much, but maybe it's
> something that we'll want to be able to simulate in layout tests anyways. Or
> instead, we could just programmatically focus an editable image when tapping
> a button (or something similar), and then add a UIScriptController hook to
> fetch the bounds of the ink picker view.

I'll try to hack up a test! But actually we have tests that should have crashed... I'll see what's up.
 
> > Source/WebKit/UIProcess/ios/WKInkPickerView.h:34
> > +- (instancetype)initWithDrawingView:(WKDrawingView *)drawingView;
> 
> Maybe make this the designated initializer and make -initWithFrame:/-init
> unavailable?

Sure!

> > Source/WebKit/UIProcess/ios/WKInkPickerView.mm:32
> > +
> 
> Are we putting all SoftLink headers in a different section now? Or should
> this be #import "PencilKitSoftLink.h" and then #import "WKDrawingView.h"?

I... have no idea why that's there. Will move.

[Tim Horton]

(In reply to Tim Horton from comment #4)
> (In reply to Wenson Hsieh from comment #3)
> > > Source/WebKit/UIProcess/ios/WKInkPickerView.mm:32
> > > +
> > 
> > Are we putting all SoftLink headers in a different section now? Or should
> > this be #import "PencilKitSoftLink.h" and then #import "WKDrawingView.h"?
> 
> I... have no idea why that's there. Will move.

This is enforced by the style checker. I'm sure there's a reason:

ERROR: Source/WebKit/UIProcess/ios/WKInkPickerView.mm:32:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]

[Tim Horton]

Created attachment 357732 [details]
Patch

[Tim Horton]

Created attachment 357733 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 357733 [details]
Patch

Clearing flags on attachment: 357733

Committed r239400: <https://trac.webkit.org/changeset/239400>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.