Reviewed by Darin.

        Fix <rdar://problem/5378214>
        Mail crashes at RenderLayer::paintLayer() when dragging a selection over To Do text

        ObjC interface does not guarantee that Document::updateRendering() gets called after
        modification are made to document. This can lead to situation where paint()
        is invoked with document still dirty which can then crash in number of interesting ways.

        - add hasChangedChild() as needsLayout() condition. layout() will then call recalcStyle()
          catching most cases and making sure document is not dirty when entering painting.
        - protect recalcStyle() and layout() from being executed during painting. There are some
          cases needsLayout() protection does not cover.

        No layout test, these states are very hard or impossible to reach using Javascript interface
        (which generally guarantees that updateRendering() is done right after execution).

        * dom/Document.cpp:
        (WebCore::Document::recalcStyle):
        * page/Frame.cpp:
        (WebCore::Frame::paint):
        (WebCore::Frame::setPaintRestriction):
        (WebCore::Frame::isPainting):
        (WebCore::FramePrivate::FramePrivate):
        * page/Frame.h:
        * page/FramePrivate.h:
        * page/FrameView.cpp:
        (WebCore::FrameView::layout):
        (WebCore::FrameView::needsLayout):

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

diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index 74e34af06e2e483c71747a65f1bce41002dc6de0..42cbd546e09041b5a9dd3e27045c332ca7996142 100644 (file)
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -1,3 +1,35 @@
+2007-08-06  Antti  <antti@apple.com>
+
+        Reviewed by Darin.
+
+        Fix <rdar://problem/5378214>
+        Mail crashes at RenderLayer::paintLayer() when dragging a selection over To Do text
+        
+        ObjC interface does not guarantee that Document::updateRendering() gets called after
+        modification are made to document. This can lead to situation where paint()
+        is invoked with document still dirty which can then crash in number of interesting ways.
+        
+        - add hasChangedChild() as needsLayout() condition. layout() will then call recalcStyle() 
+          catching most cases and making sure document is not dirty when entering painting.
+        - protect recalcStyle() and layout() from being executed during painting. There are some
+          cases needsLayout() protection does not cover.
+        
+        No layout test, these states are very hard or impossible to reach using Javascript interface
+        (which generally guarantees that updateRendering() is done right after execution).
+
+        * dom/Document.cpp:
+        (WebCore::Document::recalcStyle):
+        * page/Frame.cpp:
+        (WebCore::Frame::paint):
+        (WebCore::Frame::setPaintRestriction):
+        (WebCore::Frame::isPainting):
+        (WebCore::FramePrivate::FramePrivate):
+        * page/Frame.h:
+        * page/FramePrivate.h:
+        * page/FrameView.cpp:
+        (WebCore::FrameView::layout):
+        (WebCore::FrameView::needsLayout):
+
 2007-08-05  Maciej Stachowiak  <mjs@apple.com>
 
         Reviewed by Darin Adler.

diff --git a/WebCore/dom/Document.cpp b/WebCore/dom/Document.cpp
index 297cc406bd646be80473e0f55c18ce216784343c..175192047aee5ed0050a1a77dd95a8d7c031a1ef 100644 (file)
--- a/WebCore/dom/Document.cpp
+++ b/WebCore/dom/Document.cpp
@@ -967,6 +967,12 @@ void Document::setDocumentChanged(bool b)
 
 void Document::recalcStyle(StyleChange change)
 {
+    // we should not enter style recalc while painting
+    if (frame() && frame()->isPainting()) {
+        ASSERT(!frame()->isPainting());
+        return;
+    }
+    
     if (m_inStyleRecalc)
         return; // Guard against re-entrancy. -dwh
         

diff --git a/WebCore/page/Frame.cpp b/WebCore/page/Frame.cpp
index 368eb6349e1968ef529933a1e77ed92a0f92a20d..77e231ff11e65ca148a755fc1edfd8cc9939faa8 100644 (file)
--- a/WebCore/page/Frame.cpp
+++ b/WebCore/page/Frame.cpp
@@ -1380,11 +1380,18 @@ void Frame::paint(GraphicsContext* p, const IntRect& rect)
 #endif
     
     if (renderer()) {
+        ASSERT(d->m_view && !d->m_view->needsLayout());
+        ASSERT(!d->m_isPainting);
+        
+        d->m_isPainting = true;
+        
         // d->m_elementToDraw is used to draw only one element
         RenderObject *eltRenderer = d->m_elementToDraw ? d->m_elementToDraw->renderer() : 0;
         if (d->m_paintRestriction == PaintRestrictionNone)
             renderer()->document()->invalidateRenderedRectsForMarkersInRect(rect);
         renderer()->layer()->paint(p, rect, d->m_paintRestriction, eltRenderer);
+        
+        d->m_isPainting = false;
 
         // Regions may have changed as a result of the visibility/z-index of element changing.
         if (renderer()->document()->dashboardRegionsDirty())
@@ -1395,7 +1402,12 @@ void Frame::paint(GraphicsContext* p, const IntRect& rect)
 
 void Frame::setPaintRestriction(PaintRestriction pr)
 {
-  d->m_paintRestriction = pr;
+    d->m_paintRestriction = pr;
+}
+    
+bool Frame::isPainting() const
+{
+    return d->m_isPainting;
 }
 
 void Frame::adjustPageHeight(float *newBottom, float oldTop, float oldBottom, float bottomLimit)
@@ -1980,6 +1992,7 @@ FramePrivate::FramePrivate(Page* page, Frame* parent, Frame* thisFrame, HTMLFram
     , m_caretVisible(false)
     , m_caretPaint(true)
     , m_isActive(false)
+    , m_isPainting(false)
     , m_lifeSupportTimer(thisFrame, &Frame::lifeSupportTimerFired)
     , m_loader(new FrameLoader(thisFrame, frameLoaderClient))
     , m_userStyleSheetLoader(0)

diff --git a/WebCore/page/Frame.h b/WebCore/page/Frame.h
index 60a7e08d7b364442fb046e0c404184ba80ffe75a..3a54238cd4320833a263af23e0005ce7363782e6 100644 (file)
--- a/WebCore/page/Frame.h
+++ b/WebCore/page/Frame.h
@@ -169,6 +169,7 @@ public:
     // should move to FrameView
     void paint(GraphicsContext*, const IntRect&);
     void setPaintRestriction(PaintRestriction);
+    bool isPainting() const;
 
     void setUserStyleSheetLocation(const KURL&);
     void setUserStyleSheet(const String& styleSheetData);

diff --git a/WebCore/page/FramePrivate.h b/WebCore/page/FramePrivate.h
index 42a68a6b94082cde77f167bfe42f7df1c9cca5c8..113c118bc8754a4dc59c1c9cd144433c0adb94b5 100644 (file)
--- a/WebCore/page/FramePrivate.h
+++ b/WebCore/page/FramePrivate.h
@@ -97,6 +97,7 @@ namespace WebCore {
         bool m_caretVisible : 1;
         bool m_caretPaint : 1;
         bool m_isActive : 1;
+        bool m_isPainting : 1;
 
         RefPtr<CSSMutableStyleDeclaration> m_typingStyle;
 

diff --git a/WebCore/page/FrameView.cpp b/WebCore/page/FrameView.cpp
index 730fc7797d4aa616798bf3ddf8fc66c5f16e7de9..d33b9a7910e099a732ffb56224426952573fb38c 100644 (file)
--- a/WebCore/page/FrameView.cpp
+++ b/WebCore/page/FrameView.cpp
@@ -302,6 +302,11 @@ void FrameView::layout(bool allowSubtree)
         m_size.setWidth(visibleWidth());
         return;
     }
+    
+    // we shouldn't enter layout() while painting
+    ASSERT(!m_frame->isPainting());
+    if (m_frame->isPainting())
+        return;
 
     if (!allowSubtree && d->layoutRoot) {
         if (d->layoutRoot->renderer())
@@ -712,7 +717,9 @@ bool FrameView::needsLayout() const
     if (!m_frame)
         return false;
     RenderView* root = static_cast<RenderView*>(m_frame->renderer());
-    return layoutPending() || (root && root->needsLayout()) || d->layoutRoot;
+    Document * doc = m_frame->document();
+    // doc->hasChangedChild() condition can occur when using WebKit ObjC interface
+    return layoutPending() || (root && root->needsLayout()) || d->layoutRoot || (doc && doc->hasChangedChild());
 }
 
 void FrameView::setNeedsLayout()