Changeset 159218 in webkit


Ignore:
Timestamp:
Nov 13, 2013, 12:03:53 PM (11 years ago)
Author:
Simon Fraser
Message:

ASSERTION FAILED: m_repaintRect == renderer().clippedOverflowRectForRepaint(renderer().containerForRepaint()) after r135816
​https://bugs.webkit.org/show_bug.cgi?id=103432

Reviewed by Dave Hyatt.

RenderLayer caches repaint rects in m_repaintRect, and on updating layer
positions after scrolling, asserts that the cached rect is correct. However,
this assertion would sometimes fail if we were scrolling as a result of
doing adjustViewSize() in the middle of layout, because we haven't updated
layer positions post-layout yet.

Fix by having the poorly named FrameView::repaintFixedElementsAfterScrolling()
skip the layer updating if this FrameView is inside of adjusetViewSize() in
layout.

In order to know if we're inside view size adjusting, add a LayoutPhase
member to FrameView, replacing two existing bools that track laying out state.

Investigative work showed that there are many, many ways to re-enter FrameView::layout(),
which makes it hard (but desirable) to more assertions about state changes, but
indicated that saving and restoring the state (via TemporaryChange<LayoutPhase>)
was a good idea.

  • page/FrameView.cpp:

(WebCore::FrameView::FrameView):
(WebCore::FrameView::reset):
(WebCore::FrameView::updateCompositingLayersAfterStyleChange):
(WebCore::FrameView::layout):
(WebCore::FrameView::repaintFixedElementsAfterScrolling):

  • page/FrameView.h:
Location:
trunk/Source/WebCore
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r159214 r159218  
     12013-11-13  Simon Fraser  <simon.fraser@apple.com>
     2
     3        ASSERTION FAILED: m_repaintRect == renderer().clippedOverflowRectForRepaint(renderer().containerForRepaint()) after r135816
     4        https://bugs.webkit.org/show_bug.cgi?id=103432
     5
     6        Reviewed by Dave Hyatt.
     7
     8        RenderLayer caches repaint rects in m_repaintRect, and on updating layer
     9        positions after scrolling, asserts that the cached rect is correct. However,
     10        this assertion would sometimes fail if we were scrolling as a result of
     11        doing adjustViewSize() in the middle of layout, because we haven't updated
     12        layer positions post-layout yet.
     13       
     14        Fix by having the poorly named FrameView::repaintFixedElementsAfterScrolling()
     15        skip the layer updating if this FrameView is inside of adjusetViewSize() in
     16        layout.
     17       
     18        In order to know if we're inside view size adjusting, add a LayoutPhase
     19        member to FrameView, replacing two existing bools that track laying out state.
     20
     21        Investigative work showed that there are many, many ways to re-enter FrameView::layout(),
     22        which makes it hard (but desirable) to more assertions about state changes, but
     23        indicated that saving and restoring the state (via TemporaryChange<LayoutPhase>)
     24        was a good idea.
     25
     26        * page/FrameView.cpp:
     27        (WebCore::FrameView::FrameView):
     28        (WebCore::FrameView::reset):
     29        (WebCore::FrameView::updateCompositingLayersAfterStyleChange):
     30        (WebCore::FrameView::layout):
     31        (WebCore::FrameView::repaintFixedElementsAfterScrolling):
     32        * page/FrameView.h:
     33
    1342013-11-13  Myles C. Maxfield  <mmaxfield@apple.com>
    235

  • trunk/Source/WebCore/page/FrameView.cpp

    r158655 r159218  
    174174    , m_layoutTimer(this, &FrameView::layoutTimerFired)
    175175    , m_layoutRoot(0)
     176    , m_layoutPhase(OutsideLayout)
    176177    , m_inSynchronousPostLayout(false)
    177178    , m_postLayoutTasksTimer(this, &FrameView::postLayoutTimerFired)
    … …  
    260261    m_needsFullRepaint = true;
    261262    m_layoutSchedulingEnabled = true;
    262     m_inLayout = false;
    263     m_doingPreLayoutStyleUpdate = false;
     263    m_layoutPhase = OutsideLayout;
    264264    m_inSynchronousPostLayout = false;
    265265    m_layoutCount = 0;
    … …  
    732732
    733733    // If we expect to update compositing after an incipient layout, don't do so here.
    734     if (m_doingPreLayoutStyleUpdate || layoutPending() || renderView->needsLayout())
     734    if (inPreLayoutStyleUpdate() || layoutPending() || renderView->needsLayout())
    735735        return;
    736736
    … …  
    10661066void FrameView::layout(bool allowSubtree)
    10671067{
    1068     if (m_inLayout)
    1069         return;
     1068    if (isInLayout())
     1069        return;
     1070
     1071    // Many of the tasks performed during layout can cause this function to be re-entered,
     1072    // so save the layout phase now and restore it on exit.
     1073    TemporaryChange<LayoutPhase> layoutPhaseRestorer(m_layoutPhase, InPreLayout);
    10701074
    10711075    // Protect the view from being deleted during layout (in recalcStyle)
    … …  
    11131117            // This is a new top-level layout. If there are any remaining tasks from the previous
    11141118            // layout, finish them now.
    1115             m_inSynchronousPostLayout = true;
     1119            TemporaryChange<bool> inSynchronousPostLayoutChange(m_inSynchronousPostLayout, true);
    11161120            performPostLayoutTasks();
    1117             m_inSynchronousPostLayout = false;
    1118         }
     1121        }
     1122
     1123        m_layoutPhase = InPreLayoutStyleUpdate;
    11191124
    11201125        // Viewport-dependent media queries may cause us to need completely different style information.
    … …  
    11331138        // Always ensure our style info is up-to-date. This can happen in situations where
    11341139        // the layout beats any sort of style recalc update that needs to occur.
    1135         TemporaryChange<bool> changeDoingPreLayoutStyleUpdate(m_doingPreLayoutStyleUpdate, true);
    11361140        document.updateStyleIfNeeded();
     1141        m_layoutPhase = InPreLayout;
    11371142
    11381143        subtree = m_layoutRoot;
    … …  
    11981203                    else
    11991204                        m_lastViewportSize = visibleContentRect(IncludeScrollbars).size();
     1205
    12001206                    m_lastZoomFactor = root->style().zoom();
    12011207
    … …  
    12281234                }
    12291235            }
     1236
     1237            m_layoutPhase = InPreLayout;
    12301238        }
    12311239
    … …  
    12411249        LayoutStateDisabler layoutStateDisabler(disableLayoutState ? &root->view() : 0);
    12421250
    1243         m_inLayout = true;
     1251        ASSERT(m_layoutPhase == InPreLayout);
     1252        m_layoutPhase = InLayout;
     1253
    12441254        beginDeferredRepaints();
    12451255        forceLayoutParentViewIfNeeded();
     1256
     1257        ASSERT(m_layoutPhase == InLayout);
     1258
    12461259        root->layout();
    12471260#if ENABLE(IOS_TEXT_AUTOSIZING)
    … …  
    12601273#endif
    12611274        endDeferredRepaints();
    1262         m_inLayout = false;
     1275
     1276        ASSERT(m_layoutPhase == InLayout);
    12631277
    12641278        if (subtree)
    … …  
    12701284    }
    12711285
     1286    m_layoutPhase = InViewSizeAdjust;
     1287
    12721288    bool neededFullRepaint = m_needsFullRepaint;
    12731289
    12741290    if (!subtree && !toRenderView(*root).printing())
    12751291        adjustViewSize();
     1292
     1293    m_layoutPhase = InPostLayout;
    12761294
    12771295    m_needsFullRepaint = neededFullRepaint;
    … …  
    13151333                updateWidgetPositions();
    13161334            else {
    1317                 m_inSynchronousPostLayout = true;
     1335                TemporaryChange<bool> inSynchronousPostLayoutChange(m_inSynchronousPostLayout, true);
    13181336                performPostLayoutTasks(); // Calls resumeScheduledEvents().
    1319                 m_inSynchronousPostLayout = false;
    13201337            }
    13211338        }
    … …  
    19481965void FrameView::repaintFixedElementsAfterScrolling()
    19491966{
     1967    // If we're scrolling as a result of updating the view size after layout, we'll update widgets and layer positions soon anyway.
     1968    if (m_layoutPhase == InViewSizeAdjust)
     1969        return;
     1970
    19501971    // For fixed position elements, update widget positions and compositing layers after scrolling,
    19511972    // but only if we're not inside of layout.

  • trunk/Source/WebCore/page/FrameView.h

    r159023 r159218  
    110110    void unscheduleRelayout();
    111111    bool layoutPending() const;
    112     bool isInLayout() const { return m_inLayout; }
     112    bool isInLayout() const { return m_layoutPhase == InLayout; }
    113113
    114114    RenderObject* layoutRoot(bool onlyDuringLayout = false) const;
    … …  
    447447    void init();
    448448
     449    enum LayoutPhase {
     450        OutsideLayout,
     451        InPreLayout,
     452        InPreLayoutStyleUpdate,
     453        InLayout,
     454        InViewSizeAdjust,
     455        InPostLayout,
     456    };
     457    LayoutPhase layoutPhase() const { return m_layoutPhase; }
     458
     459    bool inPreLayoutStyleUpdate() const { return m_layoutPhase == InPreLayoutStyleUpdate; }
     460
    449461    virtual bool isFrameView() const OVERRIDE { return true; }
    450462
    … …  
    564576    bool m_delayedLayout;
    565577    RenderElement* m_layoutRoot;
    566    
     578
     579    LayoutPhase m_layoutPhase;
    567580    bool m_layoutSchedulingEnabled;
    568     bool m_inLayout;
    569     bool m_doingPreLayoutStyleUpdate;
    570581    bool m_inSynchronousPostLayout;
    571582    int m_layoutCount;
Note: See TracChangeset for help on using the changeset viewer.