Bug 212415 – WebKit Crashes when SVG Filter Logging is Turned On

• Component: SVG
• Status: RESOLVED FIXED

WebKit crashes when the flag -WebCoreLogging "Filters" is present, which turns on the logging for SVG Filters.
This is due to uncaught null pointers in 

FilterEffect::imageBufferResult(), 
FilterEffect::copyUnmultipliedResult() and 
FilterEffect::copyPremultipliedResult()

Step to reproduce: 
1. Turn on filter logging by adding the flag -WebCoreLogging "Filters" in build scheme
2. Launch MiniBrowser and load an html file with SVG

Result: 
MiniBrowser crashes

[Radar WebKit Bug Importer]

<rdar://problem/63679095>

Created attachment 400345 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 400345 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

Need some explanation.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Need justification for why there are no tests

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:272
> +        LOG_WITH_STREAM(Filters, stream << " m_premultipliedImageResult " << m_premultipliedImageResult->data());

I think this will create new lines, which probably isn't the intention. "FilterEffect" looks like it was an intentional prefix, to aid for things like grepping through logs.

How about using StringBuilder to reduce this giant line into something more readable? Perhaps putting this stuff in its own function, too?

[Simon Fraser (smfr)]

Comment on attachment 400345 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:269
> -    LOG_WITH_STREAM(Filters, stream << "FilterEffect " << filterName() << " " << this << " imageBufferResult(). Existing image buffer " << m_imageBufferResult.get() <<  " m_premultipliedImageResult " << m_premultipliedImageResult->data() << " m_unmultipliedImageResult " << m_unmultipliedImageResult->data());
> +    LOG_WITH_STREAM(Filters, stream << "FilterEffect " << filterName() << " " << this << " imageBufferResult(). Existing image buffer " << m_imageBufferResult.get());

You want to do:
LOG_WITH_STREAM(Filters, stream << "FilterEffect " << filterName() << " " << this << " imageBufferResult(). Existing image buffer " << m_imageBufferResult.get() << " m_premultipliedImageResult " << (m_premultipliedImageResult  ? m_premultipliedImageResult->data() : nullptr) << " m_unmultipliedImageResult " << (m_unmultipliedImageResult  ? m_unmultipliedImageResult->data() : nullptr));

Created attachment 401045 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 401045 [details]
Patch

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

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:284
> +    StringBuilder builder;
> +    StringPrintStream logStream;
> +    logStream.printf("FilterEffect %s %p %s(). Existing image buffer %p", filterName(), this, resultType, m_imageBufferResult.get());
> +    
> +    if (m_premultipliedImageResult)
> +        logStream.printf(" m_premultipliedImageResult %p ", m_premultipliedImageResult->data());
> +    else
> +        logStream.printf(" m_premultipliedImageResult %p", nullptr);
> +        
> +    if (m_unmultipliedImageResult)
> +        logStream.printf(" m_unmultipliedImageResult %p", m_unmultipliedImageResult->data());
> +    else
> +        logStream.printf(" m_unmultipliedImageResult %p", nullptr);
> +    builder.append(logStream.toString());
> +    
> +    return builder.toString();

This seems worse than the original. 

Implement TextStream& operator<<(TextStream&, const ImageBuffer&)

then use ValueOrNull() in the logging.

Created attachment 401456 [details]
Patch

Created attachment 401463 [details]
Patch

Created attachment 401474 [details]
Patch - updated reviewer name

[EWS]

[e-mail] does not have committer permissions according to https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 401474 [details] from commit queue.

[EWS]

Committed r262813: <https://trac.webkit.org/changeset/262813>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401463 [details].

[Simon Fraser (smfr)]

Comment on attachment 401463 [details]
Patch

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

> Source/WebCore/html/ImageData.cpp:122
> +    // Print out the address of the pixel data array
> +    return ts << imageData.data();

This comment can be removed.

This should also output the size.