Bug 168396 – ImageFrame has to implement its copy constructor

• Component: Images
• Status: RESOLVED DUPLICATE

[Said Abou-Hallawa]

Currently the ImageFrame copy constructor calls the assignment operator. This is a bad idea because the members of the object aren't initialized properly when calling the assignment operator. The problematic member is m_nativeImage. Assigning a new RetainPtr to m_nativeImage will force calling the destructor of the old RetainPtr which is garbage at that time. This may lead to the following crash:

Thread[0] EXC_BAD_ACCESS (SIGBUS) (KERN_PROTECTION_FAILURE at 0x00000001123fd000)
[  0] 0x0000000107fda288 WebCore`WebCore::ImageFrame::ImageFrame() [inlined] WTF::RetainPtr<CGImage*>::RetainPtr() at RetainPtr.h:64:19
[  0] 0x0000000107fda288 WebCore`WebCore::ImageFrame::ImageFrame() [inlined] WTF::RetainPtr<CGImage*>::RetainPtr() at RetainPtr.h:64
[  0] 0x0000000107fda288 WebCore`WebCore::ImageFrame::ImageFrame() [inlined] WebCore::ImageFrame::ImageFrame() + 20 at ImageFrame.cpp:33
[  1] 0x0000000107894a27 WebCore`WebCore::ImageFrameCache::growFrames() [inlined] WTF::VectorInitializer<true, false, WebCore::ImageFrame>::initialize(WebCore::ImageFrame*, WebCore::ImageFrame*) + 28 at Vector.h:79:32
[  1] 0x0000000107894a0b WebCore`WebCore::ImageFrameCache::growFrames() [inlined] WTF::VectorTypeOperations<WebCore::ImageFrame>::initialize(WebCore::ImageFrame*, WebCore::ImageFrame*) at Vector.h:229
[  1] 0x0000000107894a0b WebCore`WebCore::ImageFrameCache::growFrames() [inlined] WTF::Vector<WebCore::ImageFrame, 1ul, WTF::CrashOnOverflow, 16ul>::grow(unsigned long) + 77 at Vector.h:1036
[  1] 0x00000001078949be WebCore`WebCore::ImageFrameCache::growFrames() + 46 at ImageFrameCache.cpp:165
[  2] 0x0000000107ca9a1a WebCore`WebCore::ImageSource::dataChanged(WebCore::SharedBuffer*, bool) + 90 at ImageSource.cpp:155:5

[Said Abou-Hallawa]

Created attachment 301662 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/28905851>

[Simon Fraser (smfr)]

Comment on attachment 301662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301662&action=review

> Source/WebCore/ChangeLog:8
> +        Calling the assignment operator form its copy constructor is a bad idea.

form -> from

> Source/WebCore/ChangeLog:11
> +        assignment operator. Assigning a new RetainPtr to m_nativeImage will force
> +        calling the destructor of the old RetainPtr which is garbage at that time.

I don't understand this. A RetainPtr is initialized to null, and copying one just bumps the retain count. Where did the garbage come from?

[Simon Fraser (smfr)]

Comment on attachment 301662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301662&action=review

>> Source/WebCore/ChangeLog:11
>> +        calling the destructor of the old RetainPtr which is garbage at that time.
> 
> I don't understand this. A RetainPtr is initialized to null, and copying one just bumps the retain count. Where did the garbage come from?

I now understand, but you should say it more clearly, that in a copy constructor, the data members have not yet been initialized.

> Source/WebCore/platform/graphics/ImageFrame.cpp:112
> +    *this = defaultFrame();

This is a bit of a weird way to reset the data members. Usually we just reset them manually.

[Said Abou-Hallawa]

Comment on attachment 301662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301662&action=review

>>> Source/WebCore/ChangeLog:11
>>> +        calling the destructor of the old RetainPtr which is garbage at that time.
>> 
>> I don't understand this. A RetainPtr is initialized to null, and copying one just bumps the retain count. Where did the garbage come from?
> 
> I now understand, but you should say it more clearly, that in a copy constructor, the data members have not yet been initialized.

I think I am wrong here and I confused you. I realized that In general calling the assignment operator from the copy constructor is a bad idea. The reason is the members of the class *may* not be properly initialized at that time. 

-- For example this code has a bug:

class A
{
public:
    A() : m_ptr(nullptr) { }
    A(const A& other) { *this = other; }
    A& operator=(const A& other)
    {
        if (m_ptr)
            delete [] m_ptr; // BUG: m_ptr is not initialized when this is called form the copy constructor
       ....
    }
private:
    char* m_ptr;
};

-- This code is fine but it is ugly:

class B
{
public:
    B() { }
    B(const B& other) { *this = other; }
    B& operator=(const A& other)
    {
        if (m_ptr)
            delete [] m_ptr; // OK: m_ptr is initialized when this is called form the copy constructor
       ....
    }
private:
    char* m_ptr { nullptr };
    String m_str;
};

The members of the class have to be initialized before executing the body of the constructor. So m_ptr will be set to nullptr then the default constructor of the String object will be called and finally the body of the copy constructor of class B will be executed. Please see the "Initialization order" section in http://en.cppreference.com/w/cpp/language/initializer_list.

The ImageFrame is similar to class B above. Its members either have initializers in the class definition or they are classes. This means, the default constructor of the RetainPtr has to be called before entering the body of the copy constructor regardless how it is implemented.

So I think making the the copy constructor of ImageFrame calling its assignment operator is ugly but can't cause the crash I mentioned above.

[Said Abou-Hallawa]

Comment on attachment 301662 [details]
Patch

Removing the r+ flag till I figure out the reason of the crash and decide if cleaning the ImageFrame copy constructor is still needed or not.

[Said Abou-Hallawa]

The crash below was fixed by <http://trac.webkit.org/changeset/217392>. So this bug can be considered a duplicate of https://bugs.webkit.org/show_bug.cgi?id=172552.

*** This bug has been marked as a duplicate of bug 172552 ***