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: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||
Bug Blocks: | 116980 | ||||||||||||||||||||||||||||||
Attachments: |
|
I've start to look at this issue. Created attachment 246151 [details]
patch
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 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
Comment on attachment 246151 [details]
patch
Please add a layout test.
Created attachment 246267 [details]
patch
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 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
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 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
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. Created attachment 246320 [details]
patch
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 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
Comment on attachment 246320 [details]
patch
Any ideas why the layout tests started failing on the Mac EWS bot?
I'm trying to make another patch that will not break Mac EWS bot. ;; 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) 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.
Please take a look. :) 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? Created attachment 247626 [details]
patch
Created attachment 247627 [details]
patch
PTAL. 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). CC'ing Darin. I think this patch will be better to be reviewed by Darin or Rniwa. Created attachment 251841 [details]
patch
Applying Gyuyoung Kim's feedback.
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. 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. Created attachment 251925 [details]
patch
Applying reviewer's feedback.
Comment on attachment 251925 [details] patch Clearing flags on attachment: 251925 Committed r183538: <http://trac.webkit.org/changeset/183538> All reviewed patches have been landed. Closing bug. |
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