Bug 83416 – FrameData constructor zeroes all fields, causing ImageOrientation to be 0

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Tim Horton]

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.

[Radar WebKit Bug Importer]

<rdar://problem/11205425>

[Tim Horton]

(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).

[Nikolas Zimmermann]

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.

[Tim Horton]

(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.

[Tim Horton]

(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:

[Nikolas Zimmermann]

(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?

[Tim Horton]

(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.

[Nikolas Zimmermann]

(In reply to comment #7)
> I would *totally* not mind.
Great, rs=you? Now get some sleep, I'll take care of the rest ;-)

[Tim Horton]

(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!

[Nikolas Zimmermann]

(In reply to comment #9) 
> (unofficial)rs :D /still no review
Hopefully you are soon :-)

Landed in r113543, closing bug.

[Tim Horton]

Reopening for correct (less drastic) fix from Geoff which includes reverting this change.

[Tim Horton]

Created attachment 136301 [details]
patch (reinstate SimpleClassVectorTraits minus canInitializeWithMemset)

[Geoffrey Garen]

Comment on attachment 136301 [details]
patch (reinstate SimpleClassVectorTraits minus canInitializeWithMemset)

r=me

[Tim Horton]

(In reply to comment #13)
> (From update of attachment 136301 [details])
> r=me

Thanks Geoff!

Landed in http://trac.webkit.org/changeset/113624