Bug 158241 – Crash under VisibleSelection::firstRange()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Crash under VisibleSelection::firstRange():
Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x0000000000000020)
[  0] 0x00007fff966100e6 WebCore`WebCore::VisibleSelection::firstRange() const [inlined] WebCore::Node::treeScope() const at Node.h:402
       398 	
       399 	    TreeScope& treeScope() const
       400 	    {
       401 	        ASSERT(m_treeScope);
    -> 402 	        return *m_treeScope;
       403 	    }
       404 	    static ptrdiff_t treeScopeMemoryOffset() { return OBJECT_OFFSETOF(Node, m_treeScope); }
       405 	
       406 	    // Returns true if this node is associated with a document and is in its associated document's
    

     0x00007fff966100d7:     movq %r12, %rdi
     0x00007fff966100da:     movq %rbx, %rsi
     0x00007fff966100dd:    callq 0x26d810             ; WebCore::Position::parentAnchoredEquivalent at Position.cpp:217
     0x00007fff966100e2:     movq -0x30(%rbp), %rax
 ->  0x00007fff966100e6:     movq 0x20(%rax), %rax
     0x00007fff966100ea:     movq 0x8(%rax), %rsi
     0x00007fff966100ee:     movq %r14, %rdi
     0x00007fff966100f1:     movq %r15, %rdx
     0x00007fff966100f4:     movq %r12, %rcx

[  0] 0x00007fff966100e6 WebCore`WebCore::VisibleSelection::firstRange() const [inlined] WebCore::Position::anchorNode() const + 4 at Node.h:396
       392 	    // A Document node returns itself.
       393 	    Document& document() const
       394 	    {
       395 	        ASSERT(this);
    -> 396 	        return treeScope().documentScope();
       397 	    }
       398 	
       399 	    TreeScope& treeScope() const
       400 	    {
    
[  0] 0x00007fff966100e2 WebCore`WebCore::VisibleSelection::firstRange() const + 66 at VisibleSelection.cpp:132
       128 	    if (isNone())
       129 	        return 0;
       130 	    Position start = m_start.parentAnchoredEquivalent();
       131 	    Position end = m_end.parentAnchoredEquivalent();
    -> 132 	    return Range::create(start.anchorNode()->document(), start, end);
       133 	}
       134 	
       135 	PassRefPtr<Range> VisibleSelection::toNormalizedRange() const
       136 	{
    
[  1] 0x00007fff8e89f2cb WebKit`WebKit::ServicesOverlayController::buildSelectionHighlight() + 79 at ServicesOverlayController.mm:518
       514 	
       515 	    Vector<CGRect> cgRects;
       516 	    cgRects.reserveCapacity(m_currentSelectionRects.size());
       517 	
    -> 518 	    RefPtr<Range> selectionRange = m_webPage.corePage()->selection().firstRange();
       519 	    if (selectionRange) {
       520 	        Frame* mainFrame = m_webPage.mainFrame();
       521 	        FrameView& mainFrameView = *mainFrame->view();
       522 	        FrameView* viewForRange = selectionRange->ownerDocument().view();
    
[  2] 0x00007fff8e89f1e1 WebKit`WebKit::ServicesOverlayController::selectionRectsDidChange(WTF::Vector<WebCore::LayoutRect, 0ul, WTF::CrashOnOverflow> const&, WTF::Vector<WebCore::GapRects, 0ul, WTF::CrashOnOverflow> const&, bool) + 1703 at ServicesOverlayController.mm:392

I hit this when visiting the Performance Dashboard.

[Chris Dumez]

<rdar://problem/18312053>

[Chris Dumez]

Created attachment 280192 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 280192 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280192&action=review

> Source/WebCore/editing/VisibleSelection.cpp:135
> +
> +    if (start.isNull() || end.isNull())
> +        return nullptr;
> +

Actually, we should never hit this case. The problem stems from not allowing shadow root to be anchored in selection end points.
Sorry, I keep forgetting to fix this bug.  Will try to upload a fix tonight.

[Ryosuke Niwa]

Created attachment 280310 [details]
Fixes the bug

[Build Bot]

Comment on attachment 280310 [details]
Fixes the bug

Attachment 280310 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1421696

New failing tests:
editing/mac/dictionary-lookup/dictionary-lookup-input.html

[Build Bot]

Created attachment 280311 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 280310 [details]
Fixes the bug

Attachment 280310 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1421700

New failing tests:
editing/mac/dictionary-lookup/dictionary-lookup-input.html

[Build Bot]

Created attachment 280312 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 280310 [details]
Fixes the bug

Attachment 280310 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1421714

New failing tests:
accessibility/mac/text-marker-word-nav.html
accessibility/mac/text-marker-paragraph-nav.html
editing/mac/dictionary-lookup/dictionary-lookup-input.html

[Build Bot]

Created attachment 280313 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Ryosuke Niwa]

Created attachment 280319 [details]
Fixed tests

[WebKit Commit Bot]

Attachment 280319 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/VisiblePosition.cpp:590:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 280322 [details]
More null checks

[WebKit Commit Bot]

Attachment 280322 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/VisiblePosition.cpp:590:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 280322 [details]
More null checks

View in context: https://bugs.webkit.org/attachment.cgi?id=280322&action=review

> Source/WebCore/editing/VisibleSelection.cpp:132
> +    if (start.isNull() || end.isNull())

Did you mean to keep my change in there? I thought this should not happen?

[Ryosuke Niwa]

(In reply to comment #15)
> Comment on attachment 280322 [details]
> More null checks
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280322&action=review
> 
> > Source/WebCore/editing/VisibleSelection.cpp:132
> > +    if (start.isNull() || end.isNull())
> 
> Did you mean to keep my change in there? I thought this should not happen?

Yeah, I thought this crash was only reproducible with shadow roots but it turned out this crash has been reported since 2014 so I'm adding this as a safe guard.

[Enrica Casucci]

Comment on attachment 280322 [details]
More null checks

Looks good to me.

[WebKit Commit Bot]

Comment on attachment 280322 [details]
More null checks

Clearing flags on attachment: 280322

Committed r201667: <http://trac.webkit.org/changeset/201667>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.