Bug 207582 - WebPage::getFocusedElementInformation should be robust when the focused element changes during layout
Summary: WebPage::getFocusedElementInformation should be robust when the focused eleme...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-11 13:46 PST by Wenson Hsieh
Modified: 2020-02-11 16:53 PST (History)
8 users (show)

See Also:


Attachments
Speculative fix (11.15 KB, patch)
2020-02-11 14:10 PST, Wenson Hsieh
thorton: review+
Details | Formatted Diff | Diff
Patch for landing (11.15 KB, patch)
2020-02-11 16:07 PST, 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 2020-02-11 13:46:12 PST
<rdar://problem/47634344>
Comment 1 Wenson Hsieh 2020-02-11 14:10:26 PST
Created attachment 390421 [details]
Speculative fix
Comment 2 Tim Horton 2020-02-11 15:57:43 PST
Comment on attachment 390421 [details]
Speculative fix

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2988
> +    auto focusedElement = m_focusedElement;

auto makes it impossible to tell if this retains the element or not. But it needs to, right?
Comment 3 Wenson Hsieh 2020-02-11 15:59:42 PST
Comment on attachment 390421 [details]
Speculative fix

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

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2988
>> +    auto focusedElement = m_focusedElement;
> 
> auto makes it impossible to tell if this retains the element or not. But it needs to, right?

Since m_focusedElement is a RefPtr<Element>, auto will just copy it (and ensure that it is reffed).

If you think it's more clear, I can change this to `auto focusedElement = m_focusedElement.copyRef();`
Comment 4 Tim Horton 2020-02-11 16:04:05 PST
(In reply to Wenson Hsieh from comment #3)
> Comment on attachment 390421 [details]
> Speculative fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390421&action=review
> 
> >> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2988
> >> +    auto focusedElement = m_focusedElement;
> > 
> > auto makes it impossible to tell if this retains the element or not. But it needs to, right?
> 
> Since m_focusedElement is a RefPtr<Element>, auto will just copy it (and
> ensure that it is reffed).

Right, but it's impossible to read the code above and tell that it is correct without referring to another file :D
Comment 5 Ryosuke Niwa 2020-02-11 16:05:43 PST
(In reply to Tim Horton from comment #4)
> (In reply to Wenson Hsieh from comment #3)
> > Comment on attachment 390421 [details]
> > Speculative fix
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=390421&action=review
> > 
> > >> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2988
> > >> +    auto focusedElement = m_focusedElement;
> > > 
> > > auto makes it impossible to tell if this retains the element or not. But it needs to, right?
> > 
> > Since m_focusedElement is a RefPtr<Element>, auto will just copy it (and
> > ensure that it is reffed).
> 
> Right, but it's impossible to read the code above and tell that it is
> correct without referring to another file :D

Please do call copyRef().
Comment 6 Wenson Hsieh 2020-02-11 16:07:28 PST
Created attachment 390456 [details]
Patch for landing
Comment 7 Wenson Hsieh 2020-02-11 16:08:26 PST
Changed to call copyRef() explicitly. Thanks for the review!
Comment 8 WebKit Commit Bot 2020-02-11 16:50:04 PST
Comment on attachment 390456 [details]
Patch for landing

Clearing flags on attachment: 390456

Committed r256401: <https://trac.webkit.org/changeset/256401>