Bug 173077 – Image should clear its ImageObserver* when CachedImage releases the last reference to its RefCounted<ImageObserver>

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 312239 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/32611973>

[Build Bot]

Comment on attachment 312239 [details]
Patch

Attachment 312239 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3890363

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.any.html

[Build Bot]

Created attachment 312249 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 312315 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 312315 [details]
Patch

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

> Source/WebCore/loader/cache/CachedImage.cpp:375
> +        if (!m_imageObserver->cachedImages().size()) {

isEmpty()

[Simon Fraser (smfr)]

Is this testable?

[Said Abou-Hallawa]

Created attachment 312450 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 312450 [details]
Patch

Clearing flags on attachment: 312450

Committed r218003: <http://trac.webkit.org/changeset/218003>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #9)
> Comment on attachment 312450 [details]
> Patch
> 
> Clearing flags on attachment: 312450
> 
> Committed r218003: <http://trac.webkit.org/changeset/218003>

I'm seeing a few LayoutTests hit one of the asserts added with this change:

http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/the-canvas-element/size.attributes.removed.html

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r218012%20(1627)/results.html

[Ryan Haddad]

Reverted r218003 for reason:

This change caused assertion failures in existing LayoutTests.

Committed r218031: <http://trac.webkit.org/changeset/218031>

[Said Abou-Hallawa]

(In reply to Ryan Haddad from comment #11)
> (In reply to WebKit Commit Bot from comment #9)
> > Comment on attachment 312450 [details]
> > Patch
> > 
> > Clearing flags on attachment: 312450
> > 
> > Committed r218003: <http://trac.webkit.org/changeset/218003>
> 
> I'm seeing a few LayoutTests hit one of the asserts added with this change:
> 
> http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.
> html
> imported/w3c/web-platform-tests/html/semantics/embedded-content/the-canvas-
> element/size.attributes.removed.html
> 
> https://build.webkit.org/results/
> Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r218012%20(1627)/results.html

Yes. 

There is no guarantee that the Image is referenced only by CachedImage. One of the CachedImage constructors takes an argument of Image* which means the Image object is created and referenced outside the life cycle of CachedImage. When storing it in CachedImage::m_image, the refcount is incremented. So the Image object can outlive CachedImage.

The only thing we can guarantee, is CachedImageObserver can’t outlive CachedImage. So the fix is to remove ASSERT(m_image->hasOneRef()) from CachedImage::clearImage().

[Said Abou-Hallawa]

Created attachment 312530 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 312530 [details]
Patch

Clearing flags on attachment: 312530

Committed r218038: <http://trac.webkit.org/changeset/218038>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.