Bug 206621

Created attachment 388479 [details] Patch

Created attachment 388487 [details] Patch

Created attachment 388488 [details] Patch

Created attachment 388489 [details] Patch

Created attachment 388550 [details] Patch

Created attachment 388556 [details] Patch

Created attachment 388566 [details] Patch

Windows was crashing when running the layout test ast/canvas/canvas-toDataURL-jpeg-crash.html PROBLEM_CLASSES: ID: [0n313] Type: [@ACCESS_VIOLATION] Class: Addendum Scope: BUCKET_ID Name: Omit Data: Omit PID: [Unspecified] TID: [0xd48] Frame: [0] : VCRUNTIME140!memcpy ID: [0n285] Type: [INVALID_POINTER_READ] Class: Primary Scope: DEFAULT_BUCKET_ID (Failure Bucket ID prefix) BUCKET_ID Name: Add Data: Omit PID: [Unspecified] TID: [0xd48] Frame: [0] : VCRUNTIME140!memcpy LAST_CONTROL_TRANSFER: from 00007ffa3bb08474 to 00007ffa493c1480 STACK_TEXT: 000000e7`772fb438 00007ffa`3bb08474 : 00000000`00001f40 000000e7`772fb570 00000000`00001f40 00000000`00000000 : VCRUNTIME140!memcpy+0x190 000000e7`772fb440 00007ffa`3bcee330 : 000002c2`fe682670 000000e7`772fb570 000002c2`ff1e66a0 000000e7`772fc90e : CoreGraphics!CGAccessSessionGetBytes+0xa4 000000e7`772fb470 00007ffa`3bca5184 : 000002c2`ff216fe0 000002c2`ba1a6d50 000000e7`772fcb88 000002c2`ff210a60 : CoreGraphics!CGImageCreateCopyWithAlphaInfo+0x462f0 000000e7`772fc970 00007ffa`14ad5038 : 000002c2`00000001 000002c2`ff210a60 000002c2`ba1a6d50 000002c2`00000008 : CoreGraphics!CGImageDestinationFinalize+0xc4 000000e7`772fc9a0 00007ffa`1434d262 : 000002c2`ff1e66a0 000000e7`00000000 00000000`00f42400 000002c2`fe6825e0 : WebKit!WebCore::encodeImage+0x138 000000e7`772fca20 00007ffa`1434d3c8 : 000002c2`ba187760 000000e7`772fcba8 000002c2`fe660580 00007ffa`146d4b5f : WebKit!WebCore::ImageBuffer::toCFData+0x4c2 000000e7`772fcb20 00007ffa`143c61fd : 000002c2`fe660580 00000000`00000000 00000000`00000000 00000000`0000000a : WebKit!WebCore::ImageBuffer::toDataURL+0x48 000000e7`772fcb70 00007ffa`1441b22d : 00000000`00000001 000002c2`ff354000 000000e7`772fcc90 00007ffa`2982b357 : WebKit!WebCore::HTMLCanvasElement::toDataURL+0x15d 000000e7`772fcc10 00007ffa`1441b0c3 : 000002c2`fdd90080 000002c2`fe68da08 000000e7`772fcc70 000002c2`bd9b11be : WebKit!WebCore::jsHTMLCanvasElementPrototypeFunctionToDataURLBody+0x14d 000000e7`772fcca0 000002c2`bd9b11be : 000000e7`772fcd90 00007ffa`29b20cfc 000002c2`ff354000 000002c2`ff2421c0 : WebKit!WebCore::jsHTMLCanvasElementPrototypeFunctionToDataURL+0x63 This was because I moved the declaration: RefPtr<Uint8ClampedArray> premultipliedData; inside the if statement: if (CFEqual(uti.get(), jpegUTI())) { RefPtr<Uint8ClampedArray> premultipliedData; } This caused the Uint8ClampedArray to be freed before calling encodeImage().

Comment on attachment 388566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388566&action=review I think this is OK but the two apparent behavior changes worry me. Can we undo those parts? > Source/WebCore/bindings/js/SerializedScriptValue.cpp:2788 > + auto imageData = ImageData::create(IntSize(logicalWidth, logicalHeight), makeRef(*array)); Is this a data copy? If not, can it WTFMove the array? > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:442 > + protectedPixelArray = makeRefPtr(imageData->data()); This is not a copy, right? > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:451 > + image = adoptCF(CGImageCreate(pixelArrayDimensions.width(), pixelArrayDimensions.height(), 8, 32, 4 * pixelArrayDimensions.width(), sRGBColorSpaceRef(), kCGBitmapByteOrderDefault | kCGImageAlphaNoneSkipLast, dataProvider.get(), 0, false, kCGRenderingIntentDefault)); This seems like a behavior change, because the returned CGImage used to be sized based on logical size, but now it's sized based on internal size. > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:133 > + auto* result = imageData ? imageData->data() : nullptr; > + if (!result) We use "result" as the value returned by the function, but that's not the case here. > Source/WebCore/platform/graphics/filters/FEDropShadow.cpp:102 > + IntRect shadowArea(IntPoint(), resultImage->logicalSize()); This seems like a behavior change too. Did you test SVG blur filters on Retina? > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:522 > + return m_unmultipliedImageResult->data(); I don't like that we pass raw pointers to these buffers around, with undetermined lifetimes.

Created attachment 388884 [details] Patch

Comment on attachment 388566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388566&action=review >> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2788 >> + auto imageData = ImageData::create(IntSize(logicalWidth, logicalHeight), makeRef(*array)); > > Is this a data copy? If not, can it WTFMove the array? No it is not a data copy. So I used array.releaseNonNull() since array is a RefPtr<Uint8ClampedArray> and ImageData::create() expects Ref<Uint8ClampedArray>&&. >> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:442 >> + protectedPixelArray = makeRefPtr(imageData->data()); > > This is not a copy, right? No it is a copy. In a previous comment the purpose of this makeRef() is to protect the Uint8ClampedArray till encodeImage() is called at the end of this function. >> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:451 >> + image = adoptCF(CGImageCreate(pixelArrayDimensions.width(), pixelArrayDimensions.height(), 8, 32, 4 * pixelArrayDimensions.width(), sRGBColorSpaceRef(), kCGBitmapByteOrderDefault | kCGImageAlphaNoneSkipLast, dataProvider.get(), 0, false, kCGRenderingIntentDefault)); > > This seems like a behavior change, because the returned CGImage used to be sized based on logical size, but now it's sized based on internal size. I undid this part. But it is not actually a behavior change. This function is called only from toDataURL() and toData() which are called only from the Canvas code. And the Canvas code creates its ImageBuffer with resolutionScale = 1. This means m_logicalSize == m_size == m_data.backingStoreSize == imageData->size(). The resolutionScale may not be 1 only in the filter code which will never call ImageBuffer::toCFData(). >> Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:133 >> + if (!result) > > We use "result" as the value returned by the function, but that's not the case here. Fixed. >> Source/WebCore/platform/graphics/filters/FEDropShadow.cpp:102 >> + IntRect shadowArea(IntPoint(), resultImage->logicalSize()); > > This seems like a behavior change too. Did you test SVG blur filters on Retina? This is the only place in the filters code that uses BackingStoreCoordinateSystem. FEColorMatrix::platformApplySoftware() for example uses logical coordinates. So I think this code was unintentionally using backing store coordinates although logical coordinates could just work. >> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:522 >> + return m_unmultipliedImageResult->data(); > > I don't like that we pass raw pointers to these buffers around, with undetermined lifetimes. I checked the code and I found these functions are protected and are used internally by the derived classes. The returned pointers are never stored, they are just used for processing the data.

Created attachment 388890 [details] Patch

Created attachment 388931 [details] Patch

<rdar://problem/59438954>

(In reply to Said Abou-Hallawa from comment #11) > Comment on attachment 388566 [details] > Patch > >> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:522 > >> + return m_unmultipliedImageResult->data(); > > > > I don't like that we pass raw pointers to these buffers around, with undetermined lifetimes. > > I checked the code and I found these functions are protected and are used > internally by the derived classes. The returned pointers are never stored, > they are just used for processing the data. It's still not a *great* pattern.

Created attachment 390978 [details] Patch

Comment on attachment 390978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390978&action=review > Source/WebCore/platform/graphics/ImageBuffer.h:118 > + void draw(GraphicsContext&, const FloatRect& destRect, const FloatRect& srcRect = FloatRect(0, 0, -1, -1), const ImagePaintingOptions& = { }); What is the magic 0, 0, -1, -1? Can we pass on Optional<FloatRect> instead? Not new in this patch; can fix later > Source/WebCore/platform/graphics/ImageBuffer.h:125 > + static void drawConsuming(std::unique_ptr<ImageBuffer>, GraphicsContext&, const FloatRect& destRect, const FloatRect& srcRect = FloatRect(0, 0, -1, -1), const ImagePaintingOptions& = { }); Ditto.

Comment on attachment 390978 [details] Patch Clearing flags on attachment: 390978 Committed r256822: <https://trac.webkit.org/changeset/256822>

All reviewed patches have been landed. Closing bug.