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
<rdar://problem/32967205>
Created attachment 314640 [details] Patch
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.
Created attachment 314653 [details] Patch
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?
Created attachment 314666 [details] Patch
Comment on attachment 314666 [details] Patch Is this testable?
(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.
Comment on attachment 314666 [details] Patch Clearing flags on attachment: 314666 Committed r219205: <http://trac.webkit.org/changeset/219205>
All reviewed patches have been landed. Closing bug.