| Summary: | editing/firstPositionInNode-crash.html in crashing in Debug | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
| Component: | HTML Editing | Assignee: | Chris Dumez <cdumez> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | bfulgham, commit-queue, dbates, esprehn+autocc, ews-feeder, ews-watchlist, ggaren, kangil.han, product-security, rniwa, webkit-bug-importer, wenson_hsieh | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Chris Dumez
2019-10-28 14:22:48 PDT
Created attachment 382106 [details]
Patch
Comment on attachment 382106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382106&action=review > Source/WebCore/editing/Editing.h:248 > +inline Position positionInParentBeforeNode(const Node* node) I don't think we want to generate the loop everywhere. This function is used in a lot of places. We should just make this out-of-line function in Editing.cpp instead. > Source/WebCore/editing/Editing.h:254 > + return Position(ancestor, node->computeNodeIndex(), Position::PositionIsOffsetInAnchor); Clearly, we can't use whatever node index of the starting node. We need to use the node index of the last node. r- because this is clearly wrong. > Source/WebCore/editing/Editing.h:257 > +inline Position positionInParentAfterNode(const Node* node) Ditto. > Source/WebCore/editing/Editing.h:263 > + return Position(ancestor, node->computeNodeIndex() + 1, Position::PositionIsOffsetInAnchor); Ditto. Created attachment 382113 [details]
Patch
Comment on attachment 382113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382113&action=review > Source/WebCore/dom/Position.cpp:1574 > + auto* ancestor = node->parentNode(); Please use RefPtr. > Source/WebCore/dom/Position.cpp:1585 > + auto* ancestor = node->parentNode(); Ditto. Why is this in the security component?? There is no security implication here. (In reply to Ryosuke Niwa from comment #6) > Why is this in the security component?? There is no security implication > here. I thought the radar was tagged as security since this is a security test, but it looks like it is not. Comment on attachment 382113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382113&action=review >> Source/WebCore/dom/Position.cpp:1574 >> + auto* ancestor = node->parentNode(); > > Please use RefPtr. The Position() constructor takes in a raw pointer. So if ref here, it will create unnecessary ref counting churn. Why do you feel like I should ref here? I am merely going up the frame tree, not running script. (In reply to Chris Dumez from comment #8) > Comment on attachment 382113 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382113&action=review > > >> Source/WebCore/dom/Position.cpp:1574 > >> + auto* ancestor = node->parentNode(); > > > > Please use RefPtr. > > The Position() constructor takes in a raw pointer. So if ref here, it will > create unnecessary ref counting churn. Why do you feel like I should ref > here? I am merely going up the frame tree, not running script. *DOM tree, not frame tree Comment on attachment 382113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382113&action=review >>>> Source/WebCore/dom/Position.cpp:1574 >>>> + auto* ancestor = node->parentNode(); >>> >>> Please use RefPtr. >> >> The Position() constructor takes in a raw pointer. So if ref here, it will create unnecessary ref counting churn. Why do you feel like I should ref here? I am merely going up the frame tree, not running script. > > *DOM tree, not frame tree Also, if we really want to use RefPtr<>, then I'd likely have to ref node too, which means I'd have to add a new local variable to ref it. Created attachment 382114 [details]
Patch
(In reply to Chris Dumez from comment #10) > Comment on attachment 382113 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382113&action=review > > >>>> Source/WebCore/dom/Position.cpp:1574 > >>>> + auto* ancestor = node->parentNode(); > >>> > >>> Please use RefPtr. > >> > >> The Position() constructor takes in a raw pointer. So if ref here, it will create unnecessary ref counting churn. Why do you feel like I should ref here? I am merely going up the frame tree, not running script. We're trying to get rid of all uses of raw pointers in WebCore. > > *DOM tree, not frame tree > > Also, if we really want to use RefPtr<>, then I'd likely have to ref node > too, which means I'd have to add a new local variable to ref it. Okay. We can leave it for now. I'll get rid of it in a subsequent patch. Comment on attachment 382114 [details] Patch Clearing flags on attachment: 382114 Committed r251683: <https://trac.webkit.org/changeset/251683> All reviewed patches have been landed. Closing bug. |