83416 2012-04-06 19:56:23 -0700 FrameData constructor zeroes all fields, causing ImageOrientation to be 0 2012-04-09 14:47:25 -0700 1 1 1 Unclassified WebKit Images 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 thorton thorton ap ggaren simon.fraser webkit-bug-importer zimmermann oldest_to_newest 597909 0 thorton 2012-04-06 19:56:23 -0700 From BitmapImage.h: // FIXME: This declaration gives FrameData a default constructor that zeroes // all its data members, even though FrameData's default constructor defined // below does not zero all its data members. One of these must be wrong! * platform/graphics/BitmapImage.h: Added a FIXME because the current Vector traits for FrameData are logically incorrect, but I couldn't find a place where this currently results in bad behavior, and it's not immediately obvious what the right solution is. This breaks http://trac.webkit.org/changeset/113486 when the setting is enabled, as ImageOrientation should never be 0. I get the impression from ggaren's comment that it's not right to just remove the annotations. 597910 1 webkit-bug-importer 2012-04-06 19:56:41 -0700 <rdar://problem/11205425> 597911 2 thorton 2012-04-06 19:57:18 -0700 (In reply to comment #0) > This breaks http://trac.webkit.org/changeset/113486 when the setting is enabled In *some* cases (svg/as-image tests fare particularly poorly). 597937 3 zimmermann 2012-04-07 00:37:19 -0700 SHOULD NEVER BE REACHED /Users/nzimmermann/Coding/WebKit/Source/WebCore/platform/graphics/ImageOrientation.cpp(56) : WebCore::AffineTransform WebCore::ImageOrientation::transformFromDefault(const WebCore::FloatSize &) const 1 0x106781a89 WebCore::GraphicsContext::drawNativeImage(CGImage*, WebCore::FloatSize const&, WebCore::ColorSpace, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::CompositeOperator, WebCore::ImageOrientation) 2 0x106903860 WebCore::BitmapImage::draw(WebCore::GraphicsContext*, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::ColorSpace, WebCore::CompositeOperator, WebCore::RespectImageOrientationEnum) 3 0x106763623 WebCore::GraphicsContext::drawImage(WebCore::Image*, WebCore::ColorSpace, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::CompositeOperator, WebCore::RespectImageOrientationEnum, bool) 4 0x106763385 WebCore::GraphicsContext::drawImage(WebCore::Image*, WebCore::ColorSpace, WebCore::IntRect const&, WebCore::IntRect const&, WebCore::CompositeOperator, WebCore::RespectImageOrientationEnum, bool) 5 0x1067632d1 WebCore::GraphicsContext::drawImage(WebCore::Image*, WebCore::ColorSpace, WebCore::IntRect const&, WebCore::CompositeOperator, WebCore::RespectImageOrientationEnum, bool) 6 0x10722a15a WebCore::RenderImage::paintIntoRect(WebCore::GraphicsContext*, WebCore::IntRect const&) 7 0x107229959 WebCore::RenderImage::paintReplaced(WebCore::PaintInfo&, WebCore::IntPoint const&) 8 0x1072ad115 WebCore::RenderReplaced::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) 9 0x107229ada WebCore::RenderImage::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) 10 0x10691accc WebCore::InlineBox::paint(WebCore::PaintInfo&, WebCore::IntPoint const&, int, int) 11 0x106921662 WebCore::InlineFlowBox::paint(WebCore::PaintInfo&, WebCore::IntPoint const&, int, int) 12 0x1073d1f08 WebCore::RootInlineBox::paint(WebCore::PaintInfo&, WebCore::IntPoint const&, int, int) 13 0x107270514 WebCore::RenderLineBoxList::paint(WebCore::RenderBoxModelObject*, WebCore::PaintInfo&, WebCore::IntPoint const&) const 14 0x107163e5f WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::IntPoint const&) 15 0x1071648c3 WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::IntPoint const&) 16 0x1071626b8 WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) 17 0x10734146c WebCore::RenderTableCell::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) 18 0x10734a900 WebCore::RenderTableSection::paintCell(WebCore::RenderTableCell*, WebCore::PaintInfo&, WebCore::IntPoint const&) 19 0x10734b6a6 WebCore::RenderTableSection::paintObject(WebCore::PaintInfo&, WebCore::IntPoint const&) 20 0x10734a6a4 WebCore::RenderTableSection::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) 21 0x1073352e9 WebCore::RenderTable::paintObject(WebCore::PaintInfo&, WebCore::IntPoint const&) 22 0x1073350b5 WebCore::RenderTable::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) 23 0x10716427c WebCore::RenderBlock::paintChildren(WebCore::PaintInfo&, WebCore::IntPoint const&) 24 0x107163e75 WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::IntPoint const&) 25 0x1071648c3 WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::IntPoint const&) 26 0x1071626b8 WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) 27 0x10716427c WebCore::RenderBlock::paintChildren(WebCore::PaintInfo&, WebCore::IntPoint const&) 28 0x107163e75 WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::IntPoint const&) 29 0x1071648c3 WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::IntPoint const&) 30 0x1071626b8 WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::IntPoint const&) 31 0x1072462be WebCore::RenderLayer::paintLayerContents(WebCore::RenderLayer*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, WebCore::RenderObject*, WebCore::RenderRegion*, WTF::HashMap<WebCore::OverlapTestRequestClient*, WebCore::IntRect, WTF::PtrHash<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::OverlapTestRequestClient*>, WTF::HashTraits<WebCore::IntRect> >*, unsigned int) I just got this backtrace in trunk. Just running nrwt -p svg/as-image is enough to trigger this here. 597939 4 thorton 2012-04-07 00:40:17 -0700 (In reply to comment #3) > SHOULD NEVER BE REACHED That'd be the one. However, I'm interested as to why you're hitting this code at all? Do you have 113519? If so, the setting should be off (unless you turned it on!?), and drawNativeImage should not be calling transformFromDefault. Still, if we fix this (by ripping out the VectorTraits (which fixes it perfectly), or by doing something else, I'm waiting to hear back from ggaren about what the right thing to do is), that'll go away. 597940 5 thorton 2012-04-07 00:44:31 -0700 (In reply to comment #4) > (In reply to comment #3) > > SHOULD NEVER BE REACHED > > That'd be the one. > > However, I'm interested as to why you're hitting this code at all? Do you have 113519? If so, the setting should be off (unless you turned it on!?), and drawNativeImage should not be calling transformFromDefault. If you do have 113519, I'll take a closer look tomorrow and see why you're getting there. If running the tests is blocking you I'd recommend (for now): --- a/Source/WebCore/platform/graphics/BitmapImage.h +++ b/Source/WebCore/platform/graphics/BitmapImage.h @@ -54,7 +54,7 @@ namespace WTF { // FIXME: This declaration gives FrameData a default constructor that zeroes // all its data members, even though FrameData's default constructor defined // below does not zero all its data members. One of these must be wrong! - template<> struct VectorTraits<WebCore::FrameData> : public SimpleClassVectorTraits { }; + //template<> struct VectorTraits<WebCore::FrameData> : public SimpleClassVectorTraits { }; } namespace WebCore { @@ -66,16 +66,17 @@ template <typename T> class Timer; // ================================================ struct FrameData { - WTF_MAKE_NONCOPYABLE(FrameData); + //WTF_MAKE_NONCOPYABLE(FrameData); public: 597942 6 zimmermann 2012-04-07 00:59:35 -0700 (In reply to comment #5) > If you do have 113519, I'll take a closer look tomorrow and see why you're getting there. If running the tests is blocking you I'd recommend (for now): Yes, I'm on latest trunk revision. I've gdb'ed LayoutTests/svg/as-image/img-preserveAspectRatio-support-1.html: #1 0x0000000101b3ea89 in WebCore::GraphicsContext::drawNativeImage (this=0x7fff5fbfb4f0, imagePtr=0x106e4f4c0, imageSize=@0x7fff5fbf8250, styleColorSpace=WebCore::ColorSpaceDeviceRGB, destRect=@0x7fff5fbf8388, srcRect=@0x7fff5fbf8368, op=WebCore::CompositeSourceOver, orientation={m_orientation = 0}) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:241 241 CGContextConcatCTM(context, orientation.transformFromDefault(adjustedDestRect.size())); (gdb) p orientation $2 = { m_orientation = 0 } Because of the orientation bug in FrameData (nulled): orientation is not equal to the default orientation, and we call transformFromDefault - even if you disabled respectImageOrientation in the Settings. 240 if (orientation != DefaultImageOrientation) { 241 CGContextConcatCTM(context, orientation.transformFromDefault(adjustedDestRect.size())); > --- a/Source/WebCore/platform/graphics/BitmapImage.h > +++ b/Source/WebCore/platform/graphics/BitmapImage.h > @@ -54,7 +54,7 @@ namespace WTF { > // FIXME: This declaration gives FrameData a default constructor that zeroes > // all its data members, even though FrameData's default constructor defined > // below does not zero all its data members. One of these must be wrong! > - template<> struct VectorTraits<WebCore::FrameData> : public SimpleClassVectorTraits { }; > + //template<> struct VectorTraits<WebCore::FrameData> : public SimpleClassVectorTraits { }; > } > > namespace WebCore { > @@ -66,16 +66,17 @@ template <typename T> class Timer; > // ================================================ > > struct FrameData { > - WTF_MAKE_NONCOPYABLE(FrameData); > + //WTF_MAKE_NONCOPYABLE(FrameData); > public: I think this fix is actually correct - if you don't want initialization with memset() you can't use SimpleClassVectorTraits, and thus you can't make FrameData non copyable, as we really have to copy it. Would you mind if I at least land this workaround? 597943 7 thorton 2012-04-07 01:01:34 -0700 (In reply to comment #6) > (In reply to comment #5) > > If you do have 113519, I'll take a closer look tomorrow and see why you're getting there. If running the tests is blocking you I'd recommend (for now): > Yes, I'm on latest trunk revision. > > I've gdb'ed LayoutTests/svg/as-image/img-preserveAspectRatio-support-1.html: > > #1 0x0000000101b3ea89 in WebCore::GraphicsContext::drawNativeImage (this=0x7fff5fbfb4f0, imagePtr=0x106e4f4c0, imageSize=@0x7fff5fbf8250, styleColorSpace=WebCore::ColorSpaceDeviceRGB, destRect=@0x7fff5fbf8388, srcRect=@0x7fff5fbf8368, op=WebCore::CompositeSourceOver, orientation={m_orientation = 0}) at /Users/nzimmermann/Coding/WebKit/Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:241 > 241 CGContextConcatCTM(context, orientation.transformFromDefault(adjustedDestRect.size())); > (gdb) p orientation > $2 = { > m_orientation = 0 > } > > Because of the orientation bug in FrameData (nulled): orientation is not equal to the default orientation, and we call transformFromDefault - even if you disabled respectImageOrientation in the Settings. You made my phone ding so I came back :D Oh, yes, that makes sense. > 240 if (orientation != DefaultImageOrientation) { > 241 CGContextConcatCTM(context, orientation.transformFromDefault(adjustedDestRect.size())); > > > --- a/Source/WebCore/platform/graphics/BitmapImage.h > > +++ b/Source/WebCore/platform/graphics/BitmapImage.h > > @@ -54,7 +54,7 @@ namespace WTF { > > // FIXME: This declaration gives FrameData a default constructor that zeroes > > // all its data members, even though FrameData's default constructor defined > > // below does not zero all its data members. One of these must be wrong! > > - template<> struct VectorTraits<WebCore::FrameData> : public SimpleClassVectorTraits { }; > > + //template<> struct VectorTraits<WebCore::FrameData> : public SimpleClassVectorTraits { }; > > } > > > > namespace WebCore { > > @@ -66,16 +66,17 @@ template <typename T> class Timer; > > // ================================================ > > > > struct FrameData { > > - WTF_MAKE_NONCOPYABLE(FrameData); > > + //WTF_MAKE_NONCOPYABLE(FrameData); > > public: > > I think this fix is actually correct - if you don't want initialization with memset() you can't use SimpleClassVectorTraits, and thus you can't make FrameData non copyable, as we really have to copy it. > > Would you mind if I at least land this workaround? I would *totally* not mind. 597944 8 zimmermann 2012-04-07 01:06:02 -0700 (In reply to comment #7) > I would *totally* not mind. Great, rs=you? Now get some sleep, I'll take care of the rest ;-) 597945 9 thorton 2012-04-07 01:07:07 -0700 (In reply to comment #8) > (In reply to comment #7) > > I would *totally* not mind. > Great, rs=you? Now get some sleep, I'll take care of the rest ;-) (unofficial)rs :D /still no review Sleep! 597946 10 zimmermann 2012-04-07 01:10:05 -0700 (In reply to comment #9) > (unofficial)rs :D /still no review Hopefully you are soon :-) Landed in r113543, closing bug. 598587 11 thorton 2012-04-09 13:49:41 -0700 Reopening for correct (less drastic) fix from Geoff which includes reverting this change. 598590 12 136301 thorton 2012-04-09 13:54:19 -0700 Created attachment 136301 patch (reinstate SimpleClassVectorTraits minus canInitializeWithMemset) 598629 13 136301 ggaren 2012-04-09 14:43:15 -0700 Comment on attachment 136301 patch (reinstate SimpleClassVectorTraits minus canInitializeWithMemset) r=me 598635 14 thorton 2012-04-09 14:47:25 -0700 (In reply to comment #13) > (From update of attachment 136301 [details]) > r=me Thanks Geoff! Landed in http://trac.webkit.org/changeset/113624 136301 2012-04-09 13:54:19 -0700 2012-04-09 14:43:14 -0700 patch (reinstate SimpleClassVectorTraits minus canInitializeWithMemset) framedata.diff text/plain 1653 thorton ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBlOTc4OTBlLi5mNGE2ZDM4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcg QEAKKzIwMTItMDQtMDkgIFRpbSBIb3J0b24gIDx0aW1vdGh5X2hvcnRvbkBhcHBsZS5jb20+CisK KyAgICAgICAgRnJhbWVEYXRhIGNvbnN0cnVjdG9yIHplcm9lcyBhbGwgZmllbGRzLCBjYXVzaW5n IEltYWdlT3JpZW50YXRpb24gdG8gYmUgMAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9ODM0MTYKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBSZWluc3RhdGUgU2ltcGxlQ2xhc3NWZWN0b3JUcmFpdHMgb24gRnJh bWVEYXRhICh3aGljaCB3ZXJlIHJlbW92ZWQgYnkgaHR0cDovL3RyYWMud2Via2l0Lm9yZy9jaGFu Z2VzZXQvMTEzNTQzKQorICAgICAgICB3aXRoIGNhbkluaXRpYWxpemVXaXRoTWVtc2V0IGRpc2Fi bGVkLCBzaW5jZSB0d28gb2YgdGhlIEZyYW1lRGF0YSBmaWVsZHMgY2Fubm90IGJlIGluaXRpYWxp emVkIHRvIHplcm8uCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLgorCisgICAgICAgICogcGxhdGZv cm0vZ3JhcGhpY3MvQml0bWFwSW1hZ2UuaDoKKwogMjAxMi0wNC0wOSAgU2hhd24gU2luZ2ggIDxz aGF3bnNpbmdoQGNocm9taXVtLm9yZz4KIAogICAgICAgICBbY2hyb21pdW1dIEZpeCBsYXllciBz b3J0aW5nIHBlcnNwZWN0aXZlIHcgaWYgdyBiZWNvbWVzIG5lZ2F0aXZlCmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9CaXRtYXBJbWFnZS5oIGIvU291cmNlL1dl YkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvQml0bWFwSW1hZ2UuaAppbmRleCBkZjEyMThmLi45NGVl YjFkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9CaXRtYXBJ bWFnZS5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0JpdG1hcEltYWdl LmgKQEAgLTUwLDYgKzUwLDEyIEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKICAgICBzdHJ1Y3QgRnJh bWVEYXRhOwogfQogCituYW1lc3BhY2UgV1RGIHsKKyAgICB0ZW1wbGF0ZTw+IHN0cnVjdCBWZWN0 b3JUcmFpdHM8V2ViQ29yZTo6RnJhbWVEYXRhPiA6IHB1YmxpYyBTaW1wbGVDbGFzc1ZlY3RvclRy YWl0cyB7CisgICAgICAgIHN0YXRpYyBjb25zdCBib29sIGNhbkluaXRpYWxpemVXaXRoTWVtc2V0 ID0gZmFsc2U7IC8vIE5vdCBhbGwgRnJhbWVEYXRhIG1lbWJlcnMgaW5pdGlhbGl6ZSB0byAwLgor ICAgIH07Cit9CisKIG5hbWVzcGFjZSBXZWJDb3JlIHsKIAogdGVtcGxhdGUgPHR5cGVuYW1lIFQ+ IGNsYXNzIFRpbWVyOwpAQCAtNTksNiArNjUsNyBAQCB0ZW1wbGF0ZSA8dHlwZW5hbWUgVD4gY2xh c3MgVGltZXI7CiAvLyA9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KIAogc3RydWN0IEZyYW1lRGF0YSB7CisgICAgV1RGX01BS0VfTk9OQ09QWUFCTEUoRnJh bWVEYXRhKTsKIHB1YmxpYzoKICAgICBGcmFtZURhdGEoKQogICAgICAgICA6IG1fZnJhbWUoMCkK