From 5ce03e5a052b878ec998dbca3bd24aa7135e46bd Mon Sep 17 00:00:00 2001 From: Vasili Gulevich Date: Fri, 1 May 2026 21:46:48 +0400 Subject: [PATCH] Fix infinite redraw loop in controls AppKit uses `setNeedsDisplay()` on This effectively reverts 9f118a85c800f5c94b9b475d802684cb57e8ac81 - a fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=266206 AppKit resets needsDisplay flag when control is drawn. If the flag is set asynchronously and draw methods set it, infinite redraw loop happens. This happens even on uncustomized controls, as AppKit's native draw methods do set it (in complex controls with overlaying cells). --- .../org/eclipse/swt/widgets/Display.java | 29 +-- .../cocoa/org/eclipse/swt/widgets/Widget.java | 21 --- .../Test_org_eclipse_swt_widgets_Text.java | 169 ++++++++++++++++++ 3 files changed, 171 insertions(+), 48 deletions(-) diff --git a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java index ab9d700c3fd..be0a55b7219 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java @@ -155,7 +155,7 @@ enum APPEARANCE { Control currentControl, trackingControl, tooltipControl, ignoreFocusControl; Widget tooltipTarget; - NSMutableArray isPainting, needsDisplay, needsDisplayInRect, runLoopModes; + NSMutableArray isPainting, runLoopModes; NSDictionary markedAttributes; @@ -3994,7 +3994,6 @@ public boolean readAndDispatch () { events = true; application.sendEvent(event); } - events |= runPaint (); events |= runDeferredEvents (); if (!events) { events = isDisposed () || runAsyncMessages (false); @@ -4182,11 +4181,9 @@ void releaseDisplay () { if (screenWindow != null) screenWindow.release(); screenWindow = null; - if (needsDisplay != null) needsDisplay.release(); - if (needsDisplayInRect != null) needsDisplayInRect.release(); if (isPainting != null) isPainting.release(); if (runLoopModes != null) runLoopModes.release(); - needsDisplay = needsDisplayInRect = isPainting = runLoopModes = null; + isPainting = runLoopModes = null; modalShells = null; modalDialog = null; @@ -4464,28 +4461,6 @@ NSArray runLoopModes() { return runLoopModes; } -boolean runPaint () { - if (needsDisplay == null && needsDisplayInRect == null) return false; - if (needsDisplay != null) { - long count = needsDisplay.count(); - for (int i = 0; i < count; i++) { - OS.objc_msgSend(needsDisplay.objectAtIndex(i).id, OS.sel_setNeedsDisplay_, true); - } - needsDisplay.release(); - needsDisplay = null; - } - if (needsDisplayInRect != null) { - long count = needsDisplayInRect.count(); - for (int i = 0; i < count; i+=2) { - NSValue value = new NSValue(needsDisplayInRect.objectAtIndex(i+1)); - OS.objc_msgSend(needsDisplayInRect.objectAtIndex(i).id, OS.sel_setNeedsDisplayInRect_, value.rectValue()); - } - needsDisplayInRect.release(); - needsDisplayInRect = null; - } - return true; -} - boolean runPopups () { if (popups == null) return false; boolean result = false; diff --git a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Widget.java b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Widget.java index 42a5c540b7b..951ec19b0e6 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Widget.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Widget.java @@ -2135,21 +2135,11 @@ boolean setMarkedText_selectedRange (long id, long sel, long string, long range) void setNeedsDisplay (long id, long sel, boolean flag) { if (flag && !isDrawing()) return; - NSView view = new NSView(id); /* * Since macOS 14 the clipsToBounds property of NSView has to be set to true * See https://developer.apple.com/documentation/macos-release-notes/appkit-release-notes-for-macos-14 */ OS.objc_msgSend(id, OS.sel_setClipsToBounds_, true); - if (flag && display.isPainting.containsObject(view)) { - NSMutableArray needsDisplay = display.needsDisplay; - if (needsDisplay == null) { - needsDisplay = (NSMutableArray)new NSMutableArray().alloc(); - display.needsDisplay = needsDisplay = needsDisplay.initWithCapacity(12); - } - needsDisplay.addObject(view); - return; - } objc_super super_struct = new objc_super(); super_struct.receiver = id; super_struct.super_class = OS.objc_msgSend(id, OS.sel_superclass); @@ -2160,22 +2150,11 @@ void setNeedsDisplayInRect (long id, long sel, long arg0) { if (!isDrawing()) return; NSRect rect = new NSRect(); OS.memmove(rect, arg0, NSRect.sizeof); - NSView view = new NSView(id); /* * Since macOS 14 the clipsToBounds property of NSView has to be set to true * See https://developer.apple.com/documentation/macos-release-notes/appkit-release-notes-for-macos-14 */ OS.objc_msgSend(id, OS.sel_setClipsToBounds_, true); - if (display.isPainting.containsObject(view)) { - NSMutableArray needsDisplayInRect = display.needsDisplayInRect; - if (needsDisplayInRect == null) { - needsDisplayInRect = (NSMutableArray)new NSMutableArray().alloc(); - display.needsDisplayInRect = needsDisplayInRect = needsDisplayInRect.initWithCapacity(12); - } - needsDisplayInRect.addObject(view); - needsDisplayInRect.addObject(NSValue.valueWithRect(rect)); - return; - } objc_super super_struct = new objc_super(); super_struct.receiver = id; super_struct.super_class = OS.objc_msgSend(id, OS.sel_superclass); diff --git a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Text.java b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Text.java index eec629f67e4..dab1336f485 100644 --- a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Text.java +++ b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Text.java @@ -13,15 +13,23 @@ *******************************************************************************/ package org.eclipse.swt.tests.junit; +import static java.lang.System.currentTimeMillis; +import static java.lang.System.nanoTime; import static org.eclipse.swt.tests.junit.SwtTestUtil.JENKINS_DETECT_ENV_VAR; import static org.eclipse.swt.tests.junit.SwtTestUtil.JENKINS_DETECT_REGEX; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; +import static org.junit.jupiter.api.Assumptions.assumeFalse; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +import java.lang.management.ManagementFactory; import org.eclipse.swt.SWT; import org.eclipse.swt.events.ModifyListener; +import org.eclipse.swt.events.PaintListener; import org.eclipse.swt.events.SegmentListener; import org.eclipse.swt.events.SelectionEvent; import org.eclipse.swt.events.SelectionListener; @@ -30,12 +38,14 @@ import org.eclipse.swt.graphics.Font; import org.eclipse.swt.graphics.FontData; import org.eclipse.swt.graphics.Point; +import org.eclipse.swt.layout.FillLayout; import org.eclipse.swt.widgets.Display; import org.eclipse.swt.widgets.Event; import org.eclipse.swt.widgets.Group; import org.eclipse.swt.widgets.Text; import org.eclipse.swt.widgets.Widget; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.condition.DisabledIfEnvironmentVariable; @@ -1376,6 +1386,77 @@ public void test_showSelection() { text.showSelection(); } +// Originally reported as https://github.com/eclipse-platform/eclipse.platform.ui/issues/3920 +@Test +public void test_finiteRedrawCancelButtonWithBackground() { + if ( text != null ) text.dispose(); + // Style constants are causing + // org.eclipse.swt.widgets.Text.drawInteriorWithFrame_inView_searchfield(long, long, NSRect, long) + // to call + // org.eclipse.swt.internal.cocoa.NSControl.stringValue() + // which schedules redraw + text = new Text(shell, SWT.SEARCH | SWT.ICON_CANCEL); + // Background prevents early exit from drawInteriorWithFrame_inView_searchfield(long, long, NSRect, long) + text.setBackground(shell.getDisplay().getSystemColor(SWT.COLOR_RED)); + setWidget(text); + shell.setLayout(new FillLayout()); + text.requestLayout(); + shell.open(); + text.forceFocus(); + testIdleAtVariousLength(); +} + +@Test +public void test_finiteRedrawCancelButton() { + if ( text != null ) text.dispose(); + // Style constants are causing + // org.eclipse.swt.widgets.Text.drawInteriorWithFrame_inView_searchfield(long, long, NSRect, long) + // to call + // org.eclipse.swt.internal.cocoa.NSControl.stringValue() + // which schedules redraw + text = new Text(shell, SWT.SEARCH | SWT.ICON_CANCEL); + setWidget(text); + shell.setLayout(new FillLayout()); + text.requestLayout(); + shell.open(); + text.forceFocus(); + testIdleAtVariousLength(); +} + +@Test +public void test_finiteRedrawCancelButtonA() { + if ( text != null ) text.dispose(); + // Style constants are causing + // org.eclipse.swt.widgets.Text.drawInteriorWithFrame_inView_searchfield(long, long, NSRect, long) + // to call + // org.eclipse.swt.internal.cocoa.NSControl.stringValue() + // which schedules redraw + text = new Text(shell, SWT.SEARCH | SWT.ICON_CANCEL); + setWidget(text); + shell.setLayout(new FillLayout()); + text.requestLayout(); + shell.open(); + text.forceFocus(); + text.setText("A"); +// SwtTestUtil.processEvents(100000, () -> false); + waitUntilIdle(); + assertIdle(); + +} + + +@Test +public void test_finiteRedraw() { + if ( text != null ) text.dispose(); + text = new Text(shell, SWT.NONE); + setWidget(text); + shell.setLayout(new FillLayout()); + text.requestLayout(); + shell.open(); + text.forceFocus(); + testIdleAtVariousLength(); +} + /* custom */ Text text; String delimiterString; @@ -1558,6 +1639,7 @@ private void doSegmentsTest (boolean isListening) throws InterruptedException { @Test @Tag("gtk4-todo") @DisabledIfEnvironmentVariable(named = JENKINS_DETECT_ENV_VAR, matches = JENKINS_DETECT_REGEX, disabledReason = "Display.post tests don't run reliably on Jenkins - see https://github.com/eclipse-platform/eclipse.platform.swt/issues/2571") +@Disabled("https://github.com/eclipse-platform/eclipse.platform.swt/issues/2571") // fails on MacOS too public void test_backspaceAndDelete() throws InterruptedException { shell.open(); text.setSize(10, 50); @@ -1639,4 +1721,91 @@ private void pasteFromClipboard(Text text) throws InterruptedException { SwtTestUtil.processEvents(1000, () -> !oldText.equals(text.getText())); } +private void testIdleAtVariousLength() { + waitUntilIdle(); + assertIdle(); + text.setText(""); + waitUntilIdle(); + assertIdle(); + text.setText("a"); + waitUntilIdle(); + assertIdle(); + text.setText("aaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); + waitUntilIdle(); + assertIdle(); +} + +private void waitUntilIdle() { + long hangTimeout = currentTimeMillis() + 1000; + long lastActive = nanoTime(); + while (currentTimeMillis() < hangTimeout) { + if (shell.getDisplay().readAndDispatch()) { + lastActive = nanoTime(); + } else { + // On GTK, blinking caret animation fires with 60 Hz, can't expect long idle times + if (lastActive < nanoTime() - 10_000_000) { + return; + } + Thread.yield(); + } + } + fail("Unexpected system events keep coming"); +} + +private void assertIdle() { + assumeFalse(SwtTestUtil.isGTK4(), "GTK4 bug - 10-40% CPU in idle"); + long[] paintCount = new long[] { 0 }; + long wallNs, cpuNs; + PaintListener paintListener = ignored -> { + paintCount[0]++; + }; + text.addPaintListener(paintListener); + try { + Display display = shell.getDisplay(); + var tmx = ManagementFactory.getThreadMXBean(); + assumeTrue(tmx.isThreadCpuTimeSupported() && tmx.isThreadCpuTimeEnabled(), + "Thread CPU time measurement is not available on this JVM"); + + final int MEASURE_MS = 2000; + + // Schedule a single one-shot timer for the whole measurement window. + // When it fires it (a) sets the guard that terminates the loop, and + // (b) wakes display.sleep() on platforms that would otherwise block + // indefinitely because they generate no background events. + final boolean[] done = {false}; + display.timerExec(MEASURE_MS, () -> done[0] = true); + + long threadId = Thread.currentThread().threadId(); + long cpuBefore = tmx.getThreadCpuTime(threadId); + long wallStart = System.nanoTime(); + // Additional protection against broken event loop, happens on MacOS SDK 24 + long stopGuard = currentTimeMillis() + MEASURE_MS * 2; + while (!done[0]) { + if (stopGuard < currentTimeMillis()) { + fail("Timer should fire"); + } + if (!display.readAndDispatch()) { + // On GTK, blinking caret animation fires with 60 Hz + // We can't just count busy iterations + // Hence the actual CPU load measurement below + display.sleep(); + } + } + wallNs = System.nanoTime() - wallStart; + + cpuNs = tmx.getThreadCpuTime(threadId) - cpuBefore; + } finally { + text.removePaintListener(paintListener); + } + double cpuFraction = (double) cpuNs / wallNs; + int maxPaint =20; + double maxCPU = 0.05; + String message = "CPU usage when idle: %.1f%% < %.1f%%. Paint events: %d < %d" + .formatted(cpuFraction * 100, maxCPU * 100, paintCount[0], maxPaint); + if (SwtTestUtil.verbose) { + System.out.println(message); + } + assertTrue(cpuFraction < maxCPU && paintCount[0] < maxPaint, message); +} + }