Bug 161947 – null m_lastNodeInserted dereference at ReplaceSelectionCommand::InsertedNodes::lastLeafInserted

• Component: HTML Editing
• Status: RESOLVED FIXED

[Fujii Hironori]

Created attachment 288773 [details]
crash-replacing-list-by-list-crash-log.txt

[GTK] SIGSEGV at WebCore::Node::lastDescendant in editing/inserting/insert-table-in-paragraph-crash.html

trunk@205856
Fedora 24, VMWare

Debug build does not crash on my Linux box.
BuildBot does not crash both in debug and release build.

> ./Tools/Scripts/run-webkit-tests --gtk --release --no-new-test-results editing/inserting/insert-table-in-paragraph-crash.html

> Thread 1 (Thread 0x7fb595cf7a80 (LWP 81204)):
> #0  0x00007fb593f44c10 in WebCore::Node::lastDescendant() const () from /home/fujii/work/webkit/w1/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #1  0x00007fb5940161b8 in WebCore::ReplaceSelectionCommand::removeUnrenderedTextNodesAtEnds(WebCore::ReplaceSelectionCommand::InsertedNodes&) () from /home/fujii/work/webkit/w1/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #2  0x00007fb59401c508 in WebCore::ReplaceSelectionCommand::doApply() () from /home/fujii/work/webkit/w1/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #3  0x00007fb593fa9e43 in WebCore::CompositeEditCommand::apply() () from /home/fujii/work/webkit/w1/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #4  0x00007fb593fe1f8b in WebCore::executeInsertHTML(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) () from /home/fujii/work/webkit/w1/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #5  0x00007fb593ee9322 in WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) () from /home/fujii/work/webkit/w1/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #6  0x00007fb594c0d73b in WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*) () from /home/fujii/work/webkit/w1/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
> #7  0x00007fb53b1ff028 in ?? ()
> #8  0x00007ffda02ef4a0 in ?? ()
> #9  0x00007fb591f21d6f in llint_entry () from /home/fujii/work/webkit/w1/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
> Backtrace stopped: frame did not save the PC

[Fujii Hironori]

Created attachment 288774 [details]
insert-table-in-paragraph-crash-crash-log.txt

similar crash in release build on my Linux PC.

> ./Tools/Scripts/run-webkit-tests --gtk --release --no-new-test-results editing/execCommand/crash-replacing-list-by-list.html

[Fujii Hironori]

m_lastNodeInserted was null at ReplaceSelectionCommand::InsertedNodes::lastLeafInserted.

I've confirmed this problem happens even in debug build of Gtk port If I apply following patch, 

> --- a/Source/WebCore/editing/ReplaceSelectionCommand.h
> +++ b/Source/WebCore/editing/ReplaceSelectionCommand.h
> @@ -68,7 +68,10 @@ private:
>          void didReplaceNode(Node*, Node* newNode);
>  
>          Node* firstNodeInserted() const { return m_firstNodeInserted.get(); }
> -        Node* lastLeafInserted() const { return m_lastNodeInserted->lastDescendant(); }
> +        Node* lastLeafInserted() const {
> +            ASSERT(m_lastNodeInserted);
> +            return m_lastNodeInserted->lastDescendant();
> +        }
>          Node* pastLastLeaf() const
>          {
>              if (m_lastNodeInserted) {

This bug doesn't seem Gtk port specific.

[Fujii Hironori]

These are backtraces of debug build with above ASSERT.

editing/inserting/insert-table-in-paragraph-crash.html

> Thread 1 (Thread 0x7fe187b0ba80 (LWP 10045)):
> #0  0x00007fe17b1fa1bd in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:323
> #1  0x00007fe18213a92a in (anonymous namespace)::ReplaceSelectionCommand::InsertedNodes::lastLeafInserted (this=0x7fff8fb0bd40) at ../../Source/WebCore/editing/ReplaceSelectionCommand.h:73
> #2  0x00007fe182136d4d in (anonymous namespace)::ReplaceSelectionCommand::doApply (this=0x7fe162cca2e0) at ../../Source/WebCore/editing/ReplaceSelectionCommand.cpp:1172
> #3  0x00007fe1820b9ff1 in (anonymous namespace)::CompositeEditCommand::apply (this=0x7fe162cca2e0) at ../../Source/WebCore/editing/CompositeEditCommand.cpp:342
> #4  0x00007fe1820b9dca in (anonymous namespace)::applyCommand (command=...) at ../../Source/WebCore/editing/CompositeEditCommand.cpp:301
> #5  0x00007fe1820fe24e in (anonymous namespace)::executeInsertFragment (frame=..., fragment=...) at ../../Source/WebCore/editing/EditorCommand.cpp:162
> #6  0x00007fe1820ffc19 in (anonymous namespace)::executeInsertHTML (frame=..., value=...) at ../../Source/WebCore/editing/EditorCommand.cpp:472
> #7  0x00007fe182103c3c in (anonymous namespace)::Editor::Command::execute (this=0x7fff8fb0c830, parameter=..., triggeringEvent=0x0) at ../../Source/WebCore/editing/EditorCommand.cpp:1779
> #8  0x00007fe181f61f72 in (anonymous namespace)::Document::execCommand (this=0x7fe162d23000, commandName=..., userInterface=false, value=...) at ../../Source/WebCore/dom/Document.cpp:4958
> #9  0x00007fe1831b82d4 in (anonymous namespace)::jsDocumentPrototypeFunctionExecCommand (state=0x7fff8fb0c980) at DerivedSources/WebCore/JSDocument.cpp:5390
> #10 0x00007fe122bff028 in ?? ()
> #11 0x00007fff8fb0ca00 in ?? ()
> #12 0x00007fe17ae11876 in llint_entry () at ../../Source/WTF/wtf/RefPtr.h:79
> Backtrace stopped: frame did not save the PC

editing/execCommand/crash-replacing-list-by-list.html

> Thread 1 (Thread 0x7efe63426a80 (LWP 10235)):
> #0  0x00007efe56b151bd in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:323
> #1  0x00007efe5da5592a in (anonymous namespace)::ReplaceSelectionCommand::InsertedNodes::lastLeafInserted (this=0x7fffe428b280) at ../../Source/WebCore/editing/ReplaceSelectionCommand.h:73
> #2  0x00007efe5da4ead4 in (anonymous namespace)::ReplaceSelectionCommand::removeUnrenderedTextNodesAtEnds (this=0x7efe3e73b450, insertedNodes=...) at ../../Source/WebCore/editing/ReplaceSelectionCommand.cpp:694
> #3  0x00007efe5da51852 in (anonymous namespace)::ReplaceSelectionCommand::doApply (this=0x7efe3e73b450) at ../../Source/WebCore/editing/ReplaceSelectionCommand.cpp:1137
> #4  0x00007efe5d9d4ff1 in (anonymous namespace)::CompositeEditCommand::apply (this=0x7efe3e73b450) at ../../Source/WebCore/editing/CompositeEditCommand.cpp:342
> #5  0x00007efe5d9d4dca in (anonymous namespace)::applyCommand (command=...) at ../../Source/WebCore/editing/CompositeEditCommand.cpp:301
> #6  0x00007efe5da1924e in (anonymous namespace)::executeInsertFragment (frame=..., fragment=...) at ../../Source/WebCore/editing/EditorCommand.cpp:162
> #7  0x00007efe5da1ac19 in (anonymous namespace)::executeInsertHTML (frame=..., value=...) at ../../Source/WebCore/editing/EditorCommand.cpp:472
> #8  0x00007efe5da1ec3c in (anonymous namespace)::Editor::Command::execute (this=0x7fffe428bd70, parameter=..., triggeringEvent=0x0) at ../../Source/WebCore/editing/EditorCommand.cpp:1779
> #9  0x00007efe5d87cf72 in (anonymous namespace)::Document::execCommand (this=0x7efe3e723000, commandName=..., userInterface=false, value=...) at ../../Source/WebCore/dom/Document.cpp:4958
> #10 0x00007efe5ead32d4 in (anonymous namespace)::jsDocumentPrototypeFunctionExecCommand (state=0x7fffe428bec0) at DerivedSources/WebCore/JSDocument.cpp:5390
> #11 0x00007efdfe4cf028 in ?? ()
> #12 0x00007fffe428bf40 in ?? ()
> #13 0x00007efe5672c876 in llint_entry () at ../../Source/WTF/wtf/RefPtr.h:79
> Backtrace stopped: frame did not save the PC

[Fujii Hironori]

(In reply to comment #2)
> This bug doesn't seem Gtk port specific.

I've confirmed this bug occurs on Mac port with the ASSERT patch.

[Fujii Hironori]

Created attachment 288950 [details]
assert patch

[Chris Dumez]

I can also reproduce the crash if I add the ASSERT(). However, it does not crash without the assertion for me, even with guardmalloc.

[Chris Dumez]

Created attachment 288967 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 288967 [details]
Patch

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

> Source/WebCore/editing/ReplaceSelectionCommand.h:71
> -        Node* lastLeafInserted() const { return m_lastNodeInserted->lastDescendant(); }
> +        Node* lastLeafInserted() const { return m_lastNodeInserted ? &m_lastNodeInserted->lastDescendantOrSelf() : nullptr; }

I don't think this is the right fix. m_lastNodeInserted should never be null. This must be a bug elsewhere.

[Chris Dumez]

Comment on attachment 288967 [details]
Patch

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

>> Source/WebCore/editing/ReplaceSelectionCommand.h:71
>> +        Node* lastLeafInserted() const { return m_lastNodeInserted ? &m_lastNodeInserted->lastDescendantOrSelf() : nullptr; }
> 
> I don't think this is the right fix. m_lastNodeInserted should never be null. This must be a bug elsewhere.

Interesting. Note that we already null-check m_lastNodeInserted in pastLastLeaf() below.

[Ryosuke Niwa]

(In reply to comment #9)
> Comment on attachment 288967 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288967&action=review
> 
> >> Source/WebCore/editing/ReplaceSelectionCommand.h:71
> >> +        Node* lastLeafInserted() const { return m_lastNodeInserted ? &m_lastNodeInserted->lastDescendantOrSelf() : nullptr; }
> > 
> > I don't think this is the right fix. m_lastNodeInserted should never be null. This must be a bug elsewhere.
> 
> Interesting. Note that we already null-check m_lastNodeInserted in
> pastLastLeaf() below.

pastLastLeaf() could be nullptr because m_lastNodeInserted could be the last node in the document for example.  However, m_lastNodeInserted is supposed to be continuously updated to avoid becoming null so I'm surprised that this crash is happening.

[Chris Dumez]

Comment on attachment 288967 [details]
Patch

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

>>>> Source/WebCore/editing/ReplaceSelectionCommand.h:71
>>>> +        Node* lastLeafInserted() const { return m_lastNodeInserted ? &m_lastNodeInserted->lastDescendantOrSelf() : nullptr; }
>>> 
>>> I don't think this is the right fix. m_lastNodeInserted should never be null. This must be a bug elsewhere.
>> 
>> Interesting. Note that we already null-check m_lastNodeInserted in pastLastLeaf() below.
> 
> pastLastLeaf() could be nullptr because m_lastNodeInserted could be the last node in the document for example.  However, m_lastNodeInserted is supposed to be continuously updated to avoid becoming null so I'm surprised that this crash is happening.

I understand pastLastLeaf() can return null but this is not what I said. pastLastLeaf() checks that m_lastNodeInserted is not null internally. That null check was there when you introduced the code in Bug 68875.

[Ryosuke Niwa]

Oh I see. I think we need to investigate why that null check was needed then. We really need to be careful here because this command is used by many other editing commands to move stuff around. Any incorrectness in this code can result in a bad behavior elsewhere.

[Darin Adler]

Comment on attachment 288967 [details]
Patch

Ryosuke, could you do review+ or review- please? I don’t think anyone else is going to be able to make the call!

[Ryosuke Niwa]

Comment on attachment 288967 [details]
Patch

r- because I don't believe this fix is correct.

[Fujii Hironori]

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

[Fujii Hironori]

svg/custom/delete-text-crash.html and svg/custom/delete-text-innerText-crash.html have the same problem.

[Fujii Hironori]

Created attachment 334010 [details]
Patch

Created a patch based on Ryosuke's comment 12.

[EWS Watchlist]

Comment on attachment 334010 [details]
Patch

Attachment 334010 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6531591

New failing tests:
http/tests/security/http-0.9/xhr-blocked.html

[EWS Watchlist]

Created attachment 334017 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Michael Catanzaro]

Ryosuke, what do you think about this one...?

[Fujii Hironori]

Comment on attachment 334017 [details]
Archive of layout-test-results from ews102 for mac-sierra

This EWS failure is a false positive. (Bug 182857)

[Ryosuke Niwa]

Comment on attachment 334010 [details]
Patch

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

> Source/WebCore/editing/ReplaceSelectionCommand.h:72
> +        bool isEmpty() { return !m_firstNodeInserted; }

This is not right. m_firstNodeInserted should never be null because we keep updating m_firstNodeInserted in willRemoveNode and didReplaceNode.
If m_firstNodeInserted is null, that's a bug elsewhere.

[Fujii Hironori]

Thank you for the review.

(In reply to Ryosuke Niwa from comment #22)
> Comment on attachment 334010 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334010&action=review
> 
> > Source/WebCore/editing/ReplaceSelectionCommand.h:72
> > +        bool isEmpty() { return !m_firstNodeInserted; }
> 
> This is not right. m_firstNodeInserted should never be null because we keep
> updating m_firstNodeInserted in willRemoveNode and didReplaceNode.
> If m_firstNodeInserted is null, that's a bug elsewhere.

Yes, m_firstNodeInserted is updated in willRemoveNode.
Here is the code of willRemoveNode:

https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/editing/ReplaceSelectionCommand.cpp?rev=228482#L355

> inline void ReplaceSelectionCommand::InsertedNodes::willRemoveNode(Node* node)
> {
>     if (m_firstNodeInserted == node && m_lastNodeInserted == node) {
>         m_firstNodeInserted = nullptr;
>         m_lastNodeInserted = nullptr;
>     } else if (m_firstNodeInserted == node)
>         m_firstNodeInserted = NodeTraversal::nextSkippingChildren(*m_firstNodeInserted);
>     else if (m_lastNodeInserted == node)
>         m_lastNodeInserted = NodeTraversal::previousSkippingChildren(*m_lastNodeInserted);
> }

nullptr is explicitly assigned into m_firstNodeInserted.
Do you think ASSERT_NOT_REACHED() should be inserted in this case?

And, there are already a null-check of m_firstNodeInserted there.

https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/editing/ReplaceSelectionCommand.cpp?rev=#L1150

>    // Mutation events (bug 20161) may have already removed the inserted content
>    if (!insertedNodes.firstNodeInserted() || !insertedNodes.firstNodeInserted()->isConnected())
>        return;

Do you think ASSERT(insertedNodes.firstNodeInserted()) should be inserted before this condition?

[Ryosuke Niwa]

Comment on attachment 334010 [details]
Patch

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

>>> Source/WebCore/editing/ReplaceSelectionCommand.h:72
>>> +        bool isEmpty() { return !m_firstNodeInserted; }
>> 
>> This is not right. m_firstNodeInserted should never be null because we keep updating m_firstNodeInserted in willRemoveNode and didReplaceNode.
>> If m_firstNodeInserted is null, that's a bug elsewhere.
> 
> Yes, m_firstNodeInserted is updated in willRemoveNode.
> Here is the code of willRemoveNode:
> 
> https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/editing/ReplaceSelectionCommand.cpp?rev=228482#L355

Hm... I must have misremembered this. Okay.

[WebKit Commit Bot]

Comment on attachment 334010 [details]
Patch

Rejecting attachment 334010 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 334010, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
iting/ReplaceSelectionCommand.cpp
patching file Source/WebCore/editing/ReplaceSelectionCommand.h
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/gtk/TestExpectations
Hunk #1 FAILED at 1303.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/gtk/TestExpectations.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/6577377

[WebKit Commit Bot]

Comment on attachment 334010 [details]
Patch

Rejecting attachment 334010 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 334010, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
iting/ReplaceSelectionCommand.cpp
patching file Source/WebCore/editing/ReplaceSelectionCommand.h
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/gtk/TestExpectations
Hunk #1 FAILED at 1303.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/gtk/TestExpectations.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/6579998

[Fujii Hironori]

Created attachment 334211 [details]
Patch

Thank you for the review, Ryosuke.
My bad. I didn't read the commit bot error message. I rebased the patch.

[WebKit Commit Bot]

Comment on attachment 334211 [details]
Patch

Clearing flags on attachment: 334211

Committed r228724: <https://trac.webkit.org/changeset/228724>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37694847>