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
<rdar://problem/63679095>
Created attachment 400345 [details] Patch
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?
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
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
guowei_yang@apple.com 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.
Committed r262813: <https://trac.webkit.org/changeset/262813> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401463 [details].
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.