Crash in WebCore::StyledMarkupAccumulator::traverseNodesForSerialization

• Component: HTML Editing
• Status: RESOLVED FIXED

[Michael Catanzaro]

Created attachment 372932 [details]
Full backtrace

Looks like the problem is here in StyledMarkupAccumulator::traverseNodesForSerialization:

    Node* lastNode = nullptr;
    Node* next = nullptr;
    for (auto* n = startNode; n != pastEnd; lastNode = n, n = next) {

        Vector<Node*, 8> exitedAncestors;
        next = nullptr;
        if (auto* child = firstChild(*n)) // <-- n dereferenced when nullptr
            next = child;

Don't have a reproducer, only have the backtrace:

#0  WebCore::Node::firstChild (this=0x0) at ../Source/WebCore/dom/Node.h:595
No locals.
#1  WebCore::StyledMarkupAccumulator::firstChild (this=0x7ffcf45d82e0, node=...)
    at ../Source/WebCore/editing/markup.cpp:265
No locals.
#2  WebCore::StyledMarkupAccumulator::traverseNodesForSerialization (this=0x7ffcf45d82e0, startNode=<optimized out>, 
    pastEnd=0x7fedf4002838, traversalMode=WebCore::StyledMarkupAccumulator::NodeTraversalMode::DoNotEmitString)
    at ../Source/WebCore/editing/markup.cpp:629
        child = <optimized out>
        exitedAncestors = {<WTF::VectorBuffer<WebCore::Node*, 8>> = {<WTF::VectorBufferBase<WebCore::Node*>> = {
              m_buffer = 0x7ffcf45d8090, m_capacity = 8, m_size = 0}, m_inlineBuffer = {{
                __data = "H9\240\n\356\177\000", __align = {<No data fields>}}, {__data = "\360\267*?\357\177\000", 
                __align = {<No data fields>}}, {__data = "\000\000\000\000\000\000\000", 
                __align = {<No data fields>}}, {__data = "\000\215s\256\212\060\215\022", 
                __align = {<No data fields>}}, {__data = "\000\000\000\000\000\000\000", 
                __align = {<No data fields>}}, {__data = "\016\000\000\000\370\177\000", 
                __align = {<No data fields>}}, {__data = "\001\000\000\000\000\000\000", 
                __align = {<No data fields>}}, {__data = "\000\215s\256\212\060\215\022", 
                __align = {<No data fields>}}}}, <No data fields>}
        n = 0x0
        shouldEmit = false
        depth = 0
        enterNode = {__this = 0x7ffcf45d82e0, __shouldEmit = <synthetic pointer><error reading variable>, 
          __depth = <synthetic pointer><error reading variable>}
        lastClosed = 0x7fee0aa03948
        exitNode = {__depth = <synthetic pointer><error reading variable>, 
          __shouldEmit = <synthetic pointer><error reading variable>, __this = 0x7ffcf45d82e0, 
          __lastClosed = <synthetic pointer><error reading variable>}
        lastNode = 0x0
        next = 0x0
#3  0x00007fef3f2b94fd in WebCore::StyledMarkupAccumulator::serializeNodes (this=this@entry=0x7ffcf45d82e0, 
    start=..., end=...) at DerivedSources/ForwardingHeaders/wtf/DumbPtrTraits.h:43
        lastClosed = <optimized out>
        startNode = {
          static isRefPtr = <error reading variable: Missing ELF symbol "WTF::RefPtr<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >::isRefPtr".>, m_ptr = 0x7fedf3e01008}
        pastEnd = 0x7fedf4002838

See attachment for the remaining frames. Note: crash occurs with WebKitGTK 2.25.2 (r246495).

[Michael Catanzaro]

*** Bug 200711 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

*** Bug 197267 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> Don't have a reproducer, only have the backtrace:

I found a reproducer! Visit https://bugs.chromium.org/p/chromium/issues/detail?id=939804 and attempt to select any text on the page. Crash.

[Sergio Villar Senin]

*** Bug 204401 has been marked as a duplicate of this bug. ***

[Sergio Villar Senin]

Moving component to DOM. It isn't really a CSS issue.

[Sergio Villar Senin]

Adding Ryosuke and Dino as they were involved in the fix for bug 191921

[Sergio Villar Senin]

Actually the component should be "HTML Editing"

[Carlos Garcia Campos]

(In reply to Sergio Villar Senin from comment #6)
> Adding Ryosuke and Dino as they were involved in the fix for bug 191921

The problem is indeed nextSkippingChildren() that returns nullptr, so it's related to bug #191921

[Carlos Garcia Campos]

I can confirm this is not GTK specific, I managed to reproduce it with layout test editing/pasteboard/copy-paste-across-shadow-boundaries-with-style-2.html in Safari. Steps:

1. Open the test
2. Start selecting from hello
3. When reaching the space the selection moves and the rest is actually selected
4. Right click -> copy
-> Web process crashes

In GTK it crashes just selecting the text because we copy the selection to the primary clipboard.

[Carlos Garcia Campos]

*** Bug 212222 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

Created attachment 406503 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 406503 [details]
Patch

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

> LayoutTests/editing/pasteboard/copy-across-shadow-boundaries-crash.html:2
> +<body>

Nit: you don't need those two. Simply use <!DOCTYPE html> and remove also the closing tags.

> Source/WebCore/editing/markup.cpp:671
> +            if (pastEnd && (isDescendantOf(*pastEnd, *n) || !next))

Not sure if the !next check should be in a different if () block. 

For example what happens if !pastEnd and !next ? In theory we'd end up having n == nullptr in the next iteration leading to a crash, wouldn't we?

[Carlos Garcia Campos]

Comment on attachment 406503 [details]
Patch

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

>> LayoutTests/editing/pasteboard/copy-across-shadow-boundaries-crash.html:2
>> +<body>
> 
> Nit: you don't need those two. Simply use <!DOCTYPE html> and remove also the closing tags.

Does it matter?

>> Source/WebCore/editing/markup.cpp:671
>> +            if (pastEnd && (isDescendantOf(*pastEnd, *n) || !next))
> 
> Not sure if the !next check should be in a different if () block. 
> 
> For example what happens if !pastEnd and !next ? In theory we'd end up having n == nullptr in the next iteration leading to a crash, wouldn't we?

And what do we use then for next if pastEnd is also nullptr? I would need a test to understand that case, I'm afraid.

[Ryosuke Niwa]

Comment on attachment 406503 [details]
Patch

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

>>> Source/WebCore/editing/markup.cpp:671
>>> +            if (pastEnd && (isDescendantOf(*pastEnd, *n) || !next))
>> 
>> Not sure if the !next check should be in a different if () block. 
>> 
>> For example what happens if !pastEnd and !next ? In theory we'd end up having n == nullptr in the next iteration leading to a crash, wouldn't we?
> 
> And what do we use then for next if pastEnd is also nullptr? I would need a test to understand that case, I'm afraid.

The loop will terminate if next is null and pastEnd is also null because the loop invariant is that n != pastEnd.

[Carlos Garcia Campos]

Committed r265647: <https://trac.webkit.org/changeset/265647>

[Radar WebKit Bug Importer]

<rdar://problem/67046292>