[Webkit-unassigned] [Bug 174168] New: REGRESSION(r208511): RenderImageResourceStyleImage should not assume image() won't return null if its m_cachedImage is valid

[bugzilla-daemon at webkit.org]

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

            Bug ID: 174168
           Summary: REGRESSION(r208511): RenderImageResourceStyleImage
                    should not assume image() won't return null if its
                    m_cachedImage is valid
           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

In the <http://trac.webkit.org/changeset/208511>, RenderImageResource::shutdown() was changed from:

void RenderImageResource::shutdown()
{
    ASSERT(m_renderer);

    if (m_cachedImage)
        m_cachedImage->removeClient(*m_renderer);
}

to be:

void RenderImageResource::shutdown()
{
    ASSERT(m_renderer);

    if (m_cachedImage) {
        image()->stopAnimation();
        m_cachedImage->removeClient(*m_renderer);
    }
}

To check for m_cachedImage and then assume image() won't be null makes sense because the implementation of RenderImageResource::image() is the following:

RefPtr<Image> RenderImageResource::image(const IntSize&) const
{
    return m_cachedImage ? m_cachedImage->imageForRenderer(m_renderer) : &Image::nullImage();
}

The problem was the same change was copied as is to RenderImageResourceStyleImage::shutdown(). So it was changed from:

void RenderImageResourceStyleImage::shutdown()
{
    ASSERT(m_renderer);
    m_styleImage->removeClient(m_renderer);
    m_cachedImage = nullptr;
}

to be:

void RenderImageResourceStyleImage::shutdown()
{
    ASSERT(m_renderer);
    m_styleImage->removeClient(m_renderer);
    if (m_cachedImage) {
        image()->stopAnimation();
        m_cachedImage = nullptr;
    }
}

This change was wrong because RenderImageResourceStyleImage::image() can return null even if m_cachedImage is not null. Here is the implementation of RenderImageResourceStyleImage::image():

RefPtr<Image> RenderImageResourceStyleImage::image(const IntSize& size) const
{
    // Generated content may trigger calls to image() while we're still pending, don't assert but gracefully exit.
    if (m_styleImage->isPending())
        return nullptr;
    return m_styleImage->image(m_renderer, size);
}

Notice StyleCachedImage::isPending can return true if the StyleCachedImage::load() has not been called or it was constructed with CSSImageValue which does not have a valid CachedImage.

This led to the following crash:

[  1] 0x000000018b1f0773 WebCore`WebCore::RenderImageResourceStyleImage::shutdown() [inlined] WebCore::RenderImageResourceStyleImage::image(WebCore::IntSize const&) const + 63 at RenderImageResourceStyleImage.cpp:71:26
[  1] 0x000000018b1f0734 WebCore`WebCore::RenderImageResourceStyleImage::shutdown() + 60 at RenderImageResourceStyleImage.cpp:61
[  2] 0x000000018b1eef5b WebCore`WebCore::RenderImage::willBeDestroyed() + 31 at RenderImage.cpp:151:21
[  3] 0x000000018a4b2d2b WebCore`WebCore::RenderObject::destroyAndCleanupAnonymousWrappers() [inlined] WebCore::RenderObject::destroy() + 39 at RenderObject.cpp:1551:5
[  3] 0x000000018a4b2d04 WebCore`WebCore::RenderObject::destroyAndCleanupAnonymousWrappers() + 224 at RenderObject.cpp:1516
[  4] 0x000000018b2a3617 WebCore`WebCore::RenderTreeUpdater::tearDownRenderers(WebCore::Element&, WebCore::RenderTreeUpdater::TeardownType) [inlined] WebCore::RenderTreeUpdater::tearDownRenderers(WebCore::Element&, WebCore::RenderTreeUpdater::TeardownType)::$_2::operator()(unsigned int) const + 87 at RenderTreeUpdater.cpp:562:27
[  4] 0x000000018b2a35c0 WebCore`WebCore::RenderTreeUpdater::tearDownRenderers(WebCore::Element&, WebCore::RenderTreeUpdater::TeardownType) + 1144 at RenderTreeUpdater.cpp:584
[  5] 0x000000018b2a2adf WebCore`WebCore::RenderTreeUpdater::updateElementRenderer(WebCore::Element&, WebCore::Style::ElementUpdate const&) + 395 at RenderTreeUpdater.cpp:268:9
[  6] 0x000000018b2a1ddf WebCore`WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&) + 647 at RenderTreeUpdater.cpp:178:9
[  7] 0x000000018b2a1adb WebCore`WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >) + 563 at RenderTreeUpdater.cpp:125:9
[  8] 0x000000018a7fda6b WebCore`WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 587 at Document.cpp:1825:21
[  9] 0x000000018a454a57 WebCore`WebCore::ThreadTimers::sharedTimerFiredInternal() + 171 at ThreadTimers.cpp:118:16
[ 10] 0x000000018a454997 WebCore`WebCore::timerFired(__CFRunLoopTimer*, void*) + 27 at MainThreadSharedTimerCF.cpp:74:40

-- 
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/20170705/32a22fe4/attachment.html>