Summary: | null m_lastNodeInserted dereference at ReplaceSelectionCommand::InsertedNodes::lastLeafInserted | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||||||||||
Component: | HTML Editing | Assignee: | 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
Fujii Hironori
2016-09-13 22:43:36 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 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.
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 (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. Created attachment 288950 [details]
assert patch
I can also reproduce the crash if I add the ASSERT(). However, it does not crash without the assertion for me, even with guardmalloc. Created attachment 288967 [details]
Patch
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 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. (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 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. 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 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 on attachment 288967 [details]
Patch
r- because I don't believe this fix is correct.
*** Bug 172951 has been marked as a duplicate of this bug. *** svg/custom/delete-text-crash.html and svg/custom/delete-text-innerText-crash.html have the same problem. Created attachment 334010 [details] Patch Created a patch based on Ryosuke's comment 12. 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 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
Ryosuke, what do you think about this one...? 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 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. 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 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 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 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 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 on attachment 334211 [details] Patch Clearing flags on attachment: 334211 Committed r228724: <https://trac.webkit.org/changeset/228724> All reviewed patches have been landed. Closing bug. |