Changeset 65681 in webkit


Ignore:
Timestamp:
Aug 19, 2010 10:19:40 AM (14 years ago)
Author:
Simon Fraser
Message:

2010-08-19 Simon Fraser <Simon Fraser>

Reviewed by Nikolas Zimmermann.

HTMLElement::isContentEditable() can cause an updateStyleIfNeeded() to happen in the middle of layout
​https://bugs.webkit.org/show_bug.cgi?id=21834
<rdar://problem/8093653&8261394>

If we're in the middle of layout, or painting, and something causes updateStyleIfNeeded() to
get called, then we can end up entering recalcStyle() during layout or painting. This is bad
because it can create/destry the renderers and RenderLayers which are in use by layout/painting.
This is the cause of a number of random crashers, some of which show up more frequently
in content which uses accelerated compositing.

The changes here:

  1. Add an assertion in Document::updateStyleIfNeeded() that we are not laying out or painting.
  2. Remove calls to updateStyleIfNeeded() in editing and caret painting code
  3. Pass along information to CTM and BBox-related SVG methods to indicate whether it's safe to update style.

Tested by new assertions and existing tests.

  • dom/Document.cpp: (WebCore::Document::updateStyleIfNeeded): New assertion that we are not mid-layout or painting. (WebCore::command): Call updateStyleIfNeeded() to ensure that subsequent calls to isContentEditable() return the correct result.
  • dom/Element.cpp: (WebCore::Element::focus): Move the supportsFocus() call to after style has been updated.
  • editing/SelectionController.cpp: (WebCore::SelectionController::localCaretRect): (WebCore::SelectionController::caretRepaintRect): (WebCore::SelectionController::paintCaret):
  • editing/SelectionController.h: (WebCore::SelectionController::localCaretRectForPainting): When painting, use localCaretRectForPainting() which does not update style. Make localCaretRect() non-const so allowing it to update style without ugly casts.
  • html/HTMLElement.cpp: (WebCore::HTMLElement::isContentEditable): Don't call updateStyleIfNeeded() here. (WebCore::HTMLElement::isContentRichlyEditable): Ditto. (WebCore::HTMLElement::contentEditable): Ditto.
  • page/FrameView.h: (WebCore::FrameView::isMidLayout): New accessor, used for asserting.
  • rendering/RenderPath.cpp: (WebCore::fillAndStrokePath): Pass DisallowStyleUpdate to getScreenCTM since we are painting.
  • rendering/RenderSVGResourceContainer.cpp: (WebCore::RenderSVGResourceContainer::transformOnNonScalingStroke): This is only called when painting, so use DisallowStyleUpdate.
  • svg/SVGElement.cpp: (WebCore::SVGElement::attributeChanged): Changes to the style attribute should not have side effects, since a call to Element::getAttribute() is allowed to result in a call to setAttribute() for the style attribute. To avoid updateStyleIfNeeded() during painting, this must not cause SVG to do extra work.
  • svg/SVGLocatable.cpp: Pass StyleUpdateStrategy down to these methods to indicate whether it's OK to update style. (WebCore::SVGLocatable::getBBox): (WebCore::SVGLocatable::computeCTM): (WebCore::SVGLocatable::getTransformToElement):
  • svg/SVGLocatable.h: (WebCore::SVGLocatable::):
  • svg/SVGStyledLocatableElement.cpp: (WebCore::SVGStyledLocatableElement::getBBox): (WebCore::SVGStyledLocatableElement::getCTM): (WebCore::SVGStyledLocatableElement::getScreenCTM):
  • svg/SVGStyledLocatableElement.h:
  • svg/SVGStyledTransformableElement.cpp: (WebCore::SVGStyledTransformableElement::getCTM): (WebCore::SVGStyledTransformableElement::getScreenCTM): (WebCore::SVGStyledTransformableElement::getBBox):
  • svg/SVGStyledTransformableElement.h:
  • svg/SVGTextElement.cpp: (WebCore::SVGTextElement::getBBox): (WebCore::SVGTextElement::getCTM): (WebCore::SVGTextElement::getScreenCTM):
  • svg/SVGTextElement.h:
Location:
trunk/WebCore
Files:
19 edited

Legend:

Unmodified
Added
Removed

  • trunk/WebCore/ChangeLog

    r65680 r65681  
     12010-08-19  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Reviewed by Nikolas Zimmermann.
     4
     5        HTMLElement::isContentEditable() can cause an updateStyleIfNeeded() to happen in the middle of layout
     6        https://bugs.webkit.org/show_bug.cgi?id=21834
     7        <rdar://problem/8093653&8261394>
     8       
     9        If we're in the middle of layout, or painting, and something causes updateStyleIfNeeded() to
     10        get called, then we can end up entering recalcStyle() during layout or painting. This is bad
     11        because it can create/destry the renderers and RenderLayers which are in use by layout/painting.
     12        This is the cause of a number of random crashers, some of which show up more frequently
     13        in content which uses accelerated compositing.
     14       
     15        The changes here:
     16        1. Add an assertion in Document::updateStyleIfNeeded() that we are not laying out or painting.
     17        2. Remove calls to updateStyleIfNeeded() in editing and caret painting code
     18        3. Pass along information to CTM and BBox-related SVG methods to indicate whether it's safe
     19           to update style.
     20
     21        Tested by new assertions and existing tests.
     22
     23        * dom/Document.cpp:
     24        (WebCore::Document::updateStyleIfNeeded): New assertion that we are not mid-layout or painting.
     25        (WebCore::command): Call updateStyleIfNeeded() to ensure that subsequent calls to isContentEditable()
     26        return the correct result.
     27
     28        * dom/Element.cpp:
     29        (WebCore::Element::focus): Move the supportsFocus() call to after style has been updated.
     30
     31        * editing/SelectionController.cpp:
     32        (WebCore::SelectionController::localCaretRect):
     33        (WebCore::SelectionController::caretRepaintRect):
     34        (WebCore::SelectionController::paintCaret):
     35        * editing/SelectionController.h:
     36        (WebCore::SelectionController::localCaretRectForPainting): When painting, use localCaretRectForPainting()
     37        which does not update style. Make localCaretRect() non-const so allowing it to update style without ugly casts.
     38
     39        * html/HTMLElement.cpp:
     40        (WebCore::HTMLElement::isContentEditable): Don't call updateStyleIfNeeded() here.
     41        (WebCore::HTMLElement::isContentRichlyEditable): Ditto.
     42        (WebCore::HTMLElement::contentEditable): Ditto.
     43
     44        * page/FrameView.h:
     45        (WebCore::FrameView::isMidLayout): New accessor, used for asserting.
     46
     47        * rendering/RenderPath.cpp:
     48        (WebCore::fillAndStrokePath): Pass DisallowStyleUpdate to getScreenCTM since we are painting.
     49        * rendering/RenderSVGResourceContainer.cpp:
     50        (WebCore::RenderSVGResourceContainer::transformOnNonScalingStroke): This is only called when
     51        painting, so use DisallowStyleUpdate.
     52
     53        * svg/SVGElement.cpp:
     54        (WebCore::SVGElement::attributeChanged): Changes to the style attribute should not have
     55        side effects, since a call to Element::getAttribute() is allowed to result in a call to
     56        setAttribute() for the style attribute. To avoid updateStyleIfNeeded() during painting,
     57        this must not cause SVG to do extra work.
     58
     59        * svg/SVGLocatable.cpp: Pass StyleUpdateStrategy down to these methods to indicate
     60        whether it's OK to update style.
     61        (WebCore::SVGLocatable::getBBox):
     62        (WebCore::SVGLocatable::computeCTM):
     63        (WebCore::SVGLocatable::getTransformToElement):
     64        * svg/SVGLocatable.h:
     65        (WebCore::SVGLocatable::):
     66        * svg/SVGStyledLocatableElement.cpp:
     67        (WebCore::SVGStyledLocatableElement::getBBox):
     68        (WebCore::SVGStyledLocatableElement::getCTM):
     69        (WebCore::SVGStyledLocatableElement::getScreenCTM):
     70        * svg/SVGStyledLocatableElement.h:
     71        * svg/SVGStyledTransformableElement.cpp:
     72        (WebCore::SVGStyledTransformableElement::getCTM):
     73        (WebCore::SVGStyledTransformableElement::getScreenCTM):
     74        (WebCore::SVGStyledTransformableElement::getBBox):
     75        * svg/SVGStyledTransformableElement.h:
     76        * svg/SVGTextElement.cpp:
     77        (WebCore::SVGTextElement::getBBox):
     78        (WebCore::SVGTextElement::getCTM):
     79        (WebCore::SVGTextElement::getScreenCTM):
     80        * svg/SVGTextElement.h:
     81
    1822010-08-19  Ryosuke Niwa  <rniwa@webkit.org>
    283

  • trunk/WebCore/dom/Document.cpp

    r65468 r65681  
    14771477void Document::updateStyleIfNeeded()
    14781478{
     1479    ASSERT(!view() || (!view()->isInLayout() && !view()->isPainting()));
     1480   
    14791481    if (!childNeedsStyleRecalc() || inPageCache())
    14801482        return;
    … …  
    37643766    if (!frame || frame->document() != document)
    37653767        return Editor::Command();
     3768
     3769    document->updateStyleIfNeeded();
     3770
    37663771    return frame->editor()->command(commandName,
    37673772        userInterface ? CommandFromDOMWithUserInterface : CommandFromDOM);

  • trunk/WebCore/dom/Element.cpp

    r65372 r65681  
    12941294        return;
    12951295
    1296     if (!supportsFocus())
    1297         return;
    1298 
    12991296    // If the stylesheets have already been loaded we can reliably check isFocusable.
    13001297    // If not, we continue and set the focused node on the focus controller below so
    … …  
    13051302            return;
    13061303    }
     1304
     1305    if (!supportsFocus())
     1306        return;
    13071307
    13081308    RefPtr<Node> protect;

  • trunk/WebCore/editing/SelectionController.cpp

    r62873 r65681  
    953953}
    954954
    955 IntRect SelectionController::localCaretRect() const
     955IntRect SelectionController::localCaretRect()
    956956{
    957957    if (m_needsLayout)
    958         const_cast<SelectionController*>(this)->layout();
     958        layout();
    959959   
    960960    return m_caretRect;
    … …  
    988988IntRect SelectionController::caretRepaintRect() const
    989989{
    990     return absoluteBoundsForLocalRect(repaintRectForCaret(localCaretRect()));
     990    return absoluteBoundsForLocalRect(repaintRectForCaret(localCaretRectForPainting()));
    991991}
    992992
    … …  
    10801080        return;
    10811081
    1082     IntRect drawingRect = localCaretRect();
     1082    IntRect drawingRect = localCaretRectForPainting();
    10831083    drawingRect.move(tx, ty);
    10841084    IntRect caret = intersection(drawingRect, clipRect);

  • trunk/WebCore/editing/SelectionController.h

    r62816 r65681  
    101101
    102102    // Caret rect local to the caret's renderer
    103     IntRect localCaretRect() const;
     103    IntRect localCaretRect();
     104    IntRect localCaretRectForPainting() const { return m_caretRect; }
     105
    104106    // Bounds of (possibly transformed) caret in absolute coords
    105107    IntRect absoluteCaretBounds();

  • trunk/WebCore/html/HTMLElement.cpp

    r65372 r65681  
    667667        return true;
    668668
    669     // FIXME: this is a terrible thing to do here:
    670     // https://bugs.webkit.org/show_bug.cgi?id=21834
    671     document()->updateStyleIfNeeded();
     669    // Ideally we'd call ASSERT!needsStyleRecalc()) here, but
     670    // ContainerNode::setFocus() calls setNeedsStyleRecalc(), so the assertion
     671    // would fire in the middle of Document::setFocusedNode().
    672672
    673673    if (!renderer()) {
    … …  
    686686        return true;
    687687
    688     document()->updateStyleIfNeeded();
    689 
    690688    if (!renderer()) {
    691689        if (parentNode())
    … …  
    700698String HTMLElement::contentEditable() const
    701699{
    702     document()->updateStyleIfNeeded();
    703 
    704700    if (!renderer())
    705701        return "false";

  • trunk/WebCore/page/FrameView.cpp

    r65534 r65681  
    204204    m_doFullRepaint = true;
    205205    m_layoutSchedulingEnabled = true;
    206     m_midLayout = false;
     206    m_inLayout = false;
    207207    m_layoutCount = 0;
    208208    m_nestedLayoutCount = 0;
    … …  
    601601void FrameView::layout(bool allowSubtree)
    602602{
    603     if (m_midLayout)
     603    if (m_inLayout)
    604604        return;
    605605
    … …  
    773773    }
    774774       
    775     m_midLayout = true;
     775    m_inLayout = true;
    776776    beginDeferredRepaints();
    777777    root->layout();
    778778    endDeferredRepaints();
    779     m_midLayout = false;
     779    m_inLayout = false;
    780780
    781781    if (subtree) {

  • trunk/WebCore/page/FrameView.h

    r65021 r65681  
    8989    void unscheduleRelayout();
    9090    bool layoutPending() const;
     91    bool isInLayout() const { return m_inLayout; }
    9192
    9293    RenderObject* layoutRoot(bool onlyDuringLayout = false) const;
    … …  
    318319   
    319320    bool m_layoutSchedulingEnabled;
    320     bool m_midLayout;
     321    bool m_inLayout;
    321322    int m_layoutCount;
    322323    unsigned m_nestedLayoutCount;

  • trunk/WebCore/rendering/RenderPath.cpp

    r64830 r65681  
    142142        if (style->svgStyle()->vectorEffect() == VE_NON_SCALING_STROKE) {
    143143            SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(object->node());
    144             AffineTransform transform = element->getScreenCTM();
     144            AffineTransform transform = element->getScreenCTM(SVGLocatable::DisallowStyleUpdate);
    145145            if (!transform.isInvertible())
    146146                return;

  • trunk/WebCore/rendering/RenderSVGResourceContainer.cpp

    r65310 r65681  
    185185    SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(object->node());
    186186    AffineTransform transform = resourceTransform;
    187     transform.multiply(element->getScreenCTM());
     187    transform.multiply(element->getScreenCTM(SVGLocatable::DisallowStyleUpdate));
    188188    return transform;
    189189}

  • trunk/WebCore/svg/SVGElement.cpp

    r64579 r65681  
    313313        return;
    314314
    315     svgAttributeChanged(attr->name());
     315    // Changes to the style attribute are processed lazily (see Element::getAttribute() and related methods),
     316    // so we don't want changes to the style attribute to result in extra work here.
     317    if (attr->name() != styleAttr)
     318        svgAttributeChanged(attr->name());
    316319}
    317320

  • trunk/WebCore/svg/SVGLocatable.cpp

    r64579 r65681  
    7272}
    7373
    74 FloatRect SVGLocatable::getBBox(const SVGElement* element)
     74FloatRect SVGLocatable::getBBox(const SVGElement* element, StyleUpdateStrategy styleUpdateStrategy)
    7575{
    7676    ASSERT(element);
    77     element->document()->updateLayoutIgnorePendingStylesheets();
     77    if (styleUpdateStrategy == AllowStyleUpdate)
     78        element->document()->updateLayoutIgnorePendingStylesheets();
    7879
    7980    // FIXME: Eventually we should support getBBox for detached elements.
    … …  
    8485}
    8586
    86 AffineTransform SVGLocatable::computeCTM(const SVGElement* element, CTMScope mode)
     87AffineTransform SVGLocatable::computeCTM(const SVGElement* element, CTMScope mode, StyleUpdateStrategy styleUpdateStrategy)
    8788{
    8889    ASSERT(element);
    89     element->document()->updateLayoutIgnorePendingStylesheets();
     90    if (styleUpdateStrategy == AllowStyleUpdate)
     91        element->document()->updateLayoutIgnorePendingStylesheets();
    9092
    9193    AffineTransform ctm;
    … …  
    109111}
    110112
    111 AffineTransform SVGLocatable::getTransformToElement(SVGElement* target, ExceptionCode& ec) const
     113AffineTransform SVGLocatable::getTransformToElement(SVGElement* target, ExceptionCode& ec, StyleUpdateStrategy styleUpdateStrategy) const
    112114{
    113     AffineTransform ctm = getCTM();
     115    AffineTransform ctm = getCTM(styleUpdateStrategy);
    114116
    115117    if (target && target->isStyledLocatable()) {
    116         AffineTransform targetCTM = static_cast<SVGStyledLocatableElement*>(target)->getCTM();
     118        AffineTransform targetCTM = static_cast<SVGStyledLocatableElement*>(target)->getCTM(styleUpdateStrategy);
    117119        if (!targetCTM.isInvertible()) {
    118120            ec = SVGException::SVG_MATRIX_NOT_INVERTABLE;

  • trunk/WebCore/svg/SVGLocatable.h

    r64579 r65681  
    4141    virtual SVGElement* farthestViewportElement() const = 0;
    4242
    43     virtual FloatRect getBBox() const = 0;
    44     virtual AffineTransform getCTM() const = 0;
    45     virtual AffineTransform getScreenCTM() const = 0;
    46     AffineTransform getTransformToElement(SVGElement*, ExceptionCode&) const;
     43    enum StyleUpdateStrategy { AllowStyleUpdate, DisallowStyleUpdate };
     44   
     45    virtual FloatRect getBBox(StyleUpdateStrategy) const = 0;
     46    virtual AffineTransform getCTM(StyleUpdateStrategy) const = 0;
     47    virtual AffineTransform getScreenCTM(StyleUpdateStrategy) const = 0;
     48    AffineTransform getTransformToElement(SVGElement*, ExceptionCode&, StyleUpdateStrategy = AllowStyleUpdate) const;
    4749
    4850    static SVGElement* nearestViewportElement(const SVGElement*);
    … …  
    5759    virtual AffineTransform localCoordinateSpaceTransform(SVGLocatable::CTMScope) const { return AffineTransform(); }
    5860
    59     static FloatRect getBBox(const SVGElement*);
    60     static AffineTransform computeCTM(const SVGElement*, CTMScope);
     61    static FloatRect getBBox(const SVGElement*, StyleUpdateStrategy);
     62    static AffineTransform computeCTM(const SVGElement*, CTMScope, StyleUpdateStrategy);
    6163};
    6264

  • trunk/WebCore/svg/SVGStyledLocatableElement.cpp

    r64579 r65681  
    5151}
    5252
    53 FloatRect SVGStyledLocatableElement::getBBox() const
     53FloatRect SVGStyledLocatableElement::getBBox(StyleUpdateStrategy styleUpdateStrategy) const
    5454{
    55     return SVGLocatable::getBBox(this);
     55    return SVGLocatable::getBBox(this, styleUpdateStrategy);
    5656}
    5757
    58 AffineTransform SVGStyledLocatableElement::getCTM() const
     58AffineTransform SVGStyledLocatableElement::getCTM(StyleUpdateStrategy styleUpdateStrategy) const
    5959{
    60     return SVGLocatable::computeCTM(this, SVGLocatable::NearestViewportScope);
     60    return SVGLocatable::computeCTM(this, SVGLocatable::NearestViewportScope, styleUpdateStrategy);
    6161}
    6262
    63 AffineTransform SVGStyledLocatableElement::getScreenCTM() const
     63AffineTransform SVGStyledLocatableElement::getScreenCTM(StyleUpdateStrategy styleUpdateStrategy) const
    6464{
    65     return SVGLocatable::computeCTM(this, SVGLocatable::ScreenScope);
     65    return SVGLocatable::computeCTM(this, SVGLocatable::ScreenScope, styleUpdateStrategy);
    6666}
    6767

  • trunk/WebCore/svg/SVGStyledLocatableElement.h

    r64579 r65681  
    4141        virtual SVGElement* farthestViewportElement() const;
    4242
    43         virtual FloatRect getBBox() const;
    44         virtual AffineTransform getCTM() const;
    45         virtual AffineTransform getScreenCTM() const;
     43        virtual FloatRect getBBox(StyleUpdateStrategy = AllowStyleUpdate) const;
     44        virtual AffineTransform getCTM(StyleUpdateStrategy = AllowStyleUpdate) const;
     45        virtual AffineTransform getScreenCTM(StyleUpdateStrategy = AllowStyleUpdate) const;
    4646
    4747        virtual AffineTransform localCoordinateSpaceTransform(SVGLocatable::CTMScope mode) const { return SVGLocatable::localCoordinateSpaceTransform(mode); }

  • trunk/WebCore/svg/SVGStyledTransformableElement.cpp

    r64579 r65681  
    4444}
    4545
    46 AffineTransform SVGStyledTransformableElement::getCTM() const
     46AffineTransform SVGStyledTransformableElement::getCTM(StyleUpdateStrategy styleUpdateStrategy) const
    4747{
    48     return SVGLocatable::computeCTM(this, SVGLocatable::NearestViewportScope);
     48    return SVGLocatable::computeCTM(this, SVGLocatable::NearestViewportScope, styleUpdateStrategy);
    4949}
    5050
    51 AffineTransform SVGStyledTransformableElement::getScreenCTM() const
     51AffineTransform SVGStyledTransformableElement::getScreenCTM(StyleUpdateStrategy styleUpdateStrategy) const
    5252{
    53     return SVGLocatable::computeCTM(this, SVGLocatable::ScreenScope);
     53    return SVGLocatable::computeCTM(this, SVGLocatable::ScreenScope, styleUpdateStrategy);
    5454}
    5555
    … …  
    102102}
    103103
    104 FloatRect SVGStyledTransformableElement::getBBox() const
     104FloatRect SVGStyledTransformableElement::getBBox(StyleUpdateStrategy styleUpdateStrategy) const
    105105{
    106     return SVGTransformable::getBBox(this);
     106    return SVGTransformable::getBBox(this, styleUpdateStrategy);
    107107}
    108108

  • trunk/WebCore/svg/SVGStyledTransformableElement.h

    r64579 r65681  
    3939    virtual bool isStyledTransformable() const { return true; }
    4040
    41     virtual AffineTransform getCTM() const;
    42     virtual AffineTransform getScreenCTM() const;
     41    virtual AffineTransform getCTM(StyleUpdateStrategy = AllowStyleUpdate) const;
     42    virtual AffineTransform getScreenCTM(StyleUpdateStrategy = AllowStyleUpdate) const;
    4343    virtual SVGElement* nearestViewportElement() const;
    4444    virtual SVGElement* farthestViewportElement() const;
    … …  
    4848    virtual AffineTransform* supplementalTransform();
    4949
    50     virtual FloatRect getBBox() const;
     50    virtual FloatRect getBBox(StyleUpdateStrategy = AllowStyleUpdate) const;
    5151
    5252    virtual void parseMappedAttribute(Attribute*);

  • trunk/WebCore/svg/SVGTextElement.cpp

    r64579 r65681  
    6969}
    7070
    71 FloatRect SVGTextElement::getBBox() const
     71FloatRect SVGTextElement::getBBox(StyleUpdateStrategy styleUpdateStrategy) const
    7272{
    73     return SVGTransformable::getBBox(this);
     73    return SVGTransformable::getBBox(this, styleUpdateStrategy);
    7474}
    7575
    76 AffineTransform SVGTextElement::getCTM() const
     76AffineTransform SVGTextElement::getCTM(StyleUpdateStrategy styleUpdateStrategy) const
    7777{
    78     return SVGLocatable::computeCTM(this, SVGLocatable::NearestViewportScope);
     78    return SVGLocatable::computeCTM(this, SVGLocatable::NearestViewportScope, styleUpdateStrategy);
    7979}
    8080
    81 AffineTransform SVGTextElement::getScreenCTM() const
     81AffineTransform SVGTextElement::getScreenCTM(StyleUpdateStrategy styleUpdateStrategy) const
    8282{
    83     return SVGLocatable::computeCTM(this, SVGLocatable::ScreenScope);
     83    return SVGLocatable::computeCTM(this, SVGLocatable::ScreenScope, styleUpdateStrategy);
    8484}
    8585

  • trunk/WebCore/svg/SVGTextElement.h

    r64579 r65681  
    3939        virtual SVGElement* farthestViewportElement() const;
    4040
    41         virtual FloatRect getBBox() const;
    42         virtual AffineTransform getCTM() const;
    43         virtual AffineTransform getScreenCTM() const;
     41        virtual FloatRect getBBox(StyleUpdateStrategy = AllowStyleUpdate) const;
     42        virtual AffineTransform getCTM(StyleUpdateStrategy = AllowStyleUpdate) const;
     43        virtual AffineTransform getScreenCTM(StyleUpdateStrategy = AllowStyleUpdate) const;
    4444        virtual AffineTransform animatedLocalTransform() const;
    4545        virtual AffineTransform* supplementalTransform();
Note: See TracChangeset for help on using the changeset viewer.