[webkit-reviews] review granted: [Bug 193758] [iOS] Unable to make a selection in jsfiddle.net using arrow keys when requesting desktop site : [Attachment 361025] Use EditingBehavior

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 193758: [iOS] Unable to make a selection in jsfiddle.net using arrow keys
when requesting desktop site
https://bugs.webkit.org/show_bug.cgi?id=193758

Attachment 361025: Use EditingBehavior

https://bugs.webkit.org/attachment.cgi?id=361025&action=review




--- Comment #19 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 361025
  --> https://bugs.webkit.org/attachment.cgi?id=361025
Use EditingBehavior

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

>>> Source/WebCore/html/HTMLInputElement.cpp:474
>>> +	 auto frame = makeRefPtr(document().frame());
>> 
>> Is it necessary to take out a RefPtr for this? This seems like some
next-level craziness or paranoia if anyone along this chain to shouldMove...()
were to cause frame destruction and the world might as well end if
shouldMove...() is the one that causes it. Not a big thing in the grand scheme
of things, but definitely not minimal.
> 
> Fair enough. It's not incorrect to use a raw pointer here. But note that in
the future, if any code were to run after calling setSelectionRange() that uses
this raw Frame pointer, it would be a potential UAF.

FYI, not really opposed to the makeRefPtr(). I think it's silly in this case,
but r=me I am not going to stop you for it. I don't know anymore what is good
engineering practice for dealing with this. UAF is bad. However, this paranoia
you have for code after calling setSelection..()... that's setSelection...()'s
problem. That function should take out the ref if it needs lifetime guarantees
not its caller or it's caller's caller or its caller's caller's caller or 50
caller's up. If we're at that level of craziness then garbage collecting the
world doesn't seem half bad. I don't want to be responsible for a UAF and I
don't have a checkout at the moment to audit the code to say with any level
confidence other than gut feeling that we won't have a UAF. But I can say
this... the code here doesn't take out a ref of the frame for setSelection...()
so we have the situation you describe then we have a UAF now. I say, fix
setSelec..() and everyone else that needs lifetime guarantees.

>>> Source/WebCore/html/HTMLInputElement.cpp:479
>>>	 setSelectionRange(start, std::numeric_limits<int>::max(), direction,
revealMode, Element::defaultFocusTextStateChangeIntent());
>> 
>> Why don't we just bail out early if we don't have a frame? Does
setSelectionRange() do this? If it does, why not bail out before calling it?
> 
> setSelectionRange() does not bail early if frame is null. But as far as I can
tell, the only state change it makes (besides updating layout) is calling
`cacheSelection`. So it /seems/ like it should be safe to bail early, both in
setSelectionRange and all of the helper methods that wrap it (e.g.
setSelectionStart, setSelectionEnd, setSelectionDirection, select,
setDefaultSelectionAfterFocus...). Regardless, this seems like a bigger change
than is necessary for this patch, which is just about removing the compile-time
guard.
> 
> How do you feel about making this a separate change?

No preference. Just wanted ask.