Bug 156880

Summary: Crash under WebCore::DataDetection::detectContentInRange()
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, enrica, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2016-04-21 16:35:53 PDT 
Crash under WebCore::DataDetection::detectContentInRange():
Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Subtype: KERN_INVALID_ADDRESS at 0x0000000000000014
Thread[0]
[  0] 0x00000001931ca83c WebCore`WebCore::Node::insertBefore(WTF::PassRefPtr<WebCore::Node>, WebCore::Node*, int&) + 24 at Node.cpp:406:9
[  1] 0x00000001929ff7b3 WebCore`WebCore::DataDetection::detectContentInRange(WTF::RefPtr<WebCore::Range>&, WebCore::DataDetectorTypes) + 9171 at DataDetection.mm:603:13
[  2] 0x00000001929ff7b3 WebCore`WebCore::DataDetection::detectContentInRange(WTF::RefPtr<WebCore::Range>&, WebCore::DataDetectorTypes) + 9171 at DataDetection.mm:603:13
[  3] 0x00000001926bdc53 WebCore`WebCore::FrameLoader::checkLoadCompleteForThisFrame() + 1347 at FrameLoader.cpp:2293:53
[  4] 0x00000001926bd5e7 WebCore`WebCore::FrameLoader::checkLoadComplete() + 371 at FrameLoader.cpp:2485:13
Comment 1 Chris Dumez 2016-04-21 16:36:13 PDT 
rdar://problem/25622631
Comment 2 Chris Dumez 2016-04-21 16:44:24 PDT 
Created attachment 276982 [details]
Patch
Comment 3 Darin Adler 2016-04-21 18:41:59 PDT 
Comment on attachment 276982 [details]
Patch

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

> Source/WebCore/editing/cocoa/DataDetection.mm:567
> +        Vector<RefPtr<Range>>& resultRanges = allResultRanges[resultIndex];

I would like auto& better.

> Source/WebCore/editing/cocoa/DataDetection.mm:581
> +            rangeBoundaries.uncheckedAppend(std::make_pair(range->startPosition(), range->endPosition()));

Could use braces instead of make_pair?

    rangeBoundaries.uncheckedAppend({ range->startPosition(), range->endPosition() });

> Source/WebCore/editing/cocoa/DataDetection.mm:642
> +            else if (is<Element>(*parentNode)) {
> +                if (RefPtr<Attr> color = downcast<Element>(*parentNode).getAttributeNode("color"))
> +                    anchorElement->setAttribute(styleAttr, color->style()->cssText());
>              }

An Attr node is a really inefficient way to get the CSS color string that depends on a deprecated feature. In fact, Attr::style specifically says "This function only exists to support the Obj-C bindings."

I think I had a patch fixing this, but didn’t land it.

What we want is something more like this:

    if (is<StyledElement>(*parentNode)) {
        if (auto* style = downcast<StyledElement>(*parentNode).presentationAttributeStyle()) {
            String color = style->getPropertyValue(CSSPropertyIDColor);
            if (!color.isEmpty())
                anchorElement->setInlineStyleProperty(CSSPropertyIDColor, color);
        }
    }
Comment 4 Chris Dumez 2016-04-22 13:31:06 PDT 
Created attachment 277092 [details]
Patch
Comment 5 Chris Dumez 2016-04-22 13:31:42 PDT 
Comment on attachment 277092 [details]
Patch

Confirmed locally that the feature is still working.
Comment 6 Chris Dumez 2016-04-22 13:40:15 PDT 
Forgot about some of the review comments.
Comment 7 Chris Dumez 2016-04-22 13:42:00 PDT 
Created attachment 277094 [details]
Patch
Comment 8 WebKit Commit Bot 2016-04-22 14:32:04 PDT 
Comment on attachment 277094 [details]
Patch

Clearing flags on attachment: 277094

Committed r199910: <http://trac.webkit.org/changeset/199910>
Comment 9 WebKit Commit Bot 2016-04-22 14:32:09 PDT 
All reviewed patches have been landed.  Closing bug.