Bug 173077 - Image should clear its ImageObserver* when CachedImage releases the last reference to its RefCounted<ImageObserver>
Summary: Image should clear its ImageObserver* when CachedImage releases the last refe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-07 15:07 PDT by Said Abou-Hallawa
Modified: 2017-06-09 19:51 PDT (History)
11 users (show)

See Also:


Attachments
Patch (2.71 KB, patch)
2017-06-07 15:13 PDT, Said Abou-Hallawa
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.32 MB, application/zip)
2017-06-07 16:06 PDT, Build Bot
no flags Details
Patch (4.39 KB, patch)
2017-06-08 10:01 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (4.58 KB, patch)
2017-06-09 10:20 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (4.43 KB, patch)
2017-06-09 19:10 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2017-06-07 15:07:17 PDT
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.
Comment 1 Said Abou-Hallawa 2017-06-07 15:13:30 PDT
Created attachment 312239 [details]
Patch
Comment 2 Said Abou-Hallawa 2017-06-07 15:16:07 PDT
<rdar://problem/32611973>
Comment 3 Build Bot 2017-06-07 16:06:09 PDT
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
Comment 4 Build Bot 2017-06-07 16:06:10 PDT
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
Comment 5 Said Abou-Hallawa 2017-06-08 10:01:49 PDT
Created attachment 312315 [details]
Patch
Comment 6 Simon Fraser (smfr) 2017-06-08 15:29:16 PDT
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()
Comment 7 Simon Fraser (smfr) 2017-06-08 15:29:29 PDT
Is this testable?
Comment 8 Said Abou-Hallawa 2017-06-09 10:20:10 PDT
Created attachment 312450 [details]
Patch
Comment 9 WebKit Commit Bot 2017-06-09 11:38:43 PDT
Comment on attachment 312450 [details]
Patch

Clearing flags on attachment: 312450

Committed r218003: <http://trac.webkit.org/changeset/218003>
Comment 10 WebKit Commit Bot 2017-06-09 11:38:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Ryan Haddad 2017-06-09 15:09:28 PDT
(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
Comment 12 Ryan Haddad 2017-06-09 17:38:10 PDT
Reverted r218003 for reason:

This change caused assertion failures in existing LayoutTests.

Committed r218031: <http://trac.webkit.org/changeset/218031>
Comment 13 Said Abou-Hallawa 2017-06-09 18:08:57 PDT
(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().
Comment 14 Said Abou-Hallawa 2017-06-09 19:10:57 PDT
Created attachment 312530 [details]
Patch
Comment 15 WebKit Commit Bot 2017-06-09 19:51:08 PDT
Comment on attachment 312530 [details]
Patch

Clearing flags on attachment: 312530

Committed r218038: <http://trac.webkit.org/changeset/218038>
Comment 16 WebKit Commit Bot 2017-06-09 19:51:10 PDT
All reviewed patches have been landed.  Closing bug.