Reviewed by Darin.

        <rdar://problem/5398301> Xcode threw mutation exception while enumerating subviews (GC only)

        I was never able to reproduce this exception. But there can be cases where layout will
        trigger JavaScript or plugin code that can modify the WebView view hierarchy during a
        recursive enumeration of all the subviews.

        This patch does two things:
        1) Adds a check in debug builds that will LOG when any view is added or removed during layout.
        Noting that added views will not recieve layout this round and might paint without first recieving layout.

        2) Recursivly builds up an array of descendant WebHTMLViews before calling layout on them.
        This matches the behavior of makeObjectsPerformSelector: in the non-GC case (making a copy
        before enumerating.)

        * WebView/WebHTMLView.mm:
        (-[WebHTMLView _web_setPrintingModeRecursive]): Use _web_addDescendantWebHTMLViewsToArray to build up an array
        of WebHTMLViews to enumerate.
        (-[WebHTMLView _web_clearPrintingModeRecursive]): Ditto.
        (-[WebHTMLView _web_setPrintingModeRecursiveAndAdjustViewSize]): Ditto.
        (-[WebHTMLView _web_layoutIfNeededRecursive]): Ditto.
        (-[WebHTMLView _layoutIfNeeded]): Moved to WebHTMLViewFileInternal category.
        (-[WebHTMLView didAddSubview:]): LOG in debug builds.
        (-[WebHTMLView willRemoveSubview:]): Ditto.
        (-[NSView _web_addDescendantWebHTMLViewsToArray:]): Recursivly build an array of descendant WebHTMLViews.
        * WebView/WebHTMLViewInternal.h: Added a BOOL in WebHTMLViewPrivate to track subview changes (debug only.)

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

diff --git a/WebKit/ChangeLog b/WebKit/ChangeLog
index 4e84c06b4310a8b170a9de55e3fdc39c4e010e21..5a219a980677cb359dd56f5fcb2f77eb20a2dedc 100644 (file)
--- a/WebKit/ChangeLog
+++ b/WebKit/ChangeLog
@@ -1,3 +1,33 @@
+2007-08-17  Timothy Hatcher  <timothy@apple.com>
+
+        Reviewed by Darin.
+
+        <rdar://problem/5398301> Xcode threw mutation exception while enumerating subviews (GC only)
+
+        I was never able to reproduce this exception. But there can be cases where layout will
+        trigger JavaScript or plugin code that can modify the WebView view hierarchy during a
+        recursive enumeration of all the subviews.
+
+        This patch does two things:
+        1) Adds a check in debug builds that will LOG when any view is added or removed during layout.
+        Noting that added views will not recieve layout this round and might paint without first recieving layout.
+
+        2) Recursivly builds up an array of descendant WebHTMLViews before calling layout on them.
+        This matches the behavior of makeObjectsPerformSelector: in the non-GC case (making a copy
+        before enumerating.)
+
+        * WebView/WebHTMLView.mm:
+        (-[WebHTMLView _web_setPrintingModeRecursive]): Use _web_addDescendantWebHTMLViewsToArray to build up an array
+        of WebHTMLViews to enumerate.
+        (-[WebHTMLView _web_clearPrintingModeRecursive]): Ditto.
+        (-[WebHTMLView _web_setPrintingModeRecursiveAndAdjustViewSize]): Ditto.
+        (-[WebHTMLView _web_layoutIfNeededRecursive]): Ditto.
+        (-[WebHTMLView _layoutIfNeeded]): Moved to WebHTMLViewFileInternal category.
+        (-[WebHTMLView didAddSubview:]): LOG in debug builds.
+        (-[WebHTMLView willRemoveSubview:]): Ditto.
+        (-[NSView _web_addDescendantWebHTMLViewsToArray:]): Recursivly build an array of descendant WebHTMLViews.
+        * WebView/WebHTMLViewInternal.h: Added a BOOL in WebHTMLViewPrivate to track subview changes (debug only.)
+
 2007-08-17  Anders Carlsson  <andersca@apple.com>
 
         Reviewed by Dave Hyatt.

diff --git a/WebKit/WebView/WebHTMLView.mm b/WebKit/WebView/WebHTMLView.mm
index 9338739b3ba55558d92520b929b63c7f7cdcb09b..fc7f8a37a0297334108e701ec789fe90f6a07dfe 100644 (file)
--- a/WebKit/WebView/WebHTMLView.mm
+++ b/WebKit/WebView/WebHTMLView.mm
@@ -254,6 +254,10 @@ static CachedResourceClient* promisedDataClient()
 - (void)_setMouseDownEvent:(NSEvent *)event;
 - (WebHTMLView *)_topHTMLView;
 - (BOOL)_isTopHTMLView;
+- (void)_web_setPrintingModeRecursive;
+- (void)_web_setPrintingModeRecursiveAndAdjustViewSize;
+- (void)_web_clearPrintingModeRecursive;
+- (void)_web_layoutIfNeededRecursive;
 @end
 
 @interface WebHTMLView (WebForwardDeclaration) // FIXME: Put this in a normal category and stop doing the forward declaration trick.
@@ -270,10 +274,7 @@ static CachedResourceClient* promisedDataClient()
 @end
 
 @interface NSView (WebHTMLViewFileInternal)
-- (void)_web_setPrintingModeRecursive;
-- (void)_web_setPrintingModeRecursiveAndAdjustViewSize;
-- (void)_web_clearPrintingModeRecursive;
-- (void)_web_layoutIfNeededRecursive;
+- (void)_web_addDescendantWebHTMLViewsToArray:(NSMutableArray *) array;
 @end
 
 @interface NSMutableDictionary (WebHTMLViewFileInternal)
@@ -766,6 +767,108 @@ static NSURL* uniqueURLWithRelativePart(NSString *relativePart)
     return self == [self _topHTMLView];
 }
 
+- (void)_web_setPrintingModeRecursive
+{
+    [self _setPrinting:YES minimumPageWidth:0.0f maximumPageWidth:0.0f adjustViewSize:NO];
+
+#ifndef NDEBUG
+    _private->enumeratingSubviews = YES;
+#endif
+
+    NSMutableArray *decendantWebHTMLViews = [[NSMutableArray alloc] init];
+
+    [self _web_addDescendantWebHTMLViewsToArray:decendantWebHTMLViews];
+
+    unsigned count = [decendantWebHTMLViews count];
+    for (unsigned i = 0; i < count; ++i)
+        [[decendantWebHTMLViews objectAtIndex:i] _setPrinting:YES minimumPageWidth:0.0f maximumPageWidth:0.0f adjustViewSize:NO];
+
+    [decendantWebHTMLViews release];
+
+#ifndef NDEBUG
+    _private->enumeratingSubviews = NO;
+#endif
+}
+
+- (void)_web_clearPrintingModeRecursive
+{
+    [self _setPrinting:NO minimumPageWidth:0.0f maximumPageWidth:0.0f adjustViewSize:NO];
+
+#ifndef NDEBUG
+    _private->enumeratingSubviews = YES;
+#endif
+
+    NSMutableArray *decendantWebHTMLViews = [[NSMutableArray alloc] init];
+
+    [self _web_addDescendantWebHTMLViewsToArray:decendantWebHTMLViews];
+
+    unsigned count = [decendantWebHTMLViews count];
+    for (unsigned i = 0; i < count; ++i)
+        [[decendantWebHTMLViews objectAtIndex:i] _setPrinting:NO minimumPageWidth:0.0f maximumPageWidth:0.0f adjustViewSize:NO];
+
+    [decendantWebHTMLViews release];
+
+#ifndef NDEBUG
+    _private->enumeratingSubviews = NO;
+#endif
+}
+
+- (void)_web_setPrintingModeRecursiveAndAdjustViewSize
+{
+    [self _setPrinting:YES minimumPageWidth:0.0f maximumPageWidth:0.0f adjustViewSize:YES];
+
+#ifndef NDEBUG
+    _private->enumeratingSubviews = YES;
+#endif
+
+    NSMutableArray *decendantWebHTMLViews = [[NSMutableArray alloc] init];
+
+    [self _web_addDescendantWebHTMLViewsToArray:decendantWebHTMLViews];
+
+    unsigned count = [decendantWebHTMLViews count];
+    for (unsigned i = 0; i < count; ++i)
+        [[decendantWebHTMLViews objectAtIndex:i] _setPrinting:YES minimumPageWidth:0.0f maximumPageWidth:0.0f adjustViewSize:YES];
+
+    [decendantWebHTMLViews release];
+
+#ifndef NDEBUG
+    _private->enumeratingSubviews = NO;
+#endif
+}
+
+- (void)_layoutIfNeeded
+{
+    ASSERT(!_private->subviewsSetAside);
+
+    if ([[self _bridge] needsLayout])
+        _private->needsLayout = YES;
+    if (_private->needsToApplyStyles || _private->needsLayout)
+        [self layout];
+}
+
+- (void)_web_layoutIfNeededRecursive
+{
+    [self _layoutIfNeeded];
+
+#ifndef NDEBUG
+    _private->enumeratingSubviews = YES;
+#endif
+
+    NSMutableArray *decendantWebHTMLViews = [[NSMutableArray alloc] init];
+
+    [self _web_addDescendantWebHTMLViewsToArray:decendantWebHTMLViews];
+
+    unsigned count = [decendantWebHTMLViews count];
+    for (unsigned i = 0; i < count; ++i)
+        [[decendantWebHTMLViews objectAtIndex:i] _layoutIfNeeded];
+
+    [decendantWebHTMLViews release];
+
+#ifndef NDEBUG
+    _private->enumeratingSubviews = NO;
+#endif
+}
+
 @end
 
 @implementation WebHTMLView (WebPrivate)
@@ -886,6 +989,22 @@ static void _updateMouseoverTimerCallback(CFRunLoopTimerRef timer, void *info)
     _private->subviewsSetAside = NO;
 }
 
+#ifndef NDEBUG
+
+- (void)didAddSubview:(NSView *)subview
+{
+    if (_private->enumeratingSubviews)
+        LOG(View, "A view of class %s was added during subview enumeration for layout or printing mode change. This view might paint without first receiving layout.", object_getClassName([subview class]));
+}
+
+- (void)willRemoveSubview:(NSView *)subview
+{
+    if (_private->enumeratingSubviews)
+        LOG(View, "A view of class %s was removed during subview enumeration for layout or printing mode change. We will still do layout or the printing mode change even though this view is no longer in the view hierarchy.", object_getClassName([subview class]));
+}
+
+#endif
+
 #ifdef BUILDING_ON_TIGER
 
 // This is called when we are about to draw, but before our dirty rect is propagated to our ancestors.
@@ -1374,24 +1493,6 @@ static void _updateMouseoverTimerCallback(CFRunLoopTimerRef timer, void *info)
     return _private->pluginController;
 }
 
-- (void)_web_setPrintingModeRecursive
-{
-    [self _setPrinting:YES minimumPageWidth:0.0f maximumPageWidth:0.0f adjustViewSize:NO];
-    [super _web_setPrintingModeRecursive];
-}
-
-- (void)_web_clearPrintingModeRecursive
-{
-    [self _setPrinting:NO minimumPageWidth:0.0f maximumPageWidth:0.0f adjustViewSize:NO];
-    [super _web_clearPrintingModeRecursive];
-}
-
-- (void)_web_setPrintingModeRecursiveAndAdjustViewSize
-{
-    [self _setPrinting:YES minimumPageWidth:0.0f maximumPageWidth:0.0f adjustViewSize:YES];
-    [super _web_setPrintingModeRecursiveAndAdjustViewSize];
-}
-
 - (void)_layoutForPrinting
 {
     // Set printing mode temporarily so we can adjust the size of the view. This will allow
@@ -1402,22 +1503,6 @@ static void _updateMouseoverTimerCallback(CFRunLoopTimerRef timer, void *info)
     [self _web_clearPrintingModeRecursive];
 }
 
-- (void)_layoutIfNeeded
-{
-    ASSERT(!_private->subviewsSetAside);
-
-    if ([[self _bridge] needsLayout])
-        _private->needsLayout = YES;
-    if (_private->needsToApplyStyles || _private->needsLayout)
-        [self layout];
-}
-
-- (void)_web_layoutIfNeededRecursive
-{
-    [self _layoutIfNeeded];
-    [super _web_layoutIfNeededRecursive];
-}
-
 - (void)_startAutoscrollTimer: (NSEvent *)triggerEvent
 {
     if (_private->autoscrollTimer == nil) {
@@ -1801,24 +1886,15 @@ static void _updateMouseoverTimerCallback(CFRunLoopTimerRef timer, void *info)
 
 @implementation NSView (WebHTMLViewFileInternal)
 
-- (void)_web_setPrintingModeRecursive
-{
-    [_subviews makeObjectsPerformSelector:@selector(_web_setPrintingModeRecursive)];
-}
-
-- (void)_web_setPrintingModeRecursiveAndAdjustViewSize
+- (void)_web_addDescendantWebHTMLViewsToArray:(NSMutableArray *)array
 {
-    [_subviews makeObjectsPerformSelector:@selector(_web_setPrintingModeRecursiveAndAdjustViewSize)];
-}
-
-- (void)_web_clearPrintingModeRecursive
-{
-    [_subviews makeObjectsPerformSelector:@selector(_web_clearPrintingModeRecursive)];
-}
-
-- (void)_web_layoutIfNeededRecursive
-{
-    [_subviews makeObjectsPerformSelector:@selector(_web_layoutIfNeededRecursive)];
+    unsigned count = [_subviews count];
+    for (unsigned i = 0; i < count; ++i) {
+        NSView *child = [_subviews objectAtIndex:i];
+        if ([child isKindOfClass:[WebHTMLView class]])
+            [array addObject:child];
+        [child _web_addDescendantWebHTMLViewsToArray:array];
+    }
 }
 
 @end

diff --git a/WebKit/WebView/WebHTMLViewInternal.h b/WebKit/WebView/WebHTMLViewInternal.h
index 22e006ed541b896ae530aca39d0c06e9457257e2..710b15a49a4480353adc84e44e0b18f1e9e9a888 100644 (file)
--- a/WebKit/WebView/WebHTMLViewInternal.h
+++ b/WebKit/WebView/WebHTMLViewInternal.h
@@ -98,6 +98,10 @@ struct WebHTMLViewInterpretKeyEventsParameters;
     CFRunLoopTimerRef updateMouseoverTimer;
 
     SEL selectorForDoCommandBySelector;
+
+#ifndef NDEBUG
+    BOOL enumeratingSubviews;
+#endif
 }
 - (void)clear;
 @end