Bug 140261

Summary:

Null ptr crash in WebCore::LogicalSelectionOffsetCaches::ContainingBlockInfo::setBlock

Product:

WebKit

Reporter:

Renata Hodovan <rhodovan.u-szeged>

Component:

Layout and Rendering

Assignee:

Hyungwook Lee <hyungwook.lee>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, darin, ddkilzer, esprehn+autocc, glenn, gyuyoung.kim, hyatt, hyungwook.lee, kondapallykalyan, rniwa

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

116980

Attachments:

Description	Flags
Test case	none
patch	zalan: review-, buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-mavericks	none
patch	darin: review-, buildbot: commit-queue-
Archive of layout-test-results from ews105 for mac-mavericks-wk2	none
Archive of layout-test-results from ews100 for mac-mavericks	none
patch	zalan: review-, buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-mavericks	none
patch	none
patch	none
patch	none
patch	darin: review-
patch	none

Renata Hodovan

Reported 2015-01-08 11:26:42 PST

Created attachment 244274 [details] Test case The following test crashes in release/debug WK: <!DOCTYPE html> <body contenteditable> <div></div> <abbr> <label> <textarea></textarea> </label> <embed></embed> </abbr> </body> <script> document.execCommand("selectall", false, null); document.execCommand("insertorderedlist", false, null); </script> The crashing line is: m_hasFloatsOrFlowThreads = m_hasFloatsOrFlowThreads || m_block->containsFloats() || m_block->flowThreadContainingBlock(); where m_block is null. Backtrace: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fff98984700 (LWP 13967)] 0x00007ffff3874669 in WebCore::LogicalSelectionOffsetCaches::ContainingBlockInfo::setBlock (this=0x7fffffffa4b0, block=0x0, cache=0x0) at ../../Source/WebCore/rendering/LogicalSelectionOffsetCaches.h:95 95 m_hasFloatsOrFlowThreads = m_hasFloatsOrFlowThreads || m_block->containsFloats() || m_block->flowThreadContainingBlock(); (gdb) bt #0 0x00007ffff3874669 in WebCore::LogicalSelectionOffsetCaches::ContainingBlockInfo::setBlock ( this=0x7fffffffa4b0, block=0x0, cache=0x0) at ../../Source/WebCore/rendering/LogicalSelectionOffsetCaches.h:95 #1 0x00007ffff38749e0 in WebCore::LogicalSelectionOffsetCaches::LogicalSelectionOffsetCaches (this=0x7fffffffa4b0, rootBlock=...) at ../../Source/WebCore/rendering/LogicalSelectionOffsetCaches.h:147 #2 0x00007ffff3865b00 in WebCore::RenderBlock::selectionGapRectsForRepaint (this=0x7ffff7f16300, repaintContainer= 0x0) at ../../Source/WebCore/rendering/RenderBlock.cpp:1772 #3 0x00007ffff3354dc6 in WebCore::RenderBlock::selectionRectForRepaint (this=0x7ffff7f16300, repaintContainer=0x0) at ../../Source/WebCore/rendering/RenderBlock.h:462 #4 0x00007ffff3a12170 in WebCore::RenderSelectionInfo::RenderSelectionInfo (this=0x7fff982a92d8, renderer=..., clipToVisibleContent=true) at ../../Source/WebCore/rendering/RenderSelectionInfo.cpp:52 #5 0x00007ffff3a661f1 in std::make_unique<WebCore::RenderSelectionInfo, WebCore::RenderObject&, bool>(WebCore::RenderObject&, bool&&) () at ../../Source/WTF/wtf/StdLibExtras.h:337 #6 0x00007ffff3a61a30 in WebCore::RenderView::clearSubtreeSelection (this=0x7fff88f09480, root=..., blockRepaintMode=WebCore::RenderView::RepaintNewMinusOld, oldSelectionData=...) at ../../Source/WebCore/rendering/RenderView.cpp:963 #7 0x00007ffff3a615d1 in WebCore::RenderView::updateSelectionForSubtrees (this=0x7fff88f09480, renderSubtreesMap=..., blockRepaintMode=WebCore::RenderView::RepaintNewMinusOld) at ../../Source/WebCore/rendering/RenderView.cpp:927 #8 0x00007ffff3a6105f in WebCore::RenderView::setSelection (this=0x7fff88f09480, start=0x0, startPos=-1, end=0x0, endPos=-1, blockRepaintMode=WebCore::RenderView::RepaintNewMinusOld) at ../../Source/WebCore/rendering/RenderView.cpp:873 #9 0x00007ffff3a62b0d in WebCore::RenderView::clearSelection (this=0x7fff88f09480) at ../../Source/WebCore/rendering/RenderView.cpp:1097 #10 0x00007ffff31090eb in WebCore::FrameSelection::setNeedsSelectionUpdate (this=0x7ffff7f34c60) at ../../Source/WebCore/editing/FrameSelection.cpp:360 #11 0x00007ffff390ac19 in WebCore::RenderElement::removeChildInternal (this=0x7ffff7f26af8, oldChild=..., notifyChildren=WebCore::RenderElement::NotifyChildren) at ../../Source/WebCore/rendering/RenderElement.cpp:623 #12 0x00007ffff395bacd in WebCore::RenderInline::splitInlines (this=0x7ffff7f26af8, fromBlock=0x7ffff7f163c0, toBlock=0x7fff88f18cc0, middleBlock=0x7ffff7f16900, beforeChild=0x7ffff7ed55a0, oldCont=0x0) at ../../Source/WebCore/rendering/RenderInline.cpp:367 #13 0x00007ffff395c074 in WebCore::RenderInline::splitFlow (this=0x7ffff7f26af8, beforeChild=0x7ffff7ed55a0, newBlockBox=0x7ffff7f16900, newChild=0x7ffff7f16240, oldCont=0x0) at ../../Source/WebCore/rendering/RenderInline.cpp:483 #14 0x00007ffff395b8a9 in WebCore::RenderInline::addChildIgnoringContinuation (this=0x7ffff7f26af8, newChild=0x7ffff7f16240, beforeChild=0x7ffff7ed55a0) at ../../Source/WebCore/rendering/RenderInline.cpp:325 #15 0x00007ffff395b5aa in WebCore::RenderInline::addChild (this=0x7ffff7f26af8, newChild=0x7ffff7f16240, beforeChild=0x7ffff7ed55a0) at ../../Source/WebCore/rendering/RenderInline.cpp:267 #16 0x00007ffff3b5a086 in WebCore::Style::RenderTreePosition::insert (this=0x7fffffffad00, renderer=...) at ../../Source/WebCore/style/StyleResolveTree.cpp:222 #17 0x00007ffff3b5a810 in WebCore::Style::createRendererIfNeeded (element=..., inheritedStyle=..., renderTreePosition=..., resolvedStyle=...) at ../../Source/WebCore/style/StyleResolveTree.cpp:334 #18 0x00007ffff3b5bb09 in WebCore::Style::attachRenderTree (current=..., inheritedStyle=..., renderTreePosition=..., resolvedStyle=...) at ../../Source/WebCore/style/StyleResolveTree.cpp:615 #19 0x00007ffff3b5c3fc in WebCore::Style::resolveLocal (current=..., inheritedStyle=..., renderTreePosition=..., inheritedChange=WebCore::Style::NoChange) at ../../Source/WebCore/style/StyleResolveTree.cpp:756 #20 0x00007ffff3b5cb93 in WebCore::Style::resolveTree (current=..., inheritedStyle=..., renderTreePosition=..., change=WebCore::Style::NoChange) at ../../Source/WebCore/style/StyleResolveTree.cpp:918 #21 0x00007ffff3b5ce02 in WebCore::Style::resolveTree (current=..., inheritedStyle=..., renderTreePosition=..., change=WebCore::Style::NoChange) at ../../Source/WebCore/style/StyleResolveTree.cpp:955 #22 0x00007ffff3b5ce02 in WebCore::Style::resolveTree (current=..., inheritedStyle=..., renderTreePosition=..., change=WebCore::Style::NoChange) at ../../Source/WebCore/style/StyleResolveTree.cpp:955 #23 0x00007ffff3b5ce02 in WebCore::Style::resolveTree (current=..., inheritedStyle=..., renderTreePosition=..., change=WebCore::Style::NoChange) at ../../Source/WebCore/style/StyleResolveTree.cpp:955 #24 0x00007ffff3b5d0a3 in WebCore::Style::resolveTree (document=..., change=WebCore::Style::NoChange) at ../../Source/WebCore/style/StyleResolveTree.cpp:995 #25 0x00007ffff2f85e58 in WebCore::Document::recalcStyle (this=0x7fff88f0b000, change=WebCore::Style::NoChange) at ../../Source/WebCore/dom/Document.cpp:1771 #26 0x00007ffff2f8614f in WebCore::Document::updateStyleIfNeeded (this=0x7fff88f0b000) at ../../Source/WebCore/dom/Document.cpp:1819 #27 0x00007ffff2f86242 in WebCore::Document::updateLayout (this=0x7fff88f0b000) at ../../Source/WebCore/dom/Document.cpp:1838 #28 0x00007ffff2f863b2 in WebCore::Document::updateLayoutIgnorePendingStylesheets (this=0x7fff88f0b000, runPostLayoutTasks=WebCore::Document::Asynchronously) at ../../Source/WebCore/dom/Document.cpp:1876 #29 0x00007ffff3159969 in WebCore::VisiblePosition::canonicalPosition (this=0x7fffffffb740, passedPosition=...) at ../../Source/WebCore/editing/VisiblePosition.cpp:519 #30 0x00007ffff3157529 in WebCore::VisiblePosition::init (this=0x7fffffffb740, position=..., affinity=WebCore::DOWNSTREAM) at ../../Source/WebCore/editing/VisiblePosition.cpp:58 #31 0x00007ffff31574ce in WebCore::VisiblePosition::VisiblePosition (this=0x7fffffffb740, pos=..., affinity=WebCore::DOWNSTREAM) at ../../Source/WebCore/editing/VisiblePosition.cpp:51 #32 0x00007ffff311d51e in WebCore::InsertListCommand::listifyParagraph (this=0x7fff88f1cf00, originalStart=..., listTag=...) at ../../Source/WebCore/editing/InsertListCommand.cpp:393 #33 0x00007ffff311bc17 in WebCore::InsertListCommand::doApplyForSingleParagraph (this=0x7fff88f1cf00, forceCreateList=false, listTag=..., currentSelection=0x7fff982a52c0) at ../../Source/WebCore/editing/InsertListCommand.cpp:256 #34 0x00007ffff311aff2 in WebCore::InsertListCommand::doApply (this=0x7fff88f1cf00) at ../../Source/WebCore/editing/InsertListCommand.cpp:192 #35 0x00007ffff30bbd9b in WebCore::CompositeEditCommand::apply (this=0x7fff88f1cf00) at ../../Source/WebCore/editing/CompositeEditCommand.cpp:207 #36 0x00007ffff30bbb51 in WebCore::applyCommand (command=...) at ../../Source/WebCore/editing/CompositeEditCommand.cpp:167 #37 0x00007ffff3100164 in WebCore::executeInsertOrderedList (frame=...) at ../../Source/WebCore/editing/EditorCommand.cpp:549 #38 0x00007ffff310389d in WebCore::Editor::Command::execute (this=0x7fffffffbed0, parameter=..., triggeringEvent= 0x0) at ../../Source/WebCore/editing/EditorCommand.cpp:1726 #39 0x00007ffff2f90206 in WebCore::Document::execCommand (this=0x7fff88f0b000, commandName=..., userInterface=false, value=...) at ../../Source/WebCore/dom/Document.cpp:4357 #40 0x00007ffff3fd71f0 in WebCore::jsDocumentPrototypeFunctionExecCommand (exec=0x7fffffffbfd0) at DerivedSources/WebCore/JSDocument.cpp:4545 #41 0x00007fff99c7b0b4 in ?? () #42 0x00007fffffffc030 in ?? () #43 0x00007fffed8d2fa7 in llint_entry () from /home/reni/data/REPOS/webkit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18

Attachments
Test case (307 bytes, text/html) 2015-01-08 11:26 PST, Renata Hodovan	no flags	Details
patch (1.74 KB, patch) 2015-02-05 23:57 PST, Hyungwook Lee	zalan: review- buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews100 for mac-mavericks (522.15 KB, application/zip) 2015-02-06 00:42 PST, Build Bot	no flags	Details
patch (2.74 KB, patch) 2015-02-09 05:59 PST, Hyungwook Lee	darin: review- buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (665.19 KB, application/zip) 2015-02-09 06:34 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews100 for mac-mavericks (538.54 KB, application/zip) 2015-02-09 06:45 PST, Build Bot	no flags	Details
patch (3.15 KB, patch) 2015-02-10 06:29 PST, Hyungwook Lee	zalan: review- buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-mavericks (602.76 KB, application/zip) 2015-02-10 07:15 PST, Build Bot	no flags	Details
patch (3.51 KB, patch) 2015-02-13 01:54 PST, Hyungwook Lee	no flags	Details Formatted Diff Diff
patch (5.24 KB, patch) 2015-03-01 07:21 PST, Hyungwook Lee	no flags	Details Formatted Diff Diff
patch (5.24 KB, patch) 2015-03-01 07:22 PST, Hyungwook Lee	no flags	Details Formatted Diff Diff
patch (5.37 KB, patch) 2015-04-28 05:54 PDT, Hyungwook Lee	darin: review-	Details Formatted Diff Diff
patch (5.12 KB, patch) 2015-04-28 21:52 PDT, Hyungwook Lee	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Hyungwook Lee

Comment 1 2015-02-04 01:46:03 PST

I've start to look at this issue.

Hyungwook Lee

Comment 2 2015-02-05 23:57:41 PST

Created attachment 246151 [details] patch

Build Bot

Comment 3 2015-02-06 00:42:12 PST

Comment on attachment 246151 [details] patch Attachment 246151 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4554783247564800 New failing tests: editing/selection/block-cursor-overtype-mode.html

Build Bot

Comment 4 2015-02-06 00:42:16 PST

Created attachment 246154 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5

alan

Comment 5 2015-02-07 13:27:45 PST

Comment on attachment 246151 [details] patch Please add a layout test.

Hyungwook Lee

Comment 6 2015-02-09 05:59:49 PST

Created attachment 246267 [details] patch

Build Bot

Comment 7 2015-02-09 06:34:04 PST

Comment on attachment 246267 [details] patch Attachment 246267 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5436924701442048 New failing tests: editing/execCommand/crash-140261.html

Build Bot

Comment 8 2015-02-09 06:34:10 PST

Created attachment 246268 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Build Bot

Comment 9 2015-02-09 06:45:28 PST

Comment on attachment 246267 [details] patch Attachment 246267 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6203427113664512 New failing tests: editing/execCommand/crash-140261.html editing/selection/block-cursor-overtype-mode.html

Build Bot

Comment 10 2015-02-09 06:45:33 PST

Created attachment 246269 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5

Darin Adler

Comment 11 2015-02-09 09:24:57 PST

Comment on attachment 246267 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=246267&action=review Need change log for the layout tests too, not just the code change. > LayoutTests/editing/execCommand/crash-140261-expected.txt:3 > +Test for bug Crash in WebCore::LogicalSelectionOffsetCaches::ContainingBlockInfo::setBlock(). > + > +This test passes if it doesn't crash. None of this text appears in the test case; that’s why this test is failing on both bots that run tests (the Mac bots). The expected file has to match the output of the test.

Hyungwook Lee

Comment 12 2015-02-10 06:29:56 PST

Created attachment 246320 [details] patch

Build Bot

Comment 13 2015-02-10 07:15:19 PST

Comment on attachment 246320 [details] patch Attachment 246320 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5775258602700800 New failing tests: fast/repaint/selection-gap-transformed-fixed-child.html fast/repaint/selection-gap-fixed-child.html fast/repaint/selection-gap-flipped-absolute-child.html fast/repaint/selection-gap-flipped-fixed-child.html fast/repaint/selection-gap-absolute-child.html fast/repaint/selection-gap-transformed-absolute-child.html

Build Bot

Comment 14 2015-02-10 07:15:23 PST

Created attachment 246322 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5

Darin Adler

Comment 15 2015-02-10 23:22:37 PST

Comment on attachment 246320 [details] patch Any ideas why the layout tests started failing on the Mac EWS bot?

Hyungwook Lee

Comment 16 2015-02-11 00:37:29 PST

I'm trying to make another patch that will not break Mac EWS bot. ;;

alan

Comment 17 2015-02-11 09:25:31 PST

Comment on attachment 246320 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=246320&action=review > Source/WebCore/rendering/RenderBlock.cpp:1749 > + if (!shouldPaintSelectionGaps() || !repaintContainer) Judging by the stacktrace, I suspect the issue here is that we are in the detached tree state and the renderer (the <textarea> that we try to clear the selection from) has no block ancestor in this detached subtree. Assume the <textarea> itself is the repaint container (so we have a valid reapintContainer), we'd still crash when we try to dereference its (non-existing) containing block. (see containingBlockForFixedPosition/containingBlockForAbsolutePosition/containingBlockForObjectInFlow)

Hyungwook Lee

Comment 18 2015-02-13 01:54:12 PST

Created attachment 246514 [details] patch Yes, There is no <textarea>'s containing block anymore that caused by already detached from the renderer tree. I made the patch that makes check its containing block before using it.

Hyungwook Lee

Comment 19 2015-02-22 18:19:34 PST

Please take a look. :)

Ryosuke Niwa

Comment 20 2015-02-22 18:41:03 PST

Comment on attachment 246514 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=246514&action=review > Source/WebCore/rendering/RenderView.cpp:962 > - && os->selectionState() != SelectionNone) { > + && os->selectionState() != SelectionNone && os->containingBlock()) { It seems a little fragile to check for this condition here. e.g. RenderView::applySubtreeSelection has a similar check although that one probably won't be called with a null containingBlock. Perhaps we can extract a helper inline function?

Hyungwook Lee

Comment 21 2015-03-01 07:21:41 PST

Created attachment 247626 [details] patch

Hyungwook Lee

Comment 22 2015-03-01 07:22:39 PST

Created attachment 247627 [details] patch

Hyungwook Lee

Comment 23 2015-03-05 03:48:14 PST

PTAL.

Gyuyoung Kim

Comment 24 2015-04-27 18:29:41 PDT

Comment on attachment 247627 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=247627&action=review > Source/WebCore/ChangeLog:4 > + WebCore::LogicalSelectionOffsetCaches::ContainingBlockInfo::setBlock(). Unnecessary line break. > Source/WebCore/ChangeLog:9 > + We need to check its Containing Block that can be null. How about below patch description ? "We need to check whether RenderObject is valid in RenderView::fooSubtreeSelection functions because invalid object has caused a crash. This patch adds isValidObjectForNewSelection(), and use it." > LayoutTests/ChangeLog:4 > + WebCore::LogicalSelectionOffsetCaches::ContainingBlockInfo::setBlock(). ditto. > LayoutTests/editing/execCommand/crash-140261.html:13 > +if (window.testRunner) Need to have an indentation (4 spaces).

Gyuyoung Kim

Comment 25 2015-04-27 18:30:39 PDT

CC'ing Darin. I think this patch will be better to be reviewed by Darin or Rniwa.

Hyungwook Lee

Comment 26 2015-04-28 05:54:26 PDT

Created attachment 251841 [details] patch Applying Gyuyoung Kim's feedback.

Gyuyoung Kim

Comment 27 2015-04-28 06:41:11 PDT

Comment on attachment 251841 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=251841&action=review > Source/WebCore/ChangeLog:3 > + Fix crash in WebCore::LogicalSelectionOffsetCaches::ContainingBlockInfo::setBlock(). Small nit: Bug title is slightly differ from each ChangeLog.

Darin Adler

Comment 28 2015-04-28 08:58:31 PDT

Comment on attachment 251841 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=251841&action=review The bug fix looks OK. But refactoring this code into a separate function does not require touching the RenderView.h header at all! review- because of that. > Source/WebCore/rendering/RenderView.h:306 > + ALWAYS_INLINE bool isValidObjectForNewSelection(const SelectionSubtreeRoot& root, const RenderObject* obj) const Why ALWAYS_INLINE? That seems like overkill. Please don’t use the abbreviation “obj” in new code. I suggest “renderer” or “object” as a possible name. Please use a reference for the object rather than a pointer since it can’t be null. Please put the body of this private function in the .cpp file rather than in the header. You can put inline functions in the .cpp file as long as they aren’t used in any other translation unit, and that will be true for a private function. This function does’t seem to use any data members of RenderView so it can just be a separate function in the .cpp file. Doesn’t need to be a member at all and if it is a member should be a static member, not a const member. static inline bool isValidObjectForNewSelection(const SelectionSubtreeRoot& root, const RenderObject& object) { } Inside the .cpp file.

Hyungwook Lee

Comment 29 2015-04-28 21:52:41 PDT

Created attachment 251925 [details] patch Applying reviewer's feedback.

WebKit Commit Bot

Comment 30 2015-04-29 01:24:27 PDT

Comment on attachment 251925 [details] patch Clearing flags on attachment: 251925 Committed r183538: <http://trac.webkit.org/changeset/183538>

WebKit Commit Bot

Comment 31 2015-04-29 01:24:36 PDT

All reviewed patches have been landed. Closing bug.

David Kilzer (:ddkilzer)

Comment 32 2015-05-02 20:26:54 PDT

<rdar://problem/16845960>

Note You need to log in before you can comment on or make changes to this bug.