Fix infinite Text redraw for SWT.SEARCH with custom background on macOS

bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Text.java

@@ -70,6 +70,18 @@ public class Text extends Scrollable {
 	long actionSearch, actionCancel;
 	APPEARANCE lastAppAppearance;
 
+	/*
+	 * Cached emptiness of the text for the SWT.SEARCH | SWT.ICON_CANCEL paint
+	 * path only. AppKit's NSSearchFieldCell stringValue selector posts
+	 * setNeedsDisplay: synchronously when invoked during a draw pass, turning
+	 * drawInteriorWithFrame_inView_searchfield's cancel-icon visibility check
+	 * into an infinite redraw loop. This flag is updated at the text-mutation
+	 * entry points (outside any paint), so the paint method never has to query
+	 * native state.
+	 * See https://github.com/eclipse-platform/eclipse.platform.ui/issues/3920
+	 */
+	boolean searchFieldHasText;
+
 	/**
 	* The maximum number of characters that can be entered
 	* into a text widget.
@@ -331,6 +343,7 @@ public void append (String string) {
 		widget.scrollRangeToVisible (range);
 		widget.setSelectedRange(range);
 	}
+	updateSearchFieldHasText();
 	if (string.length () != 0) sendEvent (SWT.Modify);
 }
 
@@ -639,6 +652,7 @@ public void cut () {
 		}
 	}
 	Point newSelection = getSelection ();
+	updateSearchFieldHasText();
 	if (!cut || !oldSelection.equals (newSelection)) sendEvent (SWT.Modify);
 }
 
@@ -683,8 +697,21 @@ void drawBackground (long id, NSGraphicsContext context, NSRect rect) {
 	fillBackground (view, context, rect, -1);
 }
 
+// DEBUG instrumentation for https://github.com/eclipse-platform/eclipse.platform.ui/issues/3920
+// Track current paint step so Widget.setNeedsDisplay can log which native call triggered re-entry.
+static volatile String DEBUG_PAINT_STEP = null;
+static final java.util.concurrent.atomic.AtomicInteger DEBUG_PAINT_COUNTER = new java.util.concurrent.atomic.AtomicInteger();
+
+private void debugStep(int paintId, String step) {
+	DEBUG_PAINT_STEP = step;
+	System.err.println("[SWT-DEBUG-PAINT " + paintId + "] step=" + step);
+}
+
 @Override
 void drawInteriorWithFrame_inView (long id, long sel, NSRect cellFrame, long viewid) {
+	int paintId = ((style & SWT.SEARCH) != 0 && background != null) ? DEBUG_PAINT_COUNTER.incrementAndGet() : 0;
+	if (paintId != 0) debugStep(paintId, "ENTER drawInteriorWithFrame_inView");
+
 	Control control = findBackgroundControl();
 	if (control == null) control = this;
 	Image image = control.backgroundImage;
@@ -697,7 +724,12 @@ void drawInteriorWithFrame_inView (long id, long sel, NSRect cellFrame, long vie
 		drawInteriorWithFrame_inView_searchfield(id, sel, cellFrame, viewid);
 	}
 
+	if (paintId != 0) debugStep(paintId, "before super.drawInteriorWithFrame_inView");
 	super.drawInteriorWithFrame_inView(id, sel, cellFrame, viewid);
+	if (paintId != 0) {
+		debugStep(paintId, "after super.drawInteriorWithFrame_inView");
+		DEBUG_PAINT_STEP = null;
+	}
 }
 
 
@@ -710,6 +742,9 @@ void drawInteriorWithFrame_inView_searchfield (long id, long sel, NSRect cellFra
 		return;
 	}
 
+	int paintId = DEBUG_PAINT_COUNTER.get();
+	debugStep(paintId, "ENTER drawInteriorWithFrame_inView_searchfield");
+
 	double searchFieldHeight = 22.0; // Default height of search field on Cocoa
 	double borderWidth = 1.0;
 
@@ -731,26 +766,51 @@ void drawInteriorWithFrame_inView_searchfield (long id, long sel, NSRect cellFra
 		frameRect.y = cellFrame.y + (cellFrame.height - frameRect.height) / 2.0;
 	}
 
+	debugStep(paintId, "before bezierPathWithRoundedRect");
 	// Create a path of the cellFrame with rounded corners
 	NSBezierPath path = NSBezierPath.bezierPathWithRoundedRect(frameRect, 2.0d, 2.0d);
 
+	debugStep(paintId, "before NSColor.colorWithDeviceRed");
 	// Create the native color and fill the background with it
 	NSColor bgColor = NSColor.colorWithDeviceRed (background [0], background [1], background [2], background [3]);
+	debugStep(paintId, "before bgColor.setFill");
 	bgColor.setFill();
+	debugStep(paintId, "before path.fill");
 	path.fill();
 
+	debugStep(paintId, "before NSSearchField cast + cell()");
 	// Finally, paint the search and cancel icons (if present) on top of the filled background
 	NSSearchField searchField = ((NSSearchField)view);
 	NSCell _cell = (NSCell) searchField.cell();
 	SWTSearchFieldCell cell = new SWTSearchFieldCell(_cell.id);
 
+	debugStep(paintId, "before cell.searchButtonCell()");
 	if (cell.searchButtonCell() != null) {
-		cell.searchButtonCell().drawInteriorWithFrame(cell.searchButtonRectForBounds(cellFrame), view);
+		debugStep(paintId, "before searchButtonRectForBounds");
+		NSRect searchRect = cell.searchButtonRectForBounds(cellFrame);
+		debugStep(paintId, "before searchButtonCell.drawInteriorWithFrame");
+		cell.searchButtonCell().drawInteriorWithFrame(searchRect, view);
+		debugStep(paintId, "after searchButtonCell.drawInteriorWithFrame");
 	}
 
-	if (cell.cancelButtonCell() != null && ((NSSearchField) view).stringValue().length() > 0) {
-		cell.cancelButtonCell().drawInteriorWithFrame(cell.cancelButtonRectForBounds(cellFrame), view);
+	debugStep(paintId, "before cell.cancelButtonCell() (cached hasText=" + searchFieldHasText + ")");
+	NSCell cancelCell = cell.cancelButtonCell();
+	/*
+	 * FIX: Use the Java-side searchFieldHasText cache rather than calling
+	 * stringValue on the search field during paint. Both NSControl.stringValue
+	 * and NSCell.stringValue dispatch to the same AppKit selector, which
+	 * posts setNeedsDisplay: as a side effect and re-arms the paint loop we
+	 * are trying to complete. See eclipse.platform.ui#3920 and the
+	 * updateSearchFieldHasText() call sites for how the cache is kept current.
+	 */
+	if (cancelCell != null && searchFieldHasText) {
+		debugStep(paintId, "before cancelButtonRectForBounds");
+		NSRect cancelRect = cell.cancelButtonRectForBounds(cellFrame);
+		debugStep(paintId, "before cancelButtonCell.drawInteriorWithFrame");
+		cancelCell.drawInteriorWithFrame(cancelRect, view);
+		debugStep(paintId, "after cancelButtonCell.drawInteriorWithFrame");
 	}
+	debugStep(paintId, "EXIT drawInteriorWithFrame_inView_searchfield");
 }
 
 @Override
@@ -1433,6 +1493,7 @@ public void insert (String string) {
 		}
 		widget.textStorage ().replaceCharactersInRange (range, str);
 	}
+	updateSearchFieldHasText();
 	if (string.length () != 0) sendEvent (SWT.Modify);
 }
 
@@ -1571,6 +1632,7 @@ void _paste (boolean enableUndo) {
 			}
 		}
 	}
+	updateSearchFieldHasText();
 	sendEvent (SWT.Modify);
 }
 
@@ -2247,6 +2309,7 @@ public void setText (String string) {
 		widget.setString (str);
 		widget.setSelectedRange(new NSRange());
 	}
+	updateSearchFieldHasText();
 	sendEvent (SWT.Modify);
 }
 
@@ -2300,6 +2363,7 @@ public void setTextChars (char[] text) {
 		widget.setString (str);
 		widget.setSelectedRange(new NSRange());
 	}
+	updateSearchFieldHasText();
 	sendEvent (SWT.Modify);
 }
 
@@ -2397,7 +2461,10 @@ boolean shouldChangeTextInRange_replacementString(long id, long sel, long affect
 			result = false;
 		}
 	}
-	if (!result) sendEvent (SWT.Modify);
+	if (!result) {
+		updateSearchFieldHasText();
+		sendEvent (SWT.Modify);
+	}
 	return result;
 }
 
@@ -2434,9 +2501,23 @@ void textViewDidChangeSelection(long id, long sel, long aNotification) {
 @Override
 void textDidChange (long id, long sel, long aNotification) {
 	if ((style & SWT.SINGLE) != 0) super.textDidChange (id, sel, aNotification);
+	updateSearchFieldHasText();
 	postEvent (SWT.Modify);
 }
 
+/*
+ * Refresh searchFieldHasText from the native control. Only safe to call
+ * outside a paint pass; querying stringValue during paint re-arms the
+ * very loop this cache exists to break (see eclipse.platform.ui#3920).
+ */
+void updateSearchFieldHasText() {
+	if ((style & SWT.SEARCH) == 0) return;
+	if (display == null) return;
+	if (display.isPainting != null && display.isPainting.count() > 0) return;
+	NSString value = ((NSControl) view).stringValue();
+	searchFieldHasText = value != null && (int) value.length() > 0;
+}
+
 @Override
 NSRange textView_willChangeSelectionFromCharacterRange_toCharacterRange (long id, long sel, long aTextView, long oldSelectedCharRange, long newSelectedCharRange) {
 	/*

bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Widget.java

@@ -2142,6 +2142,16 @@ void setNeedsDisplay (long id, long sel, boolean flag) {
 	*/
 	OS.objc_msgSend(id, OS.sel_setClipsToBounds_, true);
 	if (flag && display.isPainting.containsObject(view)) {
+		// DEBUG instrumentation for https://github.com/eclipse-platform/eclipse.platform.ui/issues/3920
+		String step = Text.DEBUG_PAINT_STEP;
+		if (step != null) {
+			int n = DEBUG_SND_COUNT.incrementAndGet();
+			System.err.println("[SWT-DEBUG-SND " + n + "] re-entrant setNeedsDisplay during step=" + step
+					+ " view=" + id + " widget=" + this.getClass().getSimpleName());
+			if (n <= 3) {
+				new Throwable("[SWT-DEBUG-SND " + n + "] stack").printStackTrace(System.err);
+			}
+		}
 		NSMutableArray needsDisplay = display.needsDisplay;
 		if (needsDisplay == null) {
 			needsDisplay = (NSMutableArray)new NSMutableArray().alloc();
@@ -2156,6 +2166,9 @@ void setNeedsDisplay (long id, long sel, boolean flag) {
 	OS.objc_msgSendSuper(super_struct, sel, flag);
 }
 
+// DEBUG counter for https://github.com/eclipse-platform/eclipse.platform.ui/issues/3920
+static final java.util.concurrent.atomic.AtomicInteger DEBUG_SND_COUNT = new java.util.concurrent.atomic.AtomicInteger();
+
 void setNeedsDisplayInRect (long id, long sel, long arg0) {
 	if (!isDrawing()) return;
 	NSRect rect = new NSRect();
@@ -2167,6 +2180,16 @@ void setNeedsDisplayInRect (long id, long sel, long arg0) {
 	*/
 	OS.objc_msgSend(id, OS.sel_setClipsToBounds_, true);
 	if (display.isPainting.containsObject(view)) {
+		// DEBUG instrumentation for https://github.com/eclipse-platform/eclipse.platform.ui/issues/3920
+		String step = Text.DEBUG_PAINT_STEP;
+		if (step != null) {
+			int n = DEBUG_SND_COUNT.incrementAndGet();
+			System.err.println("[SWT-DEBUG-SNDR " + n + "] re-entrant setNeedsDisplayInRect during step=" + step
+					+ " view=" + id + " widget=" + this.getClass().getSimpleName());
+			if (n <= 3) {
+				new Throwable("[SWT-DEBUG-SNDR " + n + "] stack").printStackTrace(System.err);
+			}
+		}
 		NSMutableArray needsDisplayInRect = display.needsDisplayInRect;
 		if (needsDisplayInRect == null) {
 			needsDisplayInRect = (NSMutableArray)new NSMutableArray().alloc();

tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Text.java

@@ -13,12 +13,14 @@
  *******************************************************************************/
 package org.eclipse.swt.tests.junit;
 
+import static java.lang.System.currentTimeMillis;
 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 org.eclipse.swt.SWT;
 import org.eclipse.swt.events.ModifyListener;
@@ -30,6 +32,7 @@
 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;
@@ -1376,6 +1379,34 @@ public void test_showSelection() {
 	text.showSelection();
 }
 
+// Originally reported as https://github.com/eclipse-platform/eclipse.platform.ui/issues/3920
+@Test
+public void test_finiteRedraw() {
+	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();
+	Display display = shell.getDisplay();
+	text.forceFocus();
+	long stop = currentTimeMillis() + 1000;
+	// If redraws are constantly scheduled, readAndDispatch() will never return false.
+	// Side effects - high CPU usage, asyncExec() stops working in modal contexts
+	while (display.readAndDispatch()) {
+		if (currentTimeMillis() > stop) {
+			fail("UI should eventually stop refreshing");
+		}
+	}
+}
+
 /* custom */
 Text text;
 String delimiterString;