Bug 224941 - Crash in BreakBlockquoteCommand::doApply()
Summary: Crash in BreakBlockquoteCommand::doApply()
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: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-22 12:27 PDT by Julian Gonzalez
Modified: 2021-04-25 23:11 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.49 KB, patch)
2021-04-22 12:42 PDT, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (5.55 KB, patch)
2021-04-23 12:07 PDT, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (5.59 KB, patch)
2021-04-23 14:19 PDT, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (5.60 KB, patch)
2021-04-24 00:57 PDT, Julian Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Gonzalez 2021-04-22 12:27:33 PDT
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>
Comment 1 Julian Gonzalez 2021-04-22 12:42:35 PDT
Created attachment 426839 [details]
Patch
Comment 2 Ryosuke Niwa 2021-04-22 23:59:28 PDT
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?
Comment 3 Julian Gonzalez 2021-04-23 12:03:27 PDT
(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.
Comment 4 Julian Gonzalez 2021-04-23 12:07:14 PDT
Created attachment 426932 [details]
Patch
Comment 5 Darin Adler 2021-04-23 13:04:13 PDT
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;
Comment 6 Julian Gonzalez 2021-04-23 14:19:56 PDT
Created attachment 426945 [details]
Patch
Comment 7 Ryosuke Niwa 2021-04-23 18:18:24 PDT
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.
Comment 8 Julian Gonzalez 2021-04-24 00:57:59 PDT
Created attachment 426976 [details]
Patch
Comment 9 Darin Adler 2021-04-24 10:14:13 PDT
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?
Comment 10 Ryosuke Niwa 2021-04-24 15:28:08 PDT
(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.
Comment 11 EWS 2021-04-24 15:32:06 PDT
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 12 Fujii Hironori 2021-04-25 23:11:56 PDT
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()