Bug 225908 - Use RefPtr for local ref counted objects of FrameSelection::setSelectionWithoutUpdatingAppearance
Summary: Use RefPtr for local ref counted objects of FrameSelection::setSelectionWitho...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords: InRadar
Depends on: 225219
Blocks:
  Show dependency treegraph
 
Reported: 2021-05-17 23:10 PDT by Frédéric Wang (:fredw)
Modified: 2021-05-18 03:01 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.59 KB, patch)
2021-05-17 23:11 PDT, Frédéric Wang (:fredw)
rniwa: review+
Details | Formatted Diff | Diff
Patch for landing (4.58 KB, patch)
2021-05-18 02:34 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2021-05-17 23:10:12 PDT
.
Comment 1 Radar WebKit Bug Importer 2021-05-17 23:10:26 PDT
<rdar://problem/78140308>
Comment 2 Frédéric Wang (:fredw) 2021-05-17 23:11:27 PDT
Created attachment 428915 [details]
Patch
Comment 3 Ryosuke Niwa 2021-05-18 01:49:21 PDT
Comment on attachment 428915 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:1944
> -    ContainerNode* ownerElementParent = ownerElement->parentNode();
> +    RefPtr<ContainerNode> ownerElementParent = ownerElement->parentNode();

Use makeRefPtr?
Comment 4 Ryosuke Niwa 2021-05-18 01:49:37 PDT
I don't think this needs to be in the security component unless there is an actual security bug we're fixing.
Comment 5 Frédéric Wang (:fredw) 2021-05-18 01:54:07 PDT
Comment on attachment 428915 [details]
Patch

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

>> Source/WebCore/editing/FrameSelection.cpp:1944
>> +    RefPtr<ContainerNode> ownerElementParent = ownerElement->parentNode();
> 
> Use makeRefPtr?

This needs to be cast to RefPtr<ContainerNode>. Not sure how we do that in our code... or maybe you mean adding makeRefPtr in any case?
Comment 6 Frédéric Wang (:fredw) 2021-05-18 01:55:34 PDT
(In reply to Ryosuke Niwa from comment #4)
> I don't think this needs to be in the security component unless there is an
> actual security bug we're fixing.

Yeah, reading the code, I don't think any of this use is "dangerous".
Comment 7 Ryosuke Niwa 2021-05-18 02:03:51 PDT
Comment on attachment 428915 [details]
Patch

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

>>> Source/WebCore/editing/FrameSelection.cpp:1944
>>> +    RefPtr<ContainerNode> ownerElementParent = ownerElement->parentNode();
>> 
>> Use makeRefPtr?
> 
> This needs to be cast to RefPtr<ContainerNode>. Not sure how we do that in our code... or maybe you mean adding makeRefPtr in any case?

I'm confused. Given Node::parentNode() returns ContainerNode, calling makeRefPtr will surely make it RefPtr<ContainerNode>?
Comment 8 Frédéric Wang (:fredw) 2021-05-18 02:31:44 PDT
Comment on attachment 428915 [details]
Patch

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

>>>> Source/WebCore/editing/FrameSelection.cpp:1944
>>>> +    RefPtr<ContainerNode> ownerElementParent = ownerElement->parentNode();
>>> 
>>> Use makeRefPtr?
>> 
>> This needs to be cast to RefPtr<ContainerNode>. Not sure how we do that in our code... or maybe you mean adding makeRefPtr in any case?
> 
> I'm confused. Given Node::parentNode() returns ContainerNode, calling makeRefPtr will surely make it RefPtr<ContainerNode>?

OK, I stand corrected. I had build errors when I tried yesterday, but probably that was something else. Will update the patch and land it.
Comment 9 Frédéric Wang (:fredw) 2021-05-18 02:34:32 PDT
Created attachment 428923 [details]
Patch for landing
Comment 10 EWS 2021-05-18 03:01:34 PDT
Committed r277647 (237853@main): <https://commits.webkit.org/237853@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428923 [details].