Bug 224941 – Crash in BreakBlockquoteCommand::doApply()

• Component: HTML Editing
• Status: RESOLVED FIXED

[Julian Gonzalez]

e.g.

0 WTF::OptionSet<WebCore::Node::NodeFlag>::isEmpty()
1 WTF::OptionSet<WebCore::Node::NodeFlag>::operator bool()
2 WTF::OptionSet<WebCore::Node::NodeFlag>::containsAny(WTF::OptionSet<WebCore::Node::NodeFlag>)
3 WTF::OptionSet<WebCore::Node::NodeFlag>::contains(WebCore::Node::NodeFlag)
4 WebCore::Node::hasNodeFlag(WebCore::Node::NodeFlag)
5 WebCore::Node::isConnected()
6 WebCore::Node::isDescendantOf(WebCore::Node const&)
7 WebCore::BreakBlockquoteCommand::doApply()
8 WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::RawPtrTraits<WebCore::EditCommand> >&&)
9 WebCore::ReplaceSelectionCommand::doApply()
10 WebCore::CompositeEditCommand::apply()

<rdar://problem/76705379>

[Julian Gonzalez]

Created attachment 426839 [details]
Patch

[Ryosuke Niwa]

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?

[Julian Gonzalez]

(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.

[Julian Gonzalez]

Created attachment 426932 [details]
Patch

[Darin Adler]

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;

[Julian Gonzalez]

Created attachment 426945 [details]
Patch

[Ryosuke Niwa]

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.

[Julian Gonzalez]

Created attachment 426976 [details]
Patch

[Darin Adler]

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?

[Ryosuke Niwa]

(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.

[EWS]

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].

[Fujii Hironori]

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()