Bug 224836 – Crash in StyledMarkupAccumulator::traverseNodesForSerialization()

• Component: HTML Editing
• Status: RESOLVED FIXED

[Julian Gonzalez]

In Release, I can hit the following crash:

WebCore::StyledMarkupAccumulator::firstChild(WebCore::Node&)
WebCore::StyledMarkupAccumulator::traverseNodesForSerialization(WebCore::Node*, WebCore::Node*, WebCore::StyledMarkupAccumulator::NodeTraversalMode)
WebCore::StyledMarkupAccumulator::serializeNodes(WebCore::Position const&, WebCore::Position const&)
WebCore::serializePreservingVisualAppearanceInternal(WebCore::Position const&, WebCore::Position const&, WTF::Vector<WebCore::Node*, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>*, WebCore::ResolveURLs, WebCore::SerializeComposedTree, WebCore::AnnotateForInterchange, WebCore::ConvertBlocksToInlines, WebCore::StandardFontFamilySerializationMode, WebCore::MSOListMode)
WebCore::serializePreservingVisualAppearance(WebCore::VisibleSelection const&, WebCore::ResolveURLs, WebCore::SerializeComposedTree, WTF::Vector<WebCore::Node*, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>*)


in Debug, we hit an assertion first:

ASSERTION FAILED: next || !pastEnd
./editing/markup.cpp : WebCore::Node *WebCore::StyledMarkupAccumulator::traverseNodesForSerialization(WebCore::Node *, WebCore::Node *, WebCore::StyledMarkupAccumulator::NodeTraversalMode)
1   0x3e7ef90d9 WTFCrash
2   0x3b0a5d2e0 PAL::canLoad_libAccessibility__AXSIsolatedTreeMode()
3   0x3b92bc1f0 WebCore::StyledMarkupAccumulator::traverseNodesForSerialization(WebCore::Node*, WebCore::Node*, WebCore::StyledMarkupAccumulator::NodeTraversalMode)
4   0x3b92bb6a0 WebCore::StyledMarkupAccumulator::serializeNodes(WebCore::Position const&, WebCore::Position const&)
5   0x3b92befc5 WebCore::serializePreservingVisualAppearanceInternal(WebCore::Position const&, WebCore::Position const&, WTF::Vector<WebCore::Node*, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>*, WebCore::ResolveURLs, WebCore::SerializeComposedTree, WebCore::AnnotateForInterchange, WebCore::ConvertBlocksToInlines, WebCore::StandardFontFamilySerializationMode, WebCore::MSOListMode)
6   0x3b92bfa9e WebCore::serializePreservingVisualAppearance(WebCore::VisibleSelection const&, WebCore::ResolveURLs, WebCore::SerializeComposedTree, WTF::Vector<WebCore::Node*, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>*)
7   0x3ba857c30 WebCore::LegacyWebArchive::createFromSelection(WebCore::Frame*)


<rdar://problem/76328325>

[Julian Gonzalez]

Created attachment 426604 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 426604 [details]
Patch

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

> Source/WebCore/editing/markup.cpp:713
> +        bool aboutToGoPastEnd = pastEnd && isDescendantOf(*pastEnd, *n) && !next;
> +        if (aboutToGoPastEnd)

This isn't quite right. When pastEnd && isDescendantOf(*pastEnd, *n) is true,
we want to set next regardless of whether next is null or not when enterNode returned false.
We currently don't hit this case because canonicalization of position
will mostly avoid that situation to arise but I don't think we want to rely on that.

The case we care about is when both of the above conditions were false.
In that case, we've entered a node and it has children so we don't want to skip them here.

So, we probably want to define a new boolean indicating condition like this:

bool didEnterNode = false;
if (!enterNode(*n))
    next = nextSkippingChildren(*n);
else if (!hasChildNodes(*n))
    exitNode(*n);
else
    didEnterNode = true;
bool aboutToGoPastEnd = pastEnd && (isDescendantOf(*pastEnd, *n) || (!next && !didEnterNode));

[Julian Gonzalez]

(In reply to Ryosuke Niwa from comment #2)
> Comment on attachment 426604 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426604&action=review
> 
> > Source/WebCore/editing/markup.cpp:713
> > +        bool aboutToGoPastEnd = pastEnd && isDescendantOf(*pastEnd, *n) && !next;
> > +        if (aboutToGoPastEnd)
> 
> This isn't quite right. When pastEnd && isDescendantOf(*pastEnd, *n) is true,
> we want to set next regardless of whether next is null or not when enterNode
> returned false.
> We currently don't hit this case because canonicalization of position
> will mostly avoid that situation to arise but I don't think we want to rely
> on that.
> 
> The case we care about is when both of the above conditions were false.
> In that case, we've entered a node and it has children so we don't want to
> skip them here.
> 
> So, we probably want to define a new boolean indicating condition like this:
> 
> bool didEnterNode = false;
> if (!enterNode(*n))
>     next = nextSkippingChildren(*n);
> else if (!hasChildNodes(*n))
>     exitNode(*n);
> else
>     didEnterNode = true;
> bool aboutToGoPastEnd = pastEnd && (isDescendantOf(*pastEnd, *n) || (!next
> && !didEnterNode));

I don't think this is quite right either, as this approach breaks several pasteboard tests:

[1286/1900] editing/pasteboard/paste-4039777-fix.html failed unexpectedly (text diff)
[1450/1900] editing/pasteboard/paste-table-001.html failed unexpectedly (text diff)
[1471/1900] editing/pasteboard/paste-text-003.html failed unexpectedly (text diff)
[1599/1900] editing/pasteboard/simplfiying-markup-should-not-strip-content.html failed unexpectedly (text diff)
[1641/1900] editing/pasteboard/testcase-9507.html failed unexpectedly (text diff)

I think this makes sense, considering that we don't necessarily want to stop if the last node is a descendent of n if it's later on.

[Julian Gonzalez]

(In reply to Julian Gonzalez from comment #3)
> (In reply to Ryosuke Niwa from comment #2)
> > Comment on attachment 426604 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=426604&action=review
> > 
> > > Source/WebCore/editing/markup.cpp:713
> > > +        bool aboutToGoPastEnd = pastEnd && isDescendantOf(*pastEnd, *n) && !next;
> > > +        if (aboutToGoPastEnd)
> > 
> > This isn't quite right. When pastEnd && isDescendantOf(*pastEnd, *n) is true,
> > we want to set next regardless of whether next is null or not when enterNode
> > returned false.
> > We currently don't hit this case because canonicalization of position
> > will mostly avoid that situation to arise but I don't think we want to rely
> > on that.
> > 
> > The case we care about is when both of the above conditions were false.
> > In that case, we've entered a node and it has children so we don't want to
> > skip them here.
> > 
> > So, we probably want to define a new boolean indicating condition like this:
> > 
> > bool didEnterNode = false;
> > if (!enterNode(*n))
> >     next = nextSkippingChildren(*n);
> > else if (!hasChildNodes(*n))
> >     exitNode(*n);
> > else
> >     didEnterNode = true;
> > bool aboutToGoPastEnd = pastEnd && (isDescendantOf(*pastEnd, *n) || (!next
> > && !didEnterNode));
> 
> I don't think this is quite right either, as this approach breaks several
> pasteboard tests:
> 
> [1286/1900] editing/pasteboard/paste-4039777-fix.html failed unexpectedly
> (text diff)
> [1450/1900] editing/pasteboard/paste-table-001.html failed unexpectedly
> (text diff)
> [1471/1900] editing/pasteboard/paste-text-003.html failed unexpectedly (text
> diff)
> [1599/1900]
> editing/pasteboard/simplfiying-markup-should-not-strip-content.html failed
> unexpectedly (text diff)
> [1641/1900] editing/pasteboard/testcase-9507.html failed unexpectedly (text
> diff)
> 
> I think this makes sense, considering that we don't necessarily want to stop
> if the last node is a descendent of n if it's later on.

Looks like

bool aboutToGoPastEnd = pastEnd && !didEnterNode && (isDescendantOf(*pastEnd, *n) || !next);

works here.

[Julian Gonzalez]

Created attachment 426728 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 426728 [details]
Patch

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

> Source/WebCore/editing/markup.cpp:715
> +        bool aboutToGoPastEnd = pastEnd && !didEnterNode && (isDescendantOf(*pastEnd, *n) || !next);

Hm... let's flip the last two expressions and check !next first since that's faster!

[Julian Gonzalez]

Created attachment 426748 [details]
Patch

[EWS]

Committed r276394 (236869@main): <https://commits.webkit.org/236869@main>

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