Bug 158241 - Crash under VisibleSelection::firstRange()
Summary: Crash under VisibleSelection::firstRange()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2016-05-31 15:41 PDT by Chris Dumez
Modified: 2016-06-09 09:47 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.72 KB, patch)
2016-05-31 15:46 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Fixes the bug (7.57 KB, patch)
2016-06-01 23:16 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (805.54 KB, application/zip)
2016-06-02 00:07 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (793.16 KB, application/zip)
2016-06-02 00:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.56 MB, application/zip)
2016-06-02 00:24 PDT, Build Bot
no flags Details
Fixed tests (10.31 KB, patch)
2016-06-02 01:50 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
More null checks (11.19 KB, patch)
2016-06-02 02:02 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-05-31 15:41:11 PDT
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.
Comment 1 Chris Dumez 2016-05-31 15:41:32 PDT
<rdar://problem/18312053>
Comment 2 Chris Dumez 2016-05-31 15:46:05 PDT
Created attachment 280192 [details]
Patch
Comment 3 Ryosuke Niwa 2016-05-31 15:50:45 PDT
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.
Comment 4 Ryosuke Niwa 2016-06-01 23:16:18 PDT
Created attachment 280310 [details]
Fixes the bug
Comment 5 Build Bot 2016-06-02 00:07:03 PDT
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
Comment 6 Build Bot 2016-06-02 00:07:08 PDT
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
Comment 7 Build Bot 2016-06-02 00:10:13 PDT
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
Comment 8 Build Bot 2016-06-02 00:10:18 PDT
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
Comment 9 Build Bot 2016-06-02 00:23:57 PDT
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
Comment 10 Build Bot 2016-06-02 00:24:03 PDT
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
Comment 11 Ryosuke Niwa 2016-06-02 01:50:09 PDT
Created attachment 280319 [details]
Fixed tests
Comment 12 WebKit Commit Bot 2016-06-02 01:51:51 PDT
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.
Comment 13 Ryosuke Niwa 2016-06-02 02:02:58 PDT
Created attachment 280322 [details]
More null checks
Comment 14 WebKit Commit Bot 2016-06-02 02:05:24 PDT
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.
Comment 15 Chris Dumez 2016-06-02 07:31:53 PDT
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?
Comment 16 Ryosuke Niwa 2016-06-02 11:52:22 PDT
(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.
Comment 17 Enrica Casucci 2016-06-03 15:32:00 PDT
Comment on attachment 280322 [details]
More null checks

Looks good to me.
Comment 18 WebKit Commit Bot 2016-06-03 16:00:28 PDT
Comment on attachment 280322 [details]
More null checks

Clearing flags on attachment: 280322

Committed r201667: <http://trac.webkit.org/changeset/201667>
Comment 19 WebKit Commit Bot 2016-06-03 16:00:35 PDT
All reviewed patches have been landed.  Closing bug.