Bug 174168 – REGRESSION(r208511): RenderImageResourceStyleImage should not assume image() won't return null if its m_cachedImage is valid

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

<rdar://problem/32967205>

[Said Abou-Hallawa]

Created attachment 314640 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 314640 [details]
Patch

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

> Source/WebCore/rendering/style/StyleImage.h:75
> +    virtual bool isCachedImage() const { return false; }
> +    virtual bool isGeneratedImage() const { return false; }
>  
>  protected:
> -    StyleImage()
> -        : m_isCachedImage(false)
> -        , m_isGeneratedImage(false)
> -    {
> -    }
> -    bool m_isCachedImage : 1;
> -    bool m_isGeneratedImage : 1;
> +    StyleImage() { }

Why did this code change? We normally use this technique when performance matters, to avoid virtual function calls.

[Said Abou-Hallawa]

Created attachment 314653 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 314653 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        can return true and hence, RenderImageResourceStyleImage::image() will return
> +        a null pointer.

But you are making it return the nullImage(), so are there other ways that null can get returned?

[Said Abou-Hallawa]

Created attachment 314666 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 314666 [details]
Patch

Is this testable?

[Said Abou-Hallawa]

(In reply to Simon Fraser (smfr) from comment #7)
> Comment on attachment 314666 [details]
> Patch
> 
> Is this testable?

I tried but I could not find a way to reproduce the crash without the patch. It looks like the scenario is the following. A content data of type image is defined for an element, for example:

<style>
    div::before {
        content: url(some-url);
    }
</style>

And before resolving the style and before loading the pending images, i.e. before calling StyleCachedImage::load(), the renderer get deleted, i.e. RenderImageResourceStyleImage::shutdown() gets called. In this caseRenderImageResourceStyleImage::image() will return nullptr and the crash happens.

But this timing is can't be controlled by script.

[WebKit Commit Bot]

Comment on attachment 314666 [details]
Patch

Clearing flags on attachment: 314666

Committed r219205: <http://trac.webkit.org/changeset/219205>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.