Bug 224836

Summary: Crash in StyledMarkupAccumulator::traverseNodesForSerialization()
Product: WebKit Reporter: Julian Gonzalez <julian_a_gonzalez>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, mifenton, rniwa, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Julian Gonzalez 2021-04-20 15:34:06 PDT 
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>
Comment 1 Julian Gonzalez 2021-04-20 15:39:38 PDT 
Created attachment 426604 [details]
Patch
Comment 2 Ryosuke Niwa 2021-04-20 17:59:33 PDT 
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));
Comment 3 Julian Gonzalez 2021-04-21 11:49:00 PDT 
(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.
Comment 4 Julian Gonzalez 2021-04-21 12:19:37 PDT 
(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.
Comment 5 Julian Gonzalez 2021-04-21 12:26:47 PDT 
Created attachment 426728 [details]
Patch
Comment 6 Ryosuke Niwa 2021-04-21 12:49:46 PDT 
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!
Comment 7 Julian Gonzalez 2021-04-21 14:43:02 PDT 
Created attachment 426748 [details]
Patch
Comment 8 EWS 2021-04-21 15:48:46 PDT 
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].