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

Bug 140261 - Null ptr crash in WebCore::LogicalSelectionOffsetCaches::ContainingBlockInfo::setBlock

Summary: Null ptr crash in WebCore::LogicalSelectionOffsetCaches::ContainingBlockInfo:...

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Layout and Rendering (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Hyungwook Lee

URL:
Keywords:	InRadar

Depends on:
Blocks:	116980
	Show dependency tree / graph

Reported:	2015-01-08 11:26 PST by Renata Hodovan
Modified:	2015-05-02 20:26 PDT (History)
CC List:	11 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

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

Description Renata Hodovan 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

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

I've start to look at this issue.

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

Created attachment 246151 [details]
patch

Comment 3 Build Bot 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

Comment 4 Build Bot 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

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

Comment on attachment 246151 [details]
patch

Please add a layout test.

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

Created attachment 246267 [details]
patch

Comment 7 Build Bot 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

Comment 8 Build Bot 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

Comment 9 Build Bot 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

Comment 10 Build Bot 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

Comment 11 Darin Adler 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.

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

Created attachment 246320 [details]
patch

Comment 13 Build Bot 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

Comment 14 Build Bot 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

Comment 15 Darin Adler 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?

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

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

Comment 17 zalan 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)

Comment 18 Hyungwook Lee 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.

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

Please take a look. :)

Comment 20 Ryosuke Niwa 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?

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

Created attachment 247626 [details]
patch

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

Created attachment 247627 [details]
patch

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

PTAL.

Comment 24 Gyuyoung Kim 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).

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

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

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

Created attachment 251841 [details]
patch

Applying Gyuyoung Kim's feedback.

Comment 27 Gyuyoung Kim 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.

Comment 28 Darin Adler 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.

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

Created attachment 251925 [details]
patch

Applying reviewer's feedback.

Comment 30 WebKit Commit Bot 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>

Comment 31 WebKit Commit Bot 2015-04-29 01:24:36 PDT

All reviewed patches have been landed.  Closing bug.

Comment 32 David Kilzer (:ddkilzer) 2015-05-02 20:26:54 PDT

<rdar://problem/16845960>