Bug 83416 - FrameData constructor zeroes all fields, causing ImageOrientation to be 0
Summary: FrameData constructor zeroes all fields, causing ImageOrientation to be 0
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-04-06 19:56 PDT by Tim Horton
Modified: 2012-04-09 14:47 PDT (History)
5 users (show)

See Also:


Attachments
patch (reinstate SimpleClassVectorTraits minus canInitializeWithMemset) (1.61 KB, patch)
2012-04-09 13:54 PDT, Tim Horton
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2012-04-06 19:56:23 PDT
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.
Comment 1 Radar WebKit Bug Importer 2012-04-06 19:56:41 PDT
<rdar://problem/11205425>
Comment 2 Tim Horton 2012-04-06 19:57:18 PDT
(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).
Comment 3 Nikolas Zimmermann 2012-04-07 00:37:19 PDT
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.
Comment 4 Tim Horton 2012-04-07 00:40:17 PDT
(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.
Comment 5 Tim Horton 2012-04-07 00:44:31 PDT
(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:
Comment 6 Nikolas Zimmermann 2012-04-07 00:59:35 PDT
(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?
Comment 7 Tim Horton 2012-04-07 01:01:34 PDT
(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.
Comment 8 Nikolas Zimmermann 2012-04-07 01:06:02 PDT
(In reply to comment #7)
> I would *totally* not mind.
Great, rs=you? Now get some sleep, I'll take care of the rest ;-)
Comment 9 Tim Horton 2012-04-07 01:07:07 PDT
(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!
Comment 10 Nikolas Zimmermann 2012-04-07 01:10:05 PDT
(In reply to comment #9) 
> (unofficial)rs :D /still no review
Hopefully you are soon :-)

Landed in r113543, closing bug.
Comment 11 Tim Horton 2012-04-09 13:49:41 PDT
Reopening for correct (less drastic) fix from Geoff which includes reverting this change.
Comment 12 Tim Horton 2012-04-09 13:54:19 PDT
Created attachment 136301 [details]
patch (reinstate SimpleClassVectorTraits minus canInitializeWithMemset)
Comment 13 Geoffrey Garen 2012-04-09 14:43:15 PDT
Comment on attachment 136301 [details]
patch (reinstate SimpleClassVectorTraits minus canInitializeWithMemset)

r=me
Comment 14 Tim Horton 2012-04-09 14:47:25 PDT
(In reply to comment #13)
> (From update of attachment 136301 [details])
> r=me

Thanks Geoff!

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