Bug 161947

Summary: null m_lastNodeInserted dereference at ReplaceSelectionCommand::InsertedNodes::lastLeafInserted
Product: WebKit Reporter: Fujii Hironori <Hironori.Fujii>
Component: HTML EditingAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, bugs-noreply, cdumez, commit-queue, darin, ddkilzer, enrica, ews-watchlist, mcatanzaro, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
crash-replacing-list-by-list-crash-log.txt
none
insert-table-in-paragraph-crash-crash-log.txt
none
assert patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-sierra
none
Patch none

Description Fujii Hironori 2016-09-13 22:43:36 PDT
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
Comment 1 Fujii Hironori 2016-09-13 22:46:15 PDT
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
Comment 2 Fujii Hironori 2016-09-13 23:34:25 PDT
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.
Comment 3 Fujii Hironori 2016-09-15 00:55:20 PDT
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
Comment 4 Fujii Hironori 2016-09-15 00:57:32 PDT
(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.
Comment 5 Fujii Hironori 2016-09-15 03:52:26 PDT
Created attachment 288950 [details]
assert patch
Comment 6 Chris Dumez 2016-09-15 09:26:47 PDT
I can also reproduce the crash if I add the ASSERT(). However, it does not crash without the assertion for me, even with guardmalloc.
Comment 7 Chris Dumez 2016-09-15 10:28:11 PDT
Created attachment 288967 [details]
Patch
Comment 8 Ryosuke Niwa 2016-09-19 01:58:06 PDT
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.
Comment 9 Chris Dumez 2016-09-19 09:00:47 PDT
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.
Comment 10 Ryosuke Niwa 2016-09-19 09:02:18 PDT
(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.
Comment 11 Chris Dumez 2016-09-19 09:05:53 PDT
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.
Comment 12 Ryosuke Niwa 2016-09-19 16:39:20 PDT
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.
Comment 13 Darin Adler 2016-10-07 10:47:20 PDT
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!
Comment 14 Ryosuke Niwa 2016-10-07 12:50:02 PDT
Comment on attachment 288967 [details]
Patch

r- because I don't believe this fix is correct.
Comment 15 Fujii Hironori 2018-02-05 00:18:36 PST
*** Bug 172951 has been marked as a duplicate of this bug. ***
Comment 16 Fujii Hironori 2018-02-15 22:23:50 PST
svg/custom/delete-text-crash.html and svg/custom/delete-text-innerText-crash.html have the same problem.
Comment 17 Fujii Hironori 2018-02-15 23:31:51 PST
Created attachment 334010 [details]
Patch

Created a patch based on Ryosuke's comment 12.
Comment 18 EWS Watchlist 2018-02-16 00:31:43 PST
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
Comment 19 EWS Watchlist 2018-02-16 00:31:45 PST
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
Comment 20 Michael Catanzaro 2018-02-16 09:12:45 PST
Ryosuke, what do you think about this one...?
Comment 21 Fujii Hironori 2018-02-16 14:32:48 PST
Comment on attachment 334017 [details]
Archive of layout-test-results from ews102 for mac-sierra

This EWS failure is a false positive. (Bug 182857)
Comment 22 Ryosuke Niwa 2018-02-16 20:30:09 PST
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.
Comment 23 Fujii Hironori 2018-02-18 18:56:34 PST
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?
Comment 24 Ryosuke Niwa 2018-02-19 13:40:01 PST
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.
Comment 25 WebKit Commit Bot 2018-02-19 13:43:38 PST
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
Comment 26 WebKit Commit Bot 2018-02-19 16:40:20 PST
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
Comment 27 Fujii Hironori 2018-02-19 17:38:12 PST
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.
Comment 28 WebKit Commit Bot 2018-02-19 19:21:59 PST
Comment on attachment 334211 [details]
Patch

Clearing flags on attachment: 334211

Committed r228724: <https://trac.webkit.org/changeset/228724>
Comment 29 WebKit Commit Bot 2018-02-19 19:22:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 30 Radar WebKit Bug Importer 2018-02-19 19:22:38 PST
<rdar://problem/37694847>