v3: WebContent crash due to RELEASE_ASSERT in WebCore: WebCore::StyleResolver::styleF...
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 9 Sep 2016 07:42:56 +0000 (07:42 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 9 Sep 2016 07:42:56 +0000 (07:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=161689

Reviewed by Andreas Kling.

These crashes happen because synchronously triggered resource loads generate callbacks that may end up
deleting the resource loader.

Stop triggering resource loads from StyleResolver. Instead trigger them when applying style to render tree.

* css/StyleResolver.cpp:
(WebCore::StyleResolver::~StyleResolver):

    Replace the RELEASE_ASSERT against deletion during resource loads by a general isDeleted assert.

(WebCore::StyleResolver::styleForElement):
(WebCore::StyleResolver::styleForKeyframe):
(WebCore::StyleResolver::pseudoStyleForElement):
(WebCore::StyleResolver::styleForPage):
(WebCore::StyleResolver::applyMatchedProperties):
(WebCore::StyleResolver::loadPendingResources): Deleted.
* css/StyleResolver.h:
* page/animation/KeyframeAnimation.cpp:
(WebCore::KeyframeAnimation::KeyframeAnimation):
(WebCore::KeyframeAnimation::resolveKeyframeStyles):

    Ensure resource load for all animation frames.

* page/animation/KeyframeAnimation.h:
* rendering/RenderElement.cpp:
(WebCore::RenderElement::createFor):
(WebCore::RenderElement::initializeStyle):

    Load resources when renderer initializes a style.

(WebCore::RenderElement::setStyle):
(WebCore::RenderElement::getUncachedPseudoStyle):

    Load resources for pseudo styles.

* rendering/RenderImage.cpp:
(WebCore::RenderImage::RenderImage):
(WebCore::RenderImage::styleWillChange):

    Shuffle image resource initialization out from constructor so initializeStyle gets called before.

* rendering/RenderImage.h:
* rendering/style/StyleCachedImage.cpp:
(WebCore::StyleCachedImage::StyleCachedImage):

    Track pending status with a bit instead of implicitly by the existence of CachedResource.
    This is useful for asserts.

(WebCore::StyleCachedImage::load):
(WebCore::StyleCachedImage::isPending):
(WebCore::StyleCachedImage::addClient):
(WebCore::StyleCachedImage::removeClient):
(WebCore::StyleCachedImage::image):
* rendering/style/StyleCachedImage.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@205701 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/css/StyleResolver.cpp
Source/WebCore/css/StyleResolver.h
Source/WebCore/page/animation/ImplicitAnimation.cpp
Source/WebCore/page/animation/KeyframeAnimation.cpp
Source/WebCore/page/animation/KeyframeAnimation.h
Source/WebCore/rendering/RenderElement.cpp
Source/WebCore/rendering/RenderImage.cpp
Source/WebCore/rendering/RenderImage.h
Source/WebCore/rendering/style/StyleCachedImage.cpp
Source/WebCore/rendering/style/StyleCachedImage.h

index 686f620..b9c150d 100644 (file)
@@ -1,3 +1,65 @@
+2016-09-09  Antti Koivisto  <antti@apple.com>
+
+        v3: WebContent crash due to RELEASE_ASSERT in WebCore: WebCore::StyleResolver::styleForElement
+        https://bugs.webkit.org/show_bug.cgi?id=161689
+
+        Reviewed by Andreas Kling.
+
+        These crashes happen because synchronously triggered resource loads generate callbacks that may end up
+        deleting the resource loader.
+
+        Stop triggering resource loads from StyleResolver. Instead trigger them when applying style to render tree.
+
+        * css/StyleResolver.cpp:
+        (WebCore::StyleResolver::~StyleResolver):
+
+            Replace the RELEASE_ASSERT against deletion during resource loads by a general isDeleted assert.
+
+        (WebCore::StyleResolver::styleForElement):
+        (WebCore::StyleResolver::styleForKeyframe):
+        (WebCore::StyleResolver::pseudoStyleForElement):
+        (WebCore::StyleResolver::styleForPage):
+        (WebCore::StyleResolver::applyMatchedProperties):
+        (WebCore::StyleResolver::loadPendingResources): Deleted.
+        * css/StyleResolver.h:
+        * page/animation/KeyframeAnimation.cpp:
+        (WebCore::KeyframeAnimation::KeyframeAnimation):
+        (WebCore::KeyframeAnimation::resolveKeyframeStyles):
+
+            Ensure resource load for all animation frames.
+
+        * page/animation/KeyframeAnimation.h:
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::createFor):
+        (WebCore::RenderElement::initializeStyle):
+
+            Load resources when renderer initializes a style.
+
+        (WebCore::RenderElement::setStyle):
+        (WebCore::RenderElement::getUncachedPseudoStyle):
+
+            Load resources for pseudo styles.
+
+        * rendering/RenderImage.cpp:
+        (WebCore::RenderImage::RenderImage):
+        (WebCore::RenderImage::styleWillChange):
+
+            Shuffle image resource initialization out from constructor so initializeStyle gets called before.
+
+        * rendering/RenderImage.h:
+        * rendering/style/StyleCachedImage.cpp:
+        (WebCore::StyleCachedImage::StyleCachedImage):
+
+            Track pending status with a bit instead of implicitly by the existence of CachedResource.
+            This is useful for asserts.
+
+        (WebCore::StyleCachedImage::load):
+        (WebCore::StyleCachedImage::isPending):
+        (WebCore::StyleCachedImage::addClient):
+        (WebCore::StyleCachedImage::removeClient):
+        (WebCore::StyleCachedImage::image):
+        * rendering/style/StyleCachedImage.h:
+
 2016-09-08  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         ScriptRunner should be driven by PendingScript rather than ScriptElement
index 74892bb..12ba180 100644 (file)
@@ -309,7 +309,8 @@ void StyleResolver::addKeyframeStyle(Ref<StyleRuleKeyframes>&& rule)
 
 StyleResolver::~StyleResolver()
 {
-    RELEASE_ASSERT(!m_inLoadPendingImages);
+    RELEASE_ASSERT(!m_isDeleted);
+    m_isDeleted = true;
 
 #if ENABLE(CSS_DEVICE_ADAPTATION)
     m_viewportStyleResolver->clearDocument();
@@ -385,7 +386,7 @@ static inline bool isAtShadowBoundary(const Element& element)
 
 ElementStyle StyleResolver::styleForElement(const Element& element, const RenderStyle* parentStyle, RuleMatchingBehavior matchingBehavior, const RenderRegion* regionForStyling, const SelectorFilter* selectorFilter)
 {
-    RELEASE_ASSERT(!m_inLoadPendingImages);
+    RELEASE_ASSERT(!m_isDeleted);
 
     m_state = State(element, parentStyle, m_overrideDocumentElementStyle, regionForStyling, selectorFilter);
     State& state = m_state;
@@ -446,7 +447,7 @@ ElementStyle StyleResolver::styleForElement(const Element& element, const Render
 
 std::unique_ptr<RenderStyle> StyleResolver::styleForKeyframe(const RenderStyle* elementStyle, const StyleKeyframe* keyframe, KeyframeValue& keyframeValue)
 {
-    RELEASE_ASSERT(!m_inLoadPendingImages);
+    RELEASE_ASSERT(!m_isDeleted);
 
     MatchResult result;
     result.addMatchedProperties(keyframe->properties());
@@ -486,9 +487,6 @@ std::unique_ptr<RenderStyle> StyleResolver::styleForKeyframe(const RenderStyle*
 
     adjustRenderStyle(*state.style(), *state.parentStyle(), nullptr);
 
-    // Start loading resources referenced by this style.
-    loadPendingResources();
-    
     // Add all the animating properties to the keyframe.
     unsigned propertyCount = keyframe->properties().propertyCount();
     for (unsigned i = 0; i < propertyCount; ++i) {
@@ -642,16 +640,13 @@ std::unique_ptr<RenderStyle> StyleResolver::pseudoStyleForElement(const Element&
     if (state.style()->hasViewportUnits())
         document().setHasStyleWithViewportUnits();
 
-    // Start loading resources referenced by this style.
-    loadPendingResources();
-
     // Now return the style.
     return state.takeStyle();
 }
 
 std::unique_ptr<RenderStyle> StyleResolver::styleForPage(int pageIndex)
 {
-    RELEASE_ASSERT(!m_inLoadPendingImages);
+    RELEASE_ASSERT(!m_isDeleted);
 
     auto* documentElement = m_document.documentElement();
     if (!documentElement)
@@ -686,9 +681,6 @@ std::unique_ptr<RenderStyle> StyleResolver::styleForPage(int pageIndex)
 
     cascade.applyDeferredProperties(*this, &result);
 
-    // Start loading resources referenced by this style.
-    loadPendingResources();
-
     // Now return the style.
     return m_state.takeStyle();
 }
@@ -1413,9 +1405,6 @@ void StyleResolver::applyMatchedProperties(const MatchResult& matchResult, const
     // so to preserve behavior, we queue them up during cascade and flush here.
     cascade.applyDeferredProperties(*this, &matchResult);
 
-    // Start loading resources referenced by this style.
-    loadPendingResources();
-    
     ASSERT(!state.fontDirty());
     
     if (cacheItem || !cacheHash)
@@ -2043,18 +2032,6 @@ bool StyleResolver::createFilterOperations(const CSSValue& inValue, FilterOperat
     return true;
 }
 
-void StyleResolver::loadPendingResources()
-{
-    ASSERT(style());
-    if (!style())
-        return;
-
-    RELEASE_ASSERT(!m_inLoadPendingImages);
-    TemporaryChange<bool> changeInLoadPendingImages(m_inLoadPendingImages, true);
-
-    Style::loadPendingResources(*style(), document(), m_state.element());
-}
-
 inline StyleResolver::MatchedProperties::MatchedProperties()
     : possiblyPaddedMember(nullptr)
 {
index d41eed8..963e683 100644 (file)
@@ -220,9 +220,6 @@ public:
     void clearCachedPropertiesAffectedByViewportUnits();
 
     bool createFilterOperations(const CSSValue& inValue, FilterOperations& outOperations);
-    void loadPendingSVGDocuments();
-
-    void loadPendingResources();
 
     struct RuleRange {
         RuleRange(int& firstRuleIndex, int& lastRuleIndex): firstRuleIndex(firstRuleIndex), lastRuleIndex(lastRuleIndex) { }
@@ -482,8 +479,6 @@ private:
 
     void applySVGProperty(CSSPropertyID, CSSValue*);
 
-    void loadPendingImages();
-
     static unsigned computeMatchedPropertiesHash(const MatchedProperties*, unsigned size);
     struct MatchedPropertiesCacheItem {
         Vector<MatchedProperties> matchedProperties;
@@ -525,8 +520,8 @@ private:
 
     State m_state;
 
-    // Try to catch a crash. https://bugs.webkit.org/show_bug.cgi?id=141561.
-    bool m_inLoadPendingImages { false };
+    // See if we still have crashes where StyleResolver gets deleted early.
+    bool m_isDeleted { false };
 
     friend bool operator==(const MatchedProperties&, const MatchedProperties&);
     friend bool operator!=(const MatchedProperties&, const MatchedProperties&);
index 88075fd..c70053e 100644 (file)
@@ -36,6 +36,7 @@
 #include "ImplicitAnimation.h"
 #include "KeyframeAnimation.h"
 #include "RenderBox.h"
+#include "StylePendingResources.h"
 
 namespace WebCore {
 
@@ -212,6 +213,9 @@ void ImplicitAnimation::reset(const RenderStyle* to)
 
     m_toStyle = RenderStyle::clonePtr(*to);
 
+    if (m_object && m_object->element())
+        Style::loadPendingResources(*m_toStyle, m_object->element()->document(), m_object->element());
+
     // Restart the transition
     if (m_fromStyle && m_toStyle)
         updateStateMachine(AnimationStateInput::RestartAnimation, -1);
index 64bdc87..6e9e5cc 100644 (file)
@@ -37,6 +37,7 @@
 #include "GeometryUtilities.h"
 #include "RenderBox.h"
 #include "RenderStyle.h"
+#include "StylePendingResources.h"
 #include "StyleResolver.h"
 
 namespace WebCore {
@@ -46,9 +47,7 @@ KeyframeAnimation::KeyframeAnimation(const Animation& animation, RenderElement*
     , m_keyframes(animation.name())
     , m_unanimatedStyle(RenderStyle::clonePtr(*unanimatedStyle))
 {
-    // Get the keyframe RenderStyles
-    if (m_object && m_object->element())
-        m_object->element()->styleResolver().keyframeStylesForAnimation(*m_object->element(), unanimatedStyle, m_keyframes);
+    resolveKeyframeStyles();
 
     // Update the m_transformFunctionListValid flag based on whether the function lists in the keyframes match.
     validateTransformFunctionList();
@@ -350,6 +349,21 @@ bool KeyframeAnimation::affectsProperty(CSSPropertyID property) const
     return m_keyframes.containsProperty(property);
 }
 
+void KeyframeAnimation::resolveKeyframeStyles()
+{
+    if (!m_object || !m_object->element())
+        return;
+    auto& element = *m_object->element();
+
+    element.styleResolver().keyframeStylesForAnimation(*m_object->element(), m_unanimatedStyle.get(), m_keyframes);
+
+    // Ensure resource loads for all the frames.
+    for (auto& keyframe : m_keyframes.keyframes()) {
+        if (auto* style = const_cast<RenderStyle*>(keyframe.style()))
+            Style::loadPendingResources(*style, element.document(), &element);
+    }
+}
+
 void KeyframeAnimation::validateTransformFunctionList()
 {
     m_transformFunctionListsMatch = false;
index a7a11bf..848dd40 100644 (file)
@@ -81,6 +81,7 @@ protected:
 
     bool computeExtentOfAnimationForMatchingTransformLists(const FloatRect& rendererBox, LayoutRect&) const;
 
+    void resolveKeyframeStyles();
     void validateTransformFunctionList();
     void checkForMatchingFilterFunctionLists();
 #if ENABLE(FILTERS_LEVEL_2)
index 20cd009..8090788 100644 (file)
@@ -66,6 +66,7 @@
 #include "SVGRenderSupport.h"
 #include "Settings.h"
 #include "ShadowRoot.h"
+#include "StylePendingResources.h"
 #include "StyleResolver.h"
 #include <wtf/MathExtras.h>
 #include <wtf/StackStats.h>
@@ -150,6 +151,7 @@ RenderPtr<RenderElement> RenderElement::createFor(Element& element, RenderStyle&
     // Otherwise acts as if we didn't support this feature.
     const ContentData* contentData = style.contentData();
     if (contentData && !contentData->next() && is<ImageContentData>(*contentData) && !element.isPseudoElement()) {
+        Style::loadPendingResources(style, element.document(), &element);
         auto& styleImage = downcast<ImageContentData>(*contentData).image();
         auto image = createRenderer<RenderImage>(element, WTFMove(style), const_cast<StyleImage*>(&styleImage));
         image->setIsGeneratedContent();
@@ -363,6 +365,8 @@ void RenderElement::updateShapeImage(const ShapeValue* oldShapeValue, const Shap
 
 void RenderElement::initializeStyle()
 {
+    Style::loadPendingResources(m_style, document(), element());
+
     styleWillChange(StyleDifferenceNewStyle, style());
 
     m_hasInitializedStyle = true;
@@ -402,6 +406,8 @@ void RenderElement::setStyle(RenderStyle&& style, StyleDifference minimalStyleDi
 
     diff = adjustStyleDifference(diff, contextSensitiveProperties);
 
+    Style::loadPendingResources(style, document(), element());
+
     styleWillChange(diff, style);
 
     auto oldStyle = WTFMove(m_style);
@@ -1571,13 +1577,17 @@ std::unique_ptr<RenderStyle> RenderElement::getUncachedPseudoStyle(const PseudoS
 
     auto& styleResolver = element()->styleResolver();
 
+    std::unique_ptr<RenderStyle> style;
     if (pseudoStyleRequest.pseudoId == FIRST_LINE_INHERITED) {
-        auto result = styleResolver.styleForElement(*element(), parentStyle).renderStyle;
-        result->setStyleType(FIRST_LINE_INHERITED);
-        return result;
-    }
+        style = styleResolver.styleForElement(*element(), parentStyle).renderStyle;
+        style->setStyleType(FIRST_LINE_INHERITED);
+    } else
+        style = styleResolver.pseudoStyleForElement(*element(), pseudoStyleRequest, *parentStyle);
+
+    if (style)
+        Style::loadPendingResources(*style, document(), element());
 
-    return styleResolver.pseudoStyleForElement(*element(), pseudoStyleRequest, *parentStyle);
+    return style;
 }
 
 Color RenderElement::selectionColor(int colorProperty) const
index 3edf060..8c7f476 100644 (file)
@@ -131,7 +131,6 @@ RenderImage::RenderImage(Element& element, RenderStyle&& style, StyleImage* styl
     , m_imageDevicePixelRatio(imageDevicePixelRatio)
 {
     updateAltText();
-    imageResource().initialize(this);
     if (is<HTMLImageElement>(element))
         m_hasShadowControls = downcast<HTMLImageElement>(element).hasShadowControls();
 }
@@ -140,7 +139,6 @@ RenderImage::RenderImage(Document& document, RenderStyle&& style, StyleImage* st
     : RenderReplaced(document, WTFMove(style), IntSize())
     , m_imageResource(styleImage ? std::make_unique<RenderImageResourceStyleImage>(*styleImage) : std::make_unique<RenderImageResource>())
 {
-    imageResource().initialize(this);
 }
 
 RenderImage::~RenderImage()
@@ -201,6 +199,13 @@ ImageSizeChangeType RenderImage::setImageSizeForAltText(CachedImage* newImage /*
     return ImageSizeChangeForAltText;
 }
 
+void RenderImage::styleWillChange(StyleDifference diff, const RenderStyle& newStyle)
+{
+    if (!hasInitializedStyle())
+        imageResource().initialize(this);
+    RenderReplaced::styleWillChange(diff, newStyle);
+}
+
 void RenderImage::styleDidChange(StyleDifference diff, const RenderStyle* oldStyle)
 {
     RenderReplaced::styleDidChange(diff, oldStyle);
index 0320c45..bd05d93 100644 (file)
@@ -77,6 +77,7 @@ protected:
     void computeIntrinsicRatioInformation(FloatSize& intrinsicSize, double& intrinsicRatio) const final;
     bool foregroundIsKnownToBeOpaqueInRect(const LayoutRect& localRect, unsigned maxDepthToTest) const override;
 
+    void styleWillChange(StyleDifference, const RenderStyle& newStyle) override;
     void styleDidChange(StyleDifference, const RenderStyle*) override;
 
     void imageChanged(WrappedImagePtr, const IntRect* = nullptr) override;
index c5a6b0d..b181460 100644 (file)
@@ -40,8 +40,11 @@ StyleCachedImage::StyleCachedImage(CSSValue& cssValue)
     m_isCachedImage = true;
 
     // CSSImageValue doesn't get invalidated so we can grab the CachedImage immediately if it exists.
-    if (is<CSSImageValue>(m_cssValue))
+    if (is<CSSImageValue>(m_cssValue)) {
         m_cachedImage = downcast<CSSImageValue>(m_cssValue.get()).cachedImage();
+        if (m_cachedImage)
+            m_isPending = false;
+    }
 }
 
 StyleCachedImage::~StyleCachedImage()
@@ -66,7 +69,8 @@ bool StyleCachedImage::operator==(const StyleImage& other) const
 
 void StyleCachedImage::load(CachedResourceLoader& loader, const ResourceLoaderOptions& options)
 {
-    ASSERT(isPending());
+    ASSERT(m_isPending);
+    m_isPending = false;
 
     if (is<CSSImageValue>(m_cssValue)) {
         auto& imageValue = downcast<CSSImageValue>(m_cssValue.get());
@@ -106,7 +110,7 @@ bool StyleCachedImage::canRender(const RenderObject* renderer, float multiplier)
 
 bool StyleCachedImage::isPending() const
 {
-    return !m_cachedImage;
+    return m_isPending;
 }
 
 bool StyleCachedImage::isLoaded() const
@@ -169,6 +173,7 @@ void StyleCachedImage::setContainerSizeForRenderer(const RenderElement* renderer
 
 void StyleCachedImage::addClient(RenderElement* renderer)
 {
+    ASSERT(!m_isPending);
     if (!m_cachedImage)
         return;
     m_cachedImage->addClient(renderer);
@@ -176,6 +181,7 @@ void StyleCachedImage::addClient(RenderElement* renderer)
 
 void StyleCachedImage::removeClient(RenderElement* renderer)
 {
+    ASSERT(!m_isPending);
     if (!m_cachedImage)
         return;
     m_cachedImage->removeClient(renderer);
@@ -183,6 +189,7 @@ void StyleCachedImage::removeClient(RenderElement* renderer)
 
 RefPtr<Image> StyleCachedImage::image(RenderElement* renderer, const FloatSize&) const
 {
+    ASSERT(!m_isPending);
     if (!m_cachedImage)
         return nullptr;
     return m_cachedImage->imageForRenderer(renderer);
index 1410c10..e3361db 100644 (file)
@@ -69,6 +69,7 @@ private:
     StyleCachedImage(CSSValue&);
 
     Ref<CSSValue> m_cssValue;
+    bool m_isPending { true };
     mutable float m_scaleFactor { 1 };
     mutable CachedResourceHandle<CachedImage> m_cachedImage;
 };