Summary: | Crash in BreakBlockquoteCommand::doApply() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Julian Gonzalez <julian_a_gonzalez> | ||||||||||
Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | darin, ews-watchlist, Hironori.Fujii, mifenton, rniwa, wenson_hsieh | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Julian Gonzalez
2021-04-22 12:27:33 PDT
Created attachment 426839 [details]
Patch
Comment on attachment 426839 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426839&action=review > Source/WebCore/editing/BreakBlockquoteCommand.cpp:-121 > - ASSERT(startNode); > } > - Instead of bailing out, can we use the original startNode if there is no more node left? (In reply to Ryosuke Niwa from comment #2) > Comment on attachment 426839 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=426839&action=review > > > Source/WebCore/editing/BreakBlockquoteCommand.cpp:-121 > > - ASSERT(startNode); > > } > > - > > Instead of bailing out, can we use the original startNode if there is no > more node left? This seems to work, I'll update the patch here. Created attachment 426932 [details]
Patch
Comment on attachment 426932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426932&action=review > Source/WebCore/editing/BreakBlockquoteCommand.cpp:114 > + Node* nextNode = NodeTraversal::next(*startNode); > + if (nextNode) > + startNode = nextNode; Should be scoped: if (auto nextNode = NodeTraversal::next(*startNode)) startNode = nextNode; > Source/WebCore/editing/BreakBlockquoteCommand.cpp:120 > + Node* nextNode = NodeTraversal::next(*startNode); > + startNode = childAtOffset ? childAtOffset : (nextNode ? nextNode : startNode); Ideally would like to avoid doing node traversal if childAtOffset is non-null. I’d write this: if (auto child = startNode->traverseToChildAt(pos.deprecatedEditingOffset())) startNode = child; else if (auto next = NodeTraversal::next(*startNode)) startNode = next; Created attachment 426945 [details]
Patch
Comment on attachment 426945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426945&action=review > Source/WebCore/editing/BreakBlockquoteCommand.cpp:112 > + if (auto nextNode = NodeTraversal::next(*startNode)) This is a bit misleading. This should be either auto* or we should be calling makeRefPtr & WTFMove(nextNode) below > Source/WebCore/editing/BreakBlockquoteCommand.cpp:117 > + if (auto child = startNode->traverseToChildAt(pos.deprecatedEditingOffset())) Ditto. > Source/WebCore/editing/BreakBlockquoteCommand.cpp:119 > + else if (auto next = NodeTraversal::next(*startNode)) Ditto. Created attachment 426976 [details]
Patch
Comment on attachment 426945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426945&action=review >> Source/WebCore/editing/BreakBlockquoteCommand.cpp:112 >> + if (auto nextNode = NodeTraversal::next(*startNode)) > > This is a bit misleading. This should be either auto* or we should be calling makeRefPtr & WTFMove(nextNode) below This is an interesting new coding style idea/rule that I have never heard before. I often use auto for types that are raw pointers. I know some people prefer using auto*, but this is not a preference I share. Ryosuke, are you going even further and suggesting that, due to the risk of thinking that something is a safer smart pointer, we should have a style rule to never use "auto" for a raw pointer? (In reply to Darin Adler from comment #9) > Comment on attachment 426945 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=426945&action=review > > >> Source/WebCore/editing/BreakBlockquoteCommand.cpp:112 > >> + if (auto nextNode = NodeTraversal::next(*startNode)) > > > > This is a bit misleading. This should be either auto* or we should be calling makeRefPtr & WTFMove(nextNode) below > > This is an interesting new coding style idea/rule that I have never heard > before. > > I often use auto for types that are raw pointers. I know some people prefer > using auto*, but this is not a preference I share. Oh really? I thought we always used auto* for pointer types but I guess I stand corrected. > Ryosuke, are you going even further and suggesting that, due to the risk of > thinking that something is a safer smart pointer, we should have a style > rule to never use "auto" for a raw pointer? I'm not suggesting. I thought that was the existing convention / rule but I guess it's not codified anywhere. It's probably a good practice although clang static analyzer might make it obsolete since it can automatically detect pointer types based on the actual type interference. Committed r276558 (236995@main): <https://commits.webkit.org/236995@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426976 [details]. Comment on attachment 426976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426976&action=review > LayoutTests/TestExpectations:1744 > +webkit.org/b/224941 [ Debug ] editing/pasteboard/paste-as-quotation-then-paste-crash.html [ WontFix ] This assertion failure didn't seem to be filed. Filed: Bug 225046 – editing/pasteboard/paste-as-quotation-then-paste-crash.html: ASSERTION FAILED: m_parent->hasEditableStyle() || !m_parent->renderer() |