Bug 196431 – [iOS] Crash when changing inputmode for certain types of focusable elements

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

[Wenson Hsieh]

Null dereference; SSIA

[Wenson Hsieh]

Alarmingly easy to repro :/

[Radar WebKit Bug Importer]

<rdar://problem/49454962>

[Wenson Hsieh]

Created attachment 366370 [details]
Fixes the bug

[Tim Horton]

Comment on attachment 366370 [details]
Fixes the bug

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5317
>  {
> -    if (!m_focusedElement)
> +    if (!m_focusedElementForUserInput)
>          return;

I'm not sure this rename is totally necessary, especially given that you didn't rename all the things adjacent to it (like, say, this function).

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5356
> +    return is<HTMLSelectElement>(element) || isTextFormControlOrEditableContent(element);

This seems ... like an oddly small set of cases? Or maybe the function name is wrong? Any element "may require user input" if it responds to events?

[Wenson Hsieh]

Comment on attachment 366370 [details]
Fixes the bug

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

Thanks for the review!

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5317
>>          return;
> 
> I'm not sure this rename is totally necessary, especially given that you didn't rename all the things adjacent to it (like, say, this function).

Fair enough! I'll leave the renaming for a followup, then. I do think it's quite a footgun that Document has m_focusedElement and WebPage also has m_focusedElement, but they are about different concepts.

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:5356
>> +    return is<HTMLSelectElement>(element) || isTextFormControlOrEditableContent(element);
> 
> This seems ... like an oddly small set of cases? Or maybe the function name is wrong? Any element "may require user input" if it responds to events?

Yeah, I think the function name could be better. I originally went with mayRequireInputView, but it seemed too iOS-specific, so I thought "user input" was sufficiently general. I'll leave this change out for now.

[Wenson Hsieh]

Created attachment 366378 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 366378 [details]
Patch for landing

Clearing flags on attachment: 366378

Committed r243684: <https://trac.webkit.org/changeset/243684>