Crash under WebCore::DataDetection::detectContentInRange()

[Chris Dumez]

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

[Chris Dumez]

rdar://problem/25622631

[Chris Dumez]

Created attachment 276982 [details]
Patch

[Darin Adler]

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);
        }
    }

[Chris Dumez]

Created attachment 277092 [details]
Patch

[Chris Dumez]

Comment on attachment 277092 [details]
Patch

Confirmed locally that the feature is still working.

[Chris Dumez]

Forgot about some of the review comments.

[Chris Dumez]

Created attachment 277094 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 277094 [details]
Patch

Clearing flags on attachment: 277094

Committed r199910: <http://trac.webkit.org/changeset/199910>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.