Bug 196431 - [iOS] Crash when changing inputmode for certain types of focusable elements
Summary: [iOS] Crash when changing inputmode for certain types of focusable elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-30 17:16 PDT by Wenson Hsieh
Modified: 2019-03-31 13:02 PDT (History)
6 users (show)

See Also:


Attachments
Fixes the bug (25.43 KB, patch)
2019-03-30 18:07 PDT, Wenson Hsieh
thorton: review+
Details | Formatted Diff | Diff
Patch for landing (6.55 KB, patch)
2019-03-31 12:35 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2019-03-30 17:16:06 PDT
Null dereference; SSIA
Comment 1 Wenson Hsieh 2019-03-30 17:26:43 PDT
Alarmingly easy to repro :/
Comment 2 Radar WebKit Bug Importer 2019-03-30 17:49:03 PDT
<rdar://problem/49454962>
Comment 3 Wenson Hsieh 2019-03-30 18:07:24 PDT
Created attachment 366370 [details]
Fixes the bug
Comment 4 Tim Horton 2019-03-31 00:38:48 PDT
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?
Comment 5 Wenson Hsieh 2019-03-31 12:07:12 PDT
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.
Comment 6 Wenson Hsieh 2019-03-31 12:35:50 PDT
Created attachment 366378 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2019-03-31 13:01:50 PDT
Comment on attachment 366378 [details]
Patch for landing

Clearing flags on attachment: 366378

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