[Webkit-unassigned] [Bug 173077] New: Image should clear its ImageObserver* when CachedImage releases the last reference to its RefCounted<ImageObserver>

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=173077

            Bug ID: 173077
           Summary: Image should clear its ImageObserver* when CachedImage
                    releases the last reference to its
                    RefCounted<ImageObserver>
           Product: WebKit
           Version: WebKit Nightly Build
          Hardware: Unspecified
                OS: Unspecified
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: Images
          Assignee: webkit-unassigned at lists.webkit.org
          Reporter: sabouhallawa at apple.com

Here is how the objects Image, ImageObserver and CachedImage are related to each other.

-- CachedImage has a RefPtr<Image> and RefPtr<ImageObserver> (or actually it is RefPtr<CachedImageObserver>). CachedImage is the creator and the destroyer of these two pointers.
-- ImageObserver has a Vector<CachedImage*>. CachedImage maintains correct state for this vector by adding and removing itself upon creation and deletion.
-- Image has a raw pointer of ImageObserver*. This pointer is only cleared temporarily but it is never cleared permanently.

Two things to mention here:

-- CachedImage can clone itself to another CachedImage through CachedImage::setBodyDataFrom(). The RefPtr<Image> and RefPtr<ImageObserver> will be set in the cloned CachedImage, which means their ref counts are incremented.
-- Image object can outlive the CachedImage. In the CachedImage destructor, CachedImage::clearImage() is called to clear the references from the ImageObserver to the destructing CachedImage.

If CachedImage releases the last reference to its ImageObserver, this ImageObserver will eventually be deleted. A crash can happen if the Image accesses its raw pointer after the ImageObserver is deleted. So ideally CachedImage::clearImage(): should set the m_imageObserver of the Image to nullptr by calling m_image->setImageObserver(nullptr). But since the CachedImage can be cloned, ImageObserver will not be deleted until the last referencing CachedImage calls CachedImage::clearImage(). So calling m_image->setImageObserver(nullptr) unconditionally from CachedImage::clearImage() is wrong as well.

This is the history of CachedImage::clearImage():

Before https://trac.webkit.org/changeset/209914

inline void CachedImage::clearImage()
{
    // If our Image has an observer, it's always us so we need to clear the back pointer
    // before dropping our reference.
    if (m_image)
        m_image->setImageObserver(nullptr);
    m_image = nullptr;
}

After https://trac.webkit.org/changeset/209914

inline void CachedImage::clearImage()
{
    if (m_imageObserver) {
       m_imageObserver->remove(*this);
       m_imageObserver = nullptr;
    }
    m_image = nullptr;
}

After https://trac.webkit.org/changeset/216273

inline void CachedImage::clearImage()
{
    if (m_imageObserver) {
        m_imageObserver->remove(*this);
        m_imageObserver = nullptr;
    }
    if (m_image) {
        m_image->setImageObserver(nullptr);
        m_image = nullptr;
    }
}

But this caused a crash in http/tests/security/cross-origin-cached-images-with-memory-pressure.html so it was rolled out in https://trac.webkit.org/changeset/216293.

So we still have a bug for not clearing the raw pointer Image::m_imageObserver if CachedImage releases the last reference to its CachedImage::m_imageObserver and Image accesses the deleted ImageObserver through the raw pointer Image::m_imageObserver.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170607/1025db78/attachment.html>