[webkit-changes] [261256] trunk/Source/WebCore

Wed, 06 May 2020 15:57:08 -0700

Title: [261256] trunk/Source/WebCore
Revision
261256
Author
simon.fra...@apple.com
Date
2020-05-06 15:56:01 -0700 (Wed, 06 May 2020)

Log Message

REGRESSION (r261056): [ Mac WK1 ] inspector/console/console-api.html is flaky crashing
https://bugs.webkit.org/show_bug.cgi?id=211386

Reviewed by Tim Horton.

This bug was caused by the failure to clear the delegate on an NSScrollerImp when, for testing,
we flip between the native scrollbar theme, and the mock scrollbar theme.

The crux of the fix is to have ScrollAnimatorMac's scrollerImpForScrollbar() call a
static function on ScrollbarThemeMac to get the painters, rather than going through
the possibly-null ScrollbarThemeMac instance.

A belt-and-braces fix in ScrollbarThemeMac::unregisterScrollbar() always clears the delegate
on the NSScrollerImp when unregistering a scrollbar.

Finally, modernize code in various places.

* platform/mac/ScrollAnimatorMac.mm:
(WebCore::scrollerImpForScrollbar):
(WebCore::ScrollAnimatorMac::updateScrollerStyle):
* platform/mac/ScrollbarThemeMac.h:
* platform/mac/ScrollbarThemeMac.mm:
(WebCore::scrollbarMap):
(+[WebScrollbarPrefsObserver appearancePrefsChanged:]):
(WebCore::ScrollbarThemeMac::registerScrollbar):
(WebCore::ScrollbarThemeMac::unregisterScrollbar):
(WebCore::ScrollbarThemeMac::setNewPainterForScrollbar):
(WebCore::ScrollbarThemeMac::painterForScrollbar):
(WebCore::ScrollbarThemeMac::hasThumb):
(WebCore::ScrollbarThemeMac::minimumThumbLength):
(WebCore::ScrollbarThemeMac::updateEnabledState):
(WebCore::ScrollbarThemeMac::paint):

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (261255 => 261256)


--- trunk/Source/WebCore/ChangeLog	2020-05-06 22:55:30 UTC (rev 261255)
+++ trunk/Source/WebCore/ChangeLog	2020-05-06 22:56:01 UTC (rev 261256)
@@ -1,3 +1,38 @@
+2020-05-06  Simon Fraser  <simon.fra...@apple.com>
+
+        REGRESSION (r261056): [ Mac WK1 ] inspector/console/console-api.html is flaky crashing
+        https://bugs.webkit.org/show_bug.cgi?id=211386
+
+        Reviewed by Tim Horton.
+
+        This bug was caused by the failure to clear the delegate on an NSScrollerImp when, for testing,
+        we flip between the native scrollbar theme, and the mock scrollbar theme.
+
+        The crux of the fix is to have ScrollAnimatorMac's scrollerImpForScrollbar() call a
+        static function on ScrollbarThemeMac to get the painters, rather than going through
+        the possibly-null ScrollbarThemeMac instance.
+
+        A belt-and-braces fix in ScrollbarThemeMac::unregisterScrollbar() always clears the delegate
+        on the NSScrollerImp when unregistering a scrollbar.
+
+        Finally, modernize code in various places.
+
+        * platform/mac/ScrollAnimatorMac.mm:
+        (WebCore::scrollerImpForScrollbar):
+        (WebCore::ScrollAnimatorMac::updateScrollerStyle):
+        * platform/mac/ScrollbarThemeMac.h:
+        * platform/mac/ScrollbarThemeMac.mm:
+        (WebCore::scrollbarMap):
+        (+[WebScrollbarPrefsObserver appearancePrefsChanged:]):
+        (WebCore::ScrollbarThemeMac::registerScrollbar):
+        (WebCore::ScrollbarThemeMac::unregisterScrollbar):
+        (WebCore::ScrollbarThemeMac::setNewPainterForScrollbar):
+        (WebCore::ScrollbarThemeMac::painterForScrollbar):
+        (WebCore::ScrollbarThemeMac::hasThumb):
+        (WebCore::ScrollbarThemeMac::minimumThumbLength):
+        (WebCore::ScrollbarThemeMac::updateEnabledState):
+        (WebCore::ScrollbarThemeMac::paint):
+
 2020-05-06  Jack Lee  <shihchieh_...@apple.com>
 
         Nullptr crash in InsertListCommand::doApply with user-select:none elements

Modified: trunk/Source/WebCore/platform/mac/ScrollAnimatorMac.mm (261255 => 261256)


--- trunk/Source/WebCore/platform/mac/ScrollAnimatorMac.mm	2020-05-06 22:55:30 UTC (rev 261255)
+++ trunk/Source/WebCore/platform/mac/ScrollAnimatorMac.mm	2020-05-06 22:56:01 UTC (rev 261256)
@@ -52,10 +52,7 @@
 
 static NSScrollerImp *scrollerImpForScrollbar(Scrollbar& scrollbar)
 {
-    if (ScrollbarThemeMac* scrollbarTheme = macScrollbarTheme())
-        return scrollbarTheme->painterForScrollbar(scrollbar);
-
-    return nil;
+    return ScrollbarThemeMac::painterForScrollbar(scrollbar);
 }
 
 } // namespace WebCore
@@ -1464,10 +1461,10 @@
         verticalScrollbar->invalidate();
 
         NSScrollerImp *oldVerticalPainter = [m_scrollerImpPair verticalScrollerImp];
-        NSScrollerImp *newVerticalPainter = [NSScrollerImp scrollerImpWithStyle:newStyle controlSize:(NSControlSize)verticalScrollbar->controlSize() horizontal:NO replacingScrollerImp:oldVerticalPainter];
+        auto newVerticalPainter = retainPtr([NSScrollerImp scrollerImpWithStyle:newStyle controlSize:(NSControlSize)verticalScrollbar->controlSize() horizontal:NO replacingScrollerImp:oldVerticalPainter]);
 
-        [m_scrollerImpPair setVerticalScrollerImp:newVerticalPainter];
-        macTheme->setNewPainterForScrollbar(*verticalScrollbar, newVerticalPainter);
+        [m_scrollerImpPair setVerticalScrollerImp:newVerticalPainter.get()];
+        macTheme->setNewPainterForScrollbar(*verticalScrollbar, WTFMove(newVerticalPainter));
         macTheme->didCreateScrollerImp(*verticalScrollbar);
 
         // The different scrollbar styles have different thicknesses, so we must re-set the 
@@ -1481,10 +1478,10 @@
         horizontalScrollbar->invalidate();
 
         NSScrollerImp *oldHorizontalPainter = [m_scrollerImpPair horizontalScrollerImp];
-        NSScrollerImp *newHorizontalPainter = [NSScrollerImp scrollerImpWithStyle:newStyle controlSize:(NSControlSize)horizontalScrollbar->controlSize() horizontal:YES replacingScrollerImp:oldHorizontalPainter];
+        auto newHorizontalPainter = retainPtr([NSScrollerImp scrollerImpWithStyle:newStyle controlSize:(NSControlSize)horizontalScrollbar->controlSize() horizontal:YES replacingScrollerImp:oldHorizontalPainter]);
 
-        [m_scrollerImpPair setHorizontalScrollerImp:newHorizontalPainter];
-        macTheme->setNewPainterForScrollbar(*horizontalScrollbar, newHorizontalPainter);
+        [m_scrollerImpPair setHorizontalScrollerImp:newHorizontalPainter.get()];
+        macTheme->setNewPainterForScrollbar(*horizontalScrollbar, WTFMove(newHorizontalPainter));
         macTheme->didCreateScrollerImp(*horizontalScrollbar);
 
         // The different scrollbar styles have different thicknesses, so we must re-set the

Modified: trunk/Source/WebCore/platform/mac/ScrollbarThemeMac.h (261255 => 261256)


--- trunk/Source/WebCore/platform/mac/ScrollbarThemeMac.h	2020-05-06 22:55:30 UTC (rev 261255)
+++ trunk/Source/WebCore/platform/mac/ScrollbarThemeMac.h	2020-05-06 22:56:01 UTC (rev 261256)
@@ -60,8 +60,8 @@
     void registerScrollbar(Scrollbar&) override;
     void unregisterScrollbar(Scrollbar&) override;
 
-    void setNewPainterForScrollbar(Scrollbar&, NSScrollerImp *);
-    NSScrollerImp *painterForScrollbar(Scrollbar&);
+    void setNewPainterForScrollbar(Scrollbar&, RetainPtr<NSScrollerImp>&&);
+    static NSScrollerImp *painterForScrollbar(Scrollbar&);
 
     void setPaintCharacteristicsForScrollbar(Scrollbar&);

Modified: trunk/Source/WebCore/platform/mac/ScrollbarThemeMac.mm (261255 => 261256)


--- trunk/Source/WebCore/platform/mac/ScrollbarThemeMac.mm	2020-05-06 22:55:30 UTC (rev 261255)
+++ trunk/Source/WebCore/platform/mac/ScrollbarThemeMac.mm	2020-05-06 22:56:01 UTC (rev 261256)
@@ -51,20 +51,21 @@
 
 namespace WebCore {
 
-    typedef HashMap<Scrollbar*, RetainPtr<NSScrollerImp>> ScrollerImpMap;
+using ScrollbarToScrollerImpMap = HashMap<Scrollbar*, RetainPtr<NSScrollerImp>>;
 
-    static ScrollerImpMap* scrollbarMap()
-    {
-        static ScrollerImpMap* map = new ScrollerImpMap;
-        return map;
-    }
-
+static ScrollbarToScrollerImpMap& scrollbarMap()
+{
+    static NeverDestroyed<ScrollbarToScrollerImpMap> instances;
+    return instances;
 }
 
+} // namespace WebCore
+
 using WebCore::ScrollbarTheme;
 using WebCore::ScrollbarThemeMac;
 using WebCore::scrollbarMap;
-using WebCore::ScrollerImpMap;
+using WebCore::ScrollbarToScrollerImpMap;
+
 @interface NSColor (WebNSColorDetails)
 + (NSImage *)_linenPatternImage;
 @end
@@ -90,12 +91,11 @@
         return;
 
     static_cast<ScrollbarThemeMac&>(theme).preferencesChanged();
-    if (scrollbarMap()->isEmpty())
-        return;
-    ScrollerImpMap::iterator end = scrollbarMap()->end();
-    for (ScrollerImpMap::iterator it = scrollbarMap()->begin(); it != end; ++it) {
-        it->key->styleChanged();
-        it->key->invalidate();
+
+    for (auto keyValuePair : scrollbarMap()) {
+        auto* scrollbar = keyValuePair.key;
+        scrollbar->styleChanged();
+        scrollbar->invalidate();
     }
 }
 
@@ -171,8 +171,8 @@
         return;
 
     bool isHorizontal = scrollbar.orientation() == HorizontalScrollbar;
-    NSScrollerImp *scrollerImp = [NSScrollerImp scrollerImpWithStyle:ScrollerStyle::recommendedScrollerStyle() controlSize:scrollbarControlSizeToNSControlSize(scrollbar.controlSize()) horizontal:isHorizontal replacingScrollerImp:nil];
-    scrollbarMap()->add(&scrollbar, scrollerImp);
+    auto scrollerImp = retainPtr([NSScrollerImp scrollerImpWithStyle:ScrollerStyle::recommendedScrollerStyle() controlSize:scrollbarControlSizeToNSControlSize(scrollbar.controlSize()) horizontal:isHorizontal replacingScrollerImp:nil]);
+    scrollbarMap().add(&scrollbar, WTFMove(scrollerImp));
     didCreateScrollerImp(scrollbar);
     updateEnabledState(scrollbar);
     updateScrollbarOverlayStyle(scrollbar);
@@ -180,12 +180,16 @@
 
 void ScrollbarThemeMac::unregisterScrollbar(Scrollbar& scrollbar)
 {
-    scrollbarMap()->remove(&scrollbar);
+    auto iter = scrollbarMap().find(&scrollbar);
+    if (iter != scrollbarMap().end()) {
+        [iter->value setDelegate:nil];
+        scrollbarMap().remove(iter);
+    }
 }
 
-void ScrollbarThemeMac::setNewPainterForScrollbar(Scrollbar& scrollbar, NSScrollerImp *newPainter)
+void ScrollbarThemeMac::setNewPainterForScrollbar(Scrollbar& scrollbar, RetainPtr<NSScrollerImp>&& newPainter)
 {
-    scrollbarMap()->set(&scrollbar, newPainter);
+    scrollbarMap().set(&scrollbar, WTFMove(newPainter));
     updateEnabledState(scrollbar);
     updateScrollbarOverlayStyle(scrollbar);
 }
@@ -192,7 +196,7 @@
 
 NSScrollerImp *ScrollbarThemeMac::painterForScrollbar(Scrollbar& scrollbar)
 {
-    return scrollbarMap()->get(&scrollbar).get();
+    return scrollbarMap().get(&scrollbar).get();
 }
 
 bool ScrollbarThemeMac::isLayoutDirectionRTL(Scrollbar& scrollbar)
@@ -296,7 +300,7 @@
 {
     int minLengthForThumb;
 
-    NSScrollerImp *painter = scrollbarMap()->get(&scrollbar).get();
+    NSScrollerImp *painter = scrollbarMap().get(&scrollbar).get();
     minLengthForThumb = [painter knobMinLength] + [painter trackOverlapEndInset] + [painter knobOverlapEndInset]
         + 2 * ([painter trackEndInset] + [painter knobEndInset]);
 
@@ -437,7 +441,7 @@
 int ScrollbarThemeMac::minimumThumbLength(Scrollbar& scrollbar)
 {
     BEGIN_BLOCK_OBJC_EXCEPTIONS;
-    return [scrollbarMap()->get(&scrollbar) knobMinLength];
+    return [scrollbarMap().get(&scrollbar) knobMinLength];
     END_BLOCK_OBJC_EXCEPTIONS;
 }
 
@@ -496,7 +500,7 @@
 void ScrollbarThemeMac::updateEnabledState(Scrollbar& scrollbar)
 {
     BEGIN_BLOCK_OBJC_EXCEPTIONS;
-    [scrollbarMap()->get(&scrollbar) setEnabled:scrollbar.enabled()];
+    [scrollbarMap().get(&scrollbar) setEnabled:scrollbar.enabled()];
     END_BLOCK_OBJC_EXCEPTIONS;
 }
 
@@ -550,7 +554,7 @@
     context.clip(damageRect);
     context.translate(scrollbar.frameRect().location());
     LocalCurrentGraphicsContext localContext(context);
-    scrollerImpPaint(scrollbarMap()->get(&scrollbar).get(), scrollbar.enabled());
+    scrollerImpPaint(scrollbarMap().get(&scrollbar).get(), scrollbar.enabled());
 
     return true;
 }

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to