Changeset 195411 in webkit


Ignore:
Timestamp:
Jan 21, 2016 9:50:26 AM (8 years ago)
Author:
commit-queue@webkit.org
Message:

A crash reproducible in Path::isEmpty() under RenderSVGShape::paint()
https://bugs.webkit.org/show_bug.cgi?id=149613

Patch by Said Abou-Hallawa <sabouhallawa@apple.com> on 2016-01-21
Reviewed by Darin Adler.
Source/WebCore:

When RenderSVGRoot::layout() realizes its layout size has changed and
it has resources which have relative sizes, it marks all the clients of
the resources for invalidates regardless whether they belong to the
same RenderSVGRoot or not. But it reruns the layout only for its children.
If one of these clients comes before the current RenderSVGRoot in the render
tree, ee end up having renderer marked for invalidation at rendering time.
This also prevents scheduling the layout if the same renderer is marked
for another invalidation later. We prevent this because we do not want
to schedule another layout for a renderer which is already marked for
invalidation. This can cause crash if the renderer is an RenderSVGPath.

The fix is to mark "only" the clients of a resource which belong to the
same RenderSVGRoot of the resource. Also we need to run the layout for
all the resources which belong to different RenderSVGRoots before running
the layout for an SVG renderer.

Tests: svg/custom/filter-update-different-root.html

svg/custom/pattern-update-different-root.html

  • rendering/svg/RenderSVGResourceContainer.cpp:

(WebCore::RenderSVGResourceContainer::markAllClientsForInvalidation):
We should not mark any client outside the current root for invalidation

  • rendering/svg/RenderSVGResourceContainer.h: Remove unneeded private keyword.
  • rendering/svg/RenderSVGRoot.cpp:

(WebCore::RenderSVGRoot::addResourceForClientInvalidation):
Code clean up; use findTreeRootObject() instead of repeating the same code.

  • rendering/svg/RenderSVGShape.cpp:

(WebCore::RenderSVGShape::isEmpty): Avoid crashing if RenderSVGShape::isEmpty()
is called before calling RenderSVGShape::layout().

  • rendering/svg/RenderSVGText.cpp:

(WebCore::RenderSVGText::layout): findTreeRootObject() now returns a pointer.

  • rendering/svg/SVGRenderSupport.cpp:

(WebCore::SVGRenderSupport::findTreeRootObject): I do think nothing
guarantees that an SVG renderer has to have an RenderSVGRoot in its
ancestors. So change this function to return a pointer. Also Provide
the non-const version of this function.

(WebCore::SVGRenderSupport::layoutDifferentRootIfNeeded): Runs the layout
if needed for all the resources which belong to different RenderSVGRoots.

(WebCore::SVGRenderSupport::layoutChildren): Make sure all the renderer's
resources which belong to different RenderSVGRoots are laid out before
running the layout for this renderer.

  • rendering/svg/SVGRenderSupport.h: Remove a mysterious comment.
  • rendering/svg/SVGResources.cpp:

(WebCore::SVGResources::layoutDifferentRootIfNeeded): Run the layout for
all the resources which belong to different RenderSVGRoots outside the
context of their RenderSVGRoots.

  • rendering/svg/SVGResources.h:

(WebCore::SVGResources::clipper):
(WebCore::SVGResources::markerStart):
(WebCore::SVGResources::markerMid):
(WebCore::SVGResources::markerEnd):
(WebCore::SVGResources::masker):
(WebCore::SVGResources::filter):
(WebCore::SVGResources::fill):
(WebCore::SVGResources::stroke):
Code clean up; use nullptr instead of 0.

LayoutTests:

When running the layout of an SVG root and it has resources which are
referenced by clients in other SVG roots, make sure we run the layout
for these resources before running the layout for their clients.

  • svg/custom/filter-update-different-root-expected.html: Added.
  • svg/custom/filter-update-different-root.html: Added.

Without this patch this test crashes because we paint a dirty RenderSVGShape.

  • svg/custom/pattern-update-different-root-expected.html: Added.
  • svg/custom/pattern-update-different-root.html: Added.

Without this patch this test works fine but it is good to have it to catch
cases where the SVG root needs to run re-layout for its children resources
and hence their clients if its size has changed.

  • svg/custom/unicode-in-tspan-multi-svg-crash-expected.txt:
  • svg/custom/unicode-in-tspan-multi-svg-crash.html:

This test was ported from Blink in http://trac.webkit.org/changeset/166420.
The expectation of this test was changed in Blink:
https://src.chromium.org/viewvc/blink?revision=158480&view=revision.

Location:
trunk
Files:
4 added
13 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r195405 r195411  
     12016-01-21  Said Abou-Hallawa  <sabouhallawa@apple.com>
     2
     3        A crash reproducible in Path::isEmpty() under RenderSVGShape::paint()
     4        https://bugs.webkit.org/show_bug.cgi?id=149613
     5
     6        Reviewed by Darin Adler.
     7       
     8        When running the layout of an SVG root and it has resources which are
     9        referenced by clients in other SVG roots, make sure we run the layout
     10        for these resources before running the layout for their clients.
     11
     12        * svg/custom/filter-update-different-root-expected.html: Added.
     13        * svg/custom/filter-update-different-root.html: Added.
     14        Without this patch this test crashes because we paint a dirty RenderSVGShape.
     15       
     16        * svg/custom/pattern-update-different-root-expected.html: Added.
     17        * svg/custom/pattern-update-different-root.html: Added.
     18        Without this patch this test works fine but it is good to have it to catch
     19        cases where the SVG root needs to run re-layout for its children resources
     20        and hence their clients if its size has changed.
     21
     22        * svg/custom/unicode-in-tspan-multi-svg-crash-expected.txt:
     23        * svg/custom/unicode-in-tspan-multi-svg-crash.html:
     24        This test was ported from Blink in http://trac.webkit.org/changeset/166420.
     25        The expectation of this test was changed in Blink:
     26        https://src.chromium.org/viewvc/blink?revision=158480&view=revision.
     27       
     28
    1292016-01-21  Nan Wang  <n_wang@apple.com>
    230
  • trunk/LayoutTests/svg/custom/unicode-in-tspan-multi-svg-crash-expected.txt

    r168350 r195411  
     1
    12 
    2 Test Passes if there is no crash in Debug or Asan builds. There should be no characters preceding "Test".
     3Test Passes if there is no crash in Debug or Asan builds.
  • trunk/LayoutTests/svg/custom/unicode-in-tspan-multi-svg-crash.html

    r166420 r195411  
    3434    </svg>
    3535
    36     <p>Test Passes if there is no crash in Debug or Asan builds. There should be no characters preceding "Test".</p>
     36    <p>Test Passes if there is no crash in Debug or Asan builds.</p>
    3737  </body>
    3838</html>
  • trunk/Source/WebCore/ChangeLog

    r195410 r195411  
     12016-01-21  Said Abou-Hallawa  <sabouhallawa@apple.com>
     2
     3        A crash reproducible in Path::isEmpty() under RenderSVGShape::paint()
     4        https://bugs.webkit.org/show_bug.cgi?id=149613
     5
     6        Reviewed by Darin Adler.
     7
     8        When RenderSVGRoot::layout() realizes its layout size has changed and
     9        it has resources which have relative sizes, it marks all the clients of
     10        the resources for invalidates regardless whether they belong to the
     11        same RenderSVGRoot or not. But it reruns the layout only for its children.
     12        If one of these clients comes before the current RenderSVGRoot in the render
     13        tree, ee end up having renderer marked for invalidation at rendering time.
     14        This also prevents scheduling the layout if the same renderer is marked
     15        for another invalidation later. We prevent this because we do not want
     16        to schedule another layout for a renderer which is already marked for
     17        invalidation. This can cause crash if the renderer is an RenderSVGPath.
     18       
     19        The fix is to mark "only" the clients of a resource which belong to the
     20        same RenderSVGRoot of the resource. Also we need to run the layout for
     21        all the resources which belong to different RenderSVGRoots before running
     22        the layout for an SVG renderer.
     23         
     24        Tests: svg/custom/filter-update-different-root.html
     25               svg/custom/pattern-update-different-root.html
     26
     27        * rendering/svg/RenderSVGResourceContainer.cpp:
     28        (WebCore::RenderSVGResourceContainer::markAllClientsForInvalidation):
     29        We should not mark any client outside the current root for invalidation
     30       
     31        * rendering/svg/RenderSVGResourceContainer.h: Remove unneeded private keyword.
     32       
     33        * rendering/svg/RenderSVGRoot.cpp:
     34        (WebCore::RenderSVGRoot::addResourceForClientInvalidation):
     35        Code clean up; use findTreeRootObject() instead of repeating the same code.
     36       
     37        * rendering/svg/RenderSVGShape.cpp:
     38        (WebCore::RenderSVGShape::isEmpty): Avoid crashing if RenderSVGShape::isEmpty()
     39        is called before calling RenderSVGShape::layout().
     40         
     41        * rendering/svg/RenderSVGText.cpp:
     42        (WebCore::RenderSVGText::layout): findTreeRootObject() now returns a pointer.
     43       
     44        * rendering/svg/SVGRenderSupport.cpp:
     45        (WebCore::SVGRenderSupport::findTreeRootObject): I do think nothing
     46        guarantees that an SVG renderer has to have an RenderSVGRoot in its
     47        ancestors. So change this function to return a pointer. Also Provide
     48        the non-const version of this function.
     49         
     50        (WebCore::SVGRenderSupport::layoutDifferentRootIfNeeded): Runs the layout
     51        if needed for all the resources which belong to different RenderSVGRoots.
     52       
     53        (WebCore::SVGRenderSupport::layoutChildren): Make sure all the renderer's
     54        resources which belong to different RenderSVGRoots are laid out before
     55        running the layout for this renderer.
     56       
     57        * rendering/svg/SVGRenderSupport.h: Remove a mysterious comment.
     58       
     59        * rendering/svg/SVGResources.cpp:
     60        (WebCore::SVGResources::layoutDifferentRootIfNeeded): Run the layout for
     61        all the resources which belong to different RenderSVGRoots outside the
     62        context of their RenderSVGRoots.
     63       
     64        * rendering/svg/SVGResources.h:
     65        (WebCore::SVGResources::clipper):
     66        (WebCore::SVGResources::markerStart):
     67        (WebCore::SVGResources::markerMid):
     68        (WebCore::SVGResources::markerEnd):
     69        (WebCore::SVGResources::masker):
     70        (WebCore::SVGResources::filter):
     71        (WebCore::SVGResources::fill):
     72        (WebCore::SVGResources::stroke):
     73        Code clean up; use nullptr instead of 0.
     74
    1752016-01-21  Jer Noble  <jer.noble@apple.com>
    276
  • trunk/Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp

    r194496 r195411  
    9595    bool needsLayout = mode == LayoutAndBoundariesInvalidation;
    9696    bool markForInvalidation = mode != ParentOnlyInvalidation;
     97    auto* root = SVGRenderSupport::findTreeRootObject(*this);
    9798
    9899    for (auto* client : m_clients) {
     100        // We should not mark any client outside the current root for invalidation
     101        if (root != SVGRenderSupport::findTreeRootObject(*client))
     102            continue;
     103
    99104        if (is<RenderSVGResourceContainer>(*client)) {
    100105            downcast<RenderSVGResourceContainer>(*client).removeAllClientsFromCache(markForInvalidation);
  • trunk/Source/WebCore/rendering/svg/RenderSVGResourceContainer.h

    r180643 r195411  
    6767    void removeClient(RenderElement&);
    6868
    69 private:
    7069    virtual void willBeDestroyed() override final;
    7170    void registerResource();
  • trunk/Source/WebCore/rendering/svg/RenderSVGRoot.cpp

    r194496 r195411  
    456456void RenderSVGRoot::addResourceForClientInvalidation(RenderSVGResourceContainer* resource)
    457457{
    458     RenderElement* svgRoot = resource->parent();
    459     while (svgRoot && !is<RenderSVGRoot>(*svgRoot))
    460         svgRoot = svgRoot->parent();
     458    RenderSVGRoot* svgRoot = SVGRenderSupport::findTreeRootObject(*resource);
    461459    if (!svgRoot)
    462460        return;
    463     downcast<RenderSVGRoot>(*svgRoot).m_resourcesNeedingToInvalidateClients.add(resource);
    464 }
    465 
    466 }
     461    svgRoot->m_resourcesNeedingToInvalidateClients.add(resource);
     462}
     463
     464}
  • trunk/Source/WebCore/rendering/svg/RenderSVGShape.cpp

    r194819 r195411  
    9090bool RenderSVGShape::isEmpty() const
    9191{
    92     return path().isEmpty();
     92    // This function should never be called before assigning a new Path to m_path.
     93    // But this bug can happen if this renderer was created and its layout was not
     94    // done before painting. Assert this did not happen but do not crash.
     95    ASSERT(hasPath());
     96    return !hasPath() || path().isEmpty();
    9397}
    9498
  • trunk/Source/WebCore/rendering/svg/RenderSVGText.cpp

    r194496 r195411  
    370370        m_needsPositioningValuesUpdate = false;
    371371        updateCachedBoundariesInParents = true;
    372     } else if (m_needsTextMetricsUpdate || SVGRenderSupport::findTreeRootObject(*this).isLayoutSizeChanged()) {
     372    } else if (m_needsTextMetricsUpdate || SVGRenderSupport::findTreeRootObject(*this)->isLayoutSizeChanged()) {
    373373        // If the root layout size changed (eg. window size changes) or the transform to the root
    374374        // context has changed then recompute the on-screen font size.
  • trunk/Source/WebCore/rendering/svg/SVGRenderSupport.cpp

    r192900 r195411  
    185185}
    186186
    187 const RenderSVGRoot& SVGRenderSupport::findTreeRootObject(const RenderElement& start)
    188 {
    189     return *lineageOfType<RenderSVGRoot>(start).first();
     187RenderSVGRoot* SVGRenderSupport::findTreeRootObject(RenderElement& start)
     188{
     189    return lineageOfType<RenderSVGRoot>(start).first();
     190}
     191
     192const RenderSVGRoot* SVGRenderSupport::findTreeRootObject(const RenderElement& start)
     193{
     194    return lineageOfType<RenderSVGRoot>(start).first();
    190195}
    191196
     
    224229
    225230    return false;
     231}
     232
     233void SVGRenderSupport::layoutDifferentRootIfNeeded(const RenderElement& renderer)
     234{
     235    if (auto* resources = SVGResourcesCache::cachedResourcesForRenderer(renderer)) {
     236        auto* svgRoot = SVGRenderSupport::findTreeRootObject(renderer);
     237        ASSERT(svgRoot);
     238        resources->layoutDifferentRootIfNeeded(svgRoot);
     239    }
    226240}
    227241
     
    274288
    275289        if (child->needsLayout()) {
     290            layoutDifferentRootIfNeeded(downcast<RenderElement>(*child));
    276291            downcast<RenderElement>(*child).layout();
    277292            // Renderers are responsible for repainting themselves when changing, except
  • trunk/Source/WebCore/rendering/svg/SVGRenderSupport.h

    r190685 r195411  
    4545class SVGRenderSupport {
    4646public:
     47    static void layoutDifferentRootIfNeeded(const RenderElement&);
     48
    4749    // Shares child layouting code between RenderSVGRoot/RenderSVG(Hidden)Container
    4850    static void layoutChildren(RenderElement&, bool selfNeedsLayout);
     
    9294#endif
    9395
    94     // FIXME: These methods do not belong here.
    95     static const RenderSVGRoot& findTreeRootObject(const RenderElement&);
     96    static RenderSVGRoot* findTreeRootObject(RenderElement&);
     97    static const RenderSVGRoot* findTreeRootObject(const RenderElement&);
    9698
    9799private:
  • trunk/Source/WebCore/rendering/svg/SVGResources.cpp

    r194819 r195411  
    2626#include "RenderSVGResourceMarker.h"
    2727#include "RenderSVGResourceMasker.h"
     28#include "RenderSVGRoot.h"
    2829#include "SVGGradientElement.h"
    2930#include "SVGNames.h"
     
    284285}
    285286
     287void SVGResources::layoutDifferentRootIfNeeded(const RenderSVGRoot* svgRoot)
     288{
     289    if (clipper() && svgRoot != SVGRenderSupport::findTreeRootObject(*clipper()))
     290        clipper()->layoutIfNeeded();
     291
     292    if (masker() && svgRoot != SVGRenderSupport::findTreeRootObject(*masker()))
     293        masker()->layoutIfNeeded();
     294
     295    if (filter() && svgRoot != SVGRenderSupport::findTreeRootObject(*filter()))
     296        filter()->layoutIfNeeded();
     297
     298    if (markerStart() && svgRoot != SVGRenderSupport::findTreeRootObject(*markerStart()))
     299        markerStart()->layoutIfNeeded();
     300
     301    if (markerMid() && svgRoot != SVGRenderSupport::findTreeRootObject(*markerMid()))
     302        markerMid()->layoutIfNeeded();
     303
     304    if (markerEnd() && svgRoot != SVGRenderSupport::findTreeRootObject(*markerEnd()))
     305        markerEnd()->layoutIfNeeded();
     306}
     307
    286308void SVGResources::removeClientFromCache(RenderElement& renderer, bool markForInvalidation) const
    287309{
  • trunk/Source/WebCore/rendering/svg/SVGResources.h

    r188647 r195411  
    3636class RenderSVGResourceMarker;
    3737class RenderSVGResourceMasker;
     38class RenderSVGRoot;
    3839class SVGRenderStyle;
    3940
     
    4546
    4647    bool buildCachedResources(const RenderElement&, const RenderStyle&);
     48    void layoutDifferentRootIfNeeded(const RenderSVGRoot*);
    4749
    4850    // Ordinary resources
    49     RenderSVGResourceClipper* clipper() const { return m_clipperFilterMaskerData ? m_clipperFilterMaskerData->clipper : 0; }
    50     RenderSVGResourceMarker* markerStart() const { return m_markerData ? m_markerData->markerStart : 0; }
    51     RenderSVGResourceMarker* markerMid() const { return m_markerData ? m_markerData->markerMid : 0; }
    52     RenderSVGResourceMarker* markerEnd() const { return m_markerData ? m_markerData->markerEnd : 0; }
    53     RenderSVGResourceMasker* masker() const { return m_clipperFilterMaskerData ? m_clipperFilterMaskerData->masker : 0; }
    54 
    55     RenderSVGResourceFilter* filter() const
    56     {
    57         if (m_clipperFilterMaskerData)
    58             return m_clipperFilterMaskerData->filter;
    59         return 0;
    60     }
     51    RenderSVGResourceClipper* clipper() const { return m_clipperFilterMaskerData ? m_clipperFilterMaskerData->clipper : nullptr; }
     52    RenderSVGResourceMarker* markerStart() const { return m_markerData ? m_markerData->markerStart : nullptr; }
     53    RenderSVGResourceMarker* markerMid() const { return m_markerData ? m_markerData->markerMid : nullptr; }
     54    RenderSVGResourceMarker* markerEnd() const { return m_markerData ? m_markerData->markerEnd : nullptr; }
     55    RenderSVGResourceMasker* masker() const { return m_clipperFilterMaskerData ? m_clipperFilterMaskerData->masker : nullptr; }
     56    RenderSVGResourceFilter* filter() const { return m_clipperFilterMaskerData ? m_clipperFilterMaskerData->filter : nullptr; }
    6157
    6258    // Paint servers
    63     RenderSVGResourceContainer* fill() const { return m_fillStrokeData ? m_fillStrokeData->fill : 0; }
    64     RenderSVGResourceContainer* stroke() const { return m_fillStrokeData ? m_fillStrokeData->stroke : 0; }
     59    RenderSVGResourceContainer* fill() const { return m_fillStrokeData ? m_fillStrokeData->fill : nullptr; }
     60    RenderSVGResourceContainer* stroke() const { return m_fillStrokeData ? m_fillStrokeData->stroke : nullptr; }
    6561
    6662    // Chainable resources - linked through xlink:href
Note: See TracChangeset for help on using the changeset viewer.