Skip to content

Commit cfa4aa1

Browse files
iidmsarniwa
authored andcommitted
Address Safer cpp failures in mac/ScrollerMac
https://bugs.webkit.org/show_bug.cgi?id=291832 rdar://problem/149656606 Reviewed by Ryosuke Niwa. Fix clang static analyzer warnings in mac/ScrollerMac. * Source/WebCore/SaferCPPExpectations/NoUncountedMemberCheckerExpectations: * Source/WebCore/SaferCPPExpectations/UncountedCallArgsCheckerExpectations: * Source/WebCore/SaferCPPExpectations/UnretainedCallArgsCheckerExpectations: * Source/WebCore/SaferCPPExpectations/UnretainedLocalVarsCheckerExpectations: * Source/WebCore/page/scrolling/mac/ScrollerMac.h: (WebCore::ScrollerMac::pair): * Source/WebCore/page/scrolling/mac/ScrollerMac.mm: (-[WebScrollbarPartAnimationMac setCurrentProgress:]): (-[WebScrollerImpDelegateMac effectiveAppearanceForScrollerImp:]): (-[WebScrollerImpDelegateMac setUpAlphaAnimation:featureToAnimate:animateAlphaTo:duration:]): (WebCore::ScrollerMac::attach): (WebCore::ScrollerMac::updateValues): (WebCore::ScrollerMac::updateScrollbarStyle): (WebCore::ScrollerMac::updatePairScrollerImps): (WebCore::ScrollerMac::mouseEnteredScrollbar): (WebCore::ScrollerMac::mouseExitedScrollbar): (WebCore::ScrollerMac::lastKnownMousePositionInScrollbar const): (WebCore::ScrollerMac::visibilityChanged): (WebCore::ScrollerMac::updateMinimumKnobLength): (WebCore::ScrollerMac::scrollbarState const): * Source/WebCore/page/scrolling/mac/ScrollerPairMac.h: Canonical link: https://commits.webkit.org/293949@main
1 parent fc502b0 commit cfa4aa1

File tree

6 files changed

+46
-30
lines changed

6 files changed

+46
-30
lines changed

Source/WebCore/SaferCPPExpectations/NoUncountedMemberCheckerExpectations

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,6 @@ page/WheelEventTestMonitor.h
5959
page/scrolling/ScrollLatchingController.h
6060
page/scrolling/ScrollingCoordinator.h
6161
page/scrolling/ScrollingTreeGestureState.h
62-
page/scrolling/mac/ScrollerMac.h
6362
platform/PODInterval.h
6463
platform/cocoa/WebAVPlayerLayer.h
6564
platform/encryptedmedia/CDMProxy.h

Source/WebCore/SaferCPPExpectations/UncountedCallArgsCheckerExpectations

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -937,7 +937,6 @@ page/scrolling/ScrollingTree.cpp
937937
page/scrolling/ScrollingTree.h
938938
page/scrolling/ScrollingTreeGestureState.cpp
939939
page/scrolling/ThreadedScrollingCoordinator.cpp
940-
page/scrolling/mac/ScrollerMac.mm
941940
page/scrolling/mac/ScrollingCoordinatorMac.mm
942941
page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm
943942
page/text-extraction/TextExtraction.cpp

Source/WebCore/SaferCPPExpectations/UnretainedCallArgsCheckerExpectations

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,6 @@ page/mac/EventHandlerMac.mm
6969
page/mac/ImageOverlayControllerMac.mm
7070
page/mac/ServicesOverlayController.mm
7171
page/mac/TextIndicatorWindow.mm
72-
page/scrolling/mac/ScrollerMac.mm
7372
page/scrolling/mac/ScrollerPairMac.mm
7473
page/scrolling/mac/ScrollingTreeOverflowScrollingNodeMac.mm
7574
page/scrolling/mac/ScrollingTreePluginScrollingNodeMac.mm

Source/WebCore/SaferCPPExpectations/UnretainedLocalVarsCheckerExpectations

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ page/CaptionUserPreferencesMediaAF.cpp
3131
page/cocoa/ResourceUsageThreadCocoa.mm
3232
page/mac/ChromeMac.mm
3333
page/mac/EventHandlerMac.mm
34-
page/scrolling/mac/ScrollerMac.mm
3534
page/scrolling/mac/ScrollingStateScrollingNodeMac.mm
3635
page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm
3736
page/scrolling/mac/ScrollingTreeScrollingNodeDelegateMac.mm

Source/WebCore/page/scrolling/mac/ScrollerMac.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ class ScrollerMac {
4848

4949
void attach();
5050

51-
ScrollerPairMac& pair() { return m_pair; }
51+
RefPtr<ScrollerPairMac> pair() const { return m_pair.get(); }
5252

5353
ScrollbarOrientation orientation() const { return m_orientation; }
5454

@@ -86,7 +86,7 @@ class ScrollerMac {
8686
bool m_isVisible { false };
8787
bool m_isHiddenByStyle { false };
8888

89-
ScrollerPairMac& m_pair;
89+
ThreadSafeWeakPtr<ScrollerPairMac> m_pair;
9090
const ScrollbarOrientation m_orientation;
9191
IntPoint m_lastKnownMousePositionInScrollbar;
9292
UserInterfaceLayoutDirection m_scrollbarLayoutDirection { UserInterfaceLayoutDirection::LTR };

Source/WebCore/page/scrolling/mac/ScrollerMac.mm

Lines changed: 44 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -100,18 +100,20 @@ - (void)setCurrentProgress:(NSAnimationProgress)progress
100100
else
101101
currentValue = progress;
102102

103+
RetainPtr scrollerImp = _scroller->scrollerImp();
104+
103105
switch (_featureToAnimate) {
104106
case FeatureToAnimate::KnobAlpha:
105-
[_scroller->scrollerImp() setKnobAlpha:currentValue];
107+
[scrollerImp.get() setKnobAlpha:currentValue];
106108
break;
107109
case FeatureToAnimate::TrackAlpha:
108-
[_scroller->scrollerImp() setTrackAlpha:currentValue];
110+
[scrollerImp.get() setTrackAlpha:currentValue];
109111
break;
110112
case FeatureToAnimate::UIStateTransition:
111-
[_scroller->scrollerImp() setUiStateTransitionProgress:currentValue];
113+
[scrollerImp.get() setUiStateTransitionProgress:currentValue];
112114
break;
113115
case FeatureToAnimate::ExpansionTransition:
114-
[_scroller->scrollerImp() setExpansionTransitionProgress:currentValue];
116+
[scrollerImp.get() setExpansionTransitionProgress:currentValue];
115117
break;
116118
}
117119
}
@@ -204,7 +206,8 @@ - (NSAppearance *)effectiveAppearanceForScrollerImp:(NSScrollerImp *)scrollerImp
204206
if (!_scroller)
205207
return [NSAppearance currentDrawingAppearance];
206208
// The base system does not support dark Aqua, so we might get a null result.
207-
if (auto *appearance = [NSAppearance appearanceNamed:_scroller->pair().useDarkAppearance() ? NSAppearanceNameDarkAqua : NSAppearanceNameAqua])
209+
// FIXME: This is a static analysis false positive.
210+
SUPPRESS_UNRETAINED_ARG if (auto *appearance = [NSAppearance appearanceNamed:_scroller->pair()->useDarkAppearance() ? NSAppearanceNameDarkAqua : NSAppearanceNameAqua])
208211
return appearance;
209212
return [NSAppearance currentDrawingAppearance];
210213
}
@@ -217,9 +220,12 @@ - (void)setUpAlphaAnimation:(RetainPtr<WebScrollbarPartAnimationMac>&)scrollbarP
217220
scrollbarPartAnimation = nil;
218221
}
219222

223+
RetainPtr scrollerImp = _scroller->scrollerImp();
224+
CGFloat currentAlpha = featureToAnimate == FeatureToAnimate::KnobAlpha ? [scrollerImp.get() knobAlpha] : [scrollerImp.get() trackAlpha];
225+
220226
scrollbarPartAnimation = adoptNS([[WebScrollbarPartAnimationMac alloc] initWithScroller:_scroller
221227
featureToAnimate:featureToAnimate
222-
animateFrom:featureToAnimate == FeatureToAnimate::KnobAlpha ? [_scroller->scrollerImp() knobAlpha] : [_scroller->scrollerImp() trackAlpha]
228+
animateFrom:currentAlpha
223229
animateTo:newAlpha
224230
duration:duration]);
225231
[scrollbarPartAnimation startAnimation];
@@ -327,8 +333,9 @@ - (void)invalidate
327333

328334
void ScrollerMac::attach()
329335
{
336+
RefPtr pair = m_pair.get();
330337
m_scrollerImpDelegate = adoptNS([[WebScrollerImpDelegateMac alloc] initWithScroller:this]);
331-
setScrollerImp([NSScrollerImp scrollerImpWithStyle:nsScrollerStyle(m_pair.scrollbarStyle()) controlSize:nsControlSizeFromScrollbarWidth(m_pair.scrollbarWidthStyle()) horizontal:m_orientation == ScrollbarOrientation::Horizontal replacingScrollerImp:nil]);
338+
setScrollerImp([NSScrollerImp scrollerImpWithStyle:nsScrollerStyle(pair->scrollbarStyle()) controlSize:nsControlSizeFromScrollbarWidth(pair->scrollbarWidthStyle()) horizontal:m_orientation == ScrollbarOrientation::Horizontal replacingScrollerImp:nil]);
332339
[m_scrollerImp setDelegate:m_scrollerImpDelegate.get()];
333340
}
334341

@@ -366,7 +373,8 @@ - (void)invalidate
366373

367374
void ScrollerMac::updateValues()
368375
{
369-
auto values = m_pair.valuesForOrientation(m_orientation);
376+
RefPtr pair = m_pair.get();
377+
auto values = pair->valuesForOrientation(m_orientation);
370378

371379
BEGIN_BLOCK_OBJC_EXCEPTIONS
372380
[m_scrollerImp setEnabled:m_isEnabled];
@@ -381,7 +389,8 @@ - (void)invalidate
381389

382390
void ScrollerMac::updateScrollbarStyle()
383391
{
384-
setScrollerImp([NSScrollerImp scrollerImpWithStyle:nsScrollerStyle(m_pair.scrollbarStyle()) controlSize:nsControlSizeFromScrollbarWidth(m_pair.scrollbarWidthStyle()) horizontal:m_orientation == ScrollbarOrientation::Horizontal replacingScrollerImp:nil]);
392+
RefPtr pair = m_pair.get();
393+
setScrollerImp([NSScrollerImp scrollerImpWithStyle:nsScrollerStyle(pair->scrollbarStyle()) controlSize:nsControlSizeFromScrollbarWidth(pair->scrollbarWidthStyle()) horizontal:m_orientation == ScrollbarOrientation::Horizontal replacingScrollerImp:nil]);
385394
[m_scrollerImp setDelegate:m_scrollerImpDelegate.get()];
386395

387396
[m_scrollerImp setLayer:m_hostLayer.get()];
@@ -392,46 +401,54 @@ - (void)invalidate
392401

393402
void ScrollerMac::updatePairScrollerImps()
394403
{
395-
NSScrollerImp *scrollerImp = m_scrollerImp.get();
404+
RetainPtr scrollerImp = m_scrollerImp.get();
405+
RefPtr pair = m_pair.get();
396406
if (m_orientation == ScrollbarOrientation::Vertical)
397-
m_pair.setVerticalScrollerImp(scrollerImp);
407+
pair->setVerticalScrollerImp(scrollerImp.get());
398408
else
399-
m_pair.setHorizontalScrollerImp(scrollerImp);
409+
pair->setHorizontalScrollerImp(scrollerImp.get());
400410
}
401411

402412
void ScrollerMac::mouseEnteredScrollbar()
403413
{
404-
m_pair.ensureOnMainThreadWithProtectedThis([this](auto&) {
414+
RefPtr pair = m_pair.get();
415+
RetainPtr protectedScrollerImp = m_scrollerImp;
416+
RetainPtr protectedPair = pair->scrollerImpPair();
417+
pair->ensureOnMainThreadWithProtectedThis([pair, protectedScrollerImp = WTFMove(protectedScrollerImp), protectedPair = WTFMove(protectedPair)](auto&) {
405418
// At this time, only legacy scrollbars needs to send notifications here.
406-
if (m_pair.scrollbarStyle() != WebCore::ScrollbarStyle::AlwaysVisible)
419+
if (pair->scrollbarStyle() != WebCore::ScrollbarStyle::AlwaysVisible)
407420
return;
408421

409-
if ([m_pair.scrollerImpPair() overlayScrollerStateIsLocked])
422+
if ([protectedPair overlayScrollerStateIsLocked])
410423
return;
411424

412-
[m_scrollerImp mouseEnteredScroller];
425+
[protectedScrollerImp mouseEnteredScroller];
413426
});
414427
}
415428

416429
void ScrollerMac::mouseExitedScrollbar()
417430
{
418-
m_pair.ensureOnMainThreadWithProtectedThis([this](auto&) {
431+
RefPtr pair = m_pair.get();
432+
RetainPtr protectedScrollerImp = m_scrollerImp;
433+
RetainPtr protectedPair = pair->scrollerImpPair();
434+
pair->ensureOnMainThreadWithProtectedThis([pair, protectedScrollerImp = WTFMove(protectedScrollerImp), protectedPair = WTFMove(protectedPair)](auto&) {
419435
// At this time, only legacy scrollbars needs to send notifications here.
420-
if (m_pair.scrollbarStyle() != WebCore::ScrollbarStyle::AlwaysVisible)
436+
if (pair->scrollbarStyle() != WebCore::ScrollbarStyle::AlwaysVisible)
421437
return;
422438

423-
if ([m_pair.scrollerImpPair() overlayScrollerStateIsLocked])
439+
if ([protectedPair overlayScrollerStateIsLocked])
424440
return;
425441

426-
[m_scrollerImp mouseExitedScroller];
442+
[protectedScrollerImp mouseExitedScroller];
427443
});
428444
}
429445

430446
IntPoint ScrollerMac::lastKnownMousePositionInScrollbar() const
431447
{
432448
// When we dont have an update from the Web Process, return
433449
// a point outside of the scrollbars
434-
if (!m_pair.mouseInContentArea())
450+
RefPtr pair = m_pair.get();
451+
if (!pair->mouseInContentArea())
435452
return { -1, -1 };
436453
return m_lastKnownMousePositionInScrollbar;
437454
}
@@ -442,7 +459,8 @@ - (void)invalidate
442459
return;
443460
m_isVisible = isVisible;
444461

445-
if (RefPtr node = m_pair.protectedNode())
462+
RefPtr pair = m_pair.get();
463+
if (RefPtr node = pair->protectedNode())
446464
node->scrollbarVisibilityDidChange(m_orientation, isVisible);
447465
}
448466

@@ -452,7 +470,8 @@ - (void)invalidate
452470
return;
453471
m_minimumKnobLength = minimumKnobLength;
454472

455-
if (RefPtr node = m_pair.protectedNode())
473+
RefPtr pair = m_pair.get();
474+
if (RefPtr node = pair->protectedNode())
456475
node->scrollbarMinimumThumbLengthDidChange(m_orientation, m_minimumKnobLength);
457476
}
458477

@@ -487,7 +506,8 @@ - (void)invalidate
487506
StringBuilder result;
488507
result.append([m_scrollerImp isEnabled] ? "enabled"_s: "disabled"_s);
489508

490-
if (m_pair.scrollbarStyle() != WebCore::ScrollbarStyle::Overlay)
509+
RefPtr pair = m_pair.get();
510+
if (pair->scrollbarStyle() != WebCore::ScrollbarStyle::Overlay)
491511
return result.toString();
492512

493513
if ([m_scrollerImp isExpanded])

0 commit comments

Comments
 (0)