From ec3af8a7083889bbacafd55787b1d6f4ad042dfa Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Wed, 29 Jul 2026 20:43:54 +0300 Subject: [PATCH 1/2] Fail fast in the simulator on surfaces usage that stalls a device Surfaces publishing is cheap in the simulator and expensive on hardware: the same call that is a Java2D encode plus a local file write here writes into a shared app-group container, hands the payload to WidgetKit or ActivityKit over IPC and, for any image that is not already an EncodedImage, blocks the caller on the platform UI thread while the pixels are read back off the GPU (IOSNative.m createImageFile does a Metal flush and a dispatch_sync onto main). An app that publishes on the EDT therefore looks fine in the simulator and freezes on a phone, which is the worst possible place to find out. Discussion #5490 is exactly this: every call funnelled onto the EDT through callSerially, an icon handed over as a live Image, and a widget kind that was never registered. Add SurfaceDiagnostics, active only when Display.isSimulator() (so a shipped build pays nothing) and overridable with the new Surfaces.setDiagnosticsEnabled(Boolean). Conditions that are certain to misbehave on a device throw IllegalStateException naming the fix; the rest log once: - throws when a non-EncodedImage is rasterized on the EDT, checked in SurfaceSerializer.encode so the stack lands on the app's SurfaceImage - throws when publish() targets a kind that was never registered, and names the kinds that are registered so a typo is obvious - warns once when publish/start/update/end runs on the EDT - warns when one kind or activity is republished past the platform's reload budget, pointing at SurfaceDynamicText and future timeline entries as the way to avoid it - warns when an inert LiveActivity handle is used, since update/end are silent no-ops and that is why a refused start goes unnoticed The image check caught the same bug in our own reference code: SurfacesSample generated a mutable avatar and published it from EDT button handlers, with a comment conceding a real app would ship an EncodedImage. It now caches one, generating the pixels on the caller's thread and encoding inside invokeAndBlock; the developer-guide snippet's courierAvatar field changes to EncodedImage for the same reason. Surfaces.publish claimed "no step blocks on the EDT or the platform UI thread", which was never true of the rasterizing encode; that clause is corrected and LiveActivity.start gains the threading section it lacked. SurfaceTest 28/28, full core-unittests module 4286/4286. Verified in a real simulator run both ways: the fixed sample publishes with only the EDT warning, a deliberately-broken copy throws with the stack pointing at its SurfaceImage. Co-Authored-By: Claude Opus 5 (1M context) --- .../com/codename1/surfaces/LiveActivity.java | 17 ++ .../surfaces/SurfaceDiagnostics.java | 282 ++++++++++++++++++ .../codename1/surfaces/SurfaceSerializer.java | 7 + .../src/com/codename1/surfaces/Surfaces.java | 55 +++- .../SurfacesSample/SurfacesSample.java | 40 ++- .../surfaces/SurfacesSnippets.java | 29 +- .../com/codename1/surfaces/SurfaceTest.java | 120 ++++++++ 7 files changed, 533 insertions(+), 17 deletions(-) create mode 100644 CodenameOne/src/com/codename1/surfaces/SurfaceDiagnostics.java diff --git a/CodenameOne/src/com/codename1/surfaces/LiveActivity.java b/CodenameOne/src/com/codename1/surfaces/LiveActivity.java index fab94f095c4..844f57f2138 100644 --- a/CodenameOne/src/com/codename1/surfaces/LiveActivity.java +++ b/CodenameOne/src/com/codename1/surfaces/LiveActivity.java @@ -64,6 +64,17 @@ public static boolean isSupported() { /// Starts a live activity. On unsupported platforms (or when the platform refuses, e.g. the /// user disabled live activities) this returns an inert handle rather than throwing. /// + /// #### Threading + /// + /// Callable from any thread, and a background thread is the right one. Starting an activity + /// serializes the descriptor, writes its PNG blobs where the platform renderer can reach them + /// and makes a synchronous native request (`Activity.request` is an XPC round trip on iOS). + /// The simulator makes all of that free, so an app that starts activities on the EDT looks + /// fine there and stalls on hardware; [Surfaces#setDiagnosticsEnabled(Boolean)] describes the + /// checks that catch it. Note also that the returned handle is the only way to update or end + /// this activity: check [#isActive()] rather than tracking a flag of your own, or a start that + /// the platform refused leaves you starting a second activity on top of a live one. + /// /// #### Parameters /// /// - `descriptor`: the activity layout and regions @@ -74,6 +85,7 @@ public static boolean isSupported() { /// a handle to the running activity; check [#isActive()] to know whether it is live public static LiveActivity start(LiveActivityDescriptor descriptor, Map initialState) { + SurfaceDiagnostics.offEdtPreferred("LiveActivity.start"); SurfaceBridge b = Surfaces.bridgeInternal(); if (b == null || !b.isLiveActivitySupported()) { return new LiveActivity(null); @@ -108,8 +120,11 @@ public static void endRemote(String id, String finalStateJson, boolean dismissIm /// - `state`: the new state map public void update(Map state) { if (!active) { + SurfaceDiagnostics.inertActivity("LiveActivity.update"); return; } + SurfaceDiagnostics.offEdtPreferred("LiveActivity.update"); + SurfaceDiagnostics.noteRepublish("activity:" + id, "live activity \"" + id + "\""); SurfaceBridge b = Surfaces.bridgeInternal(); if (b != null) { b.updateLiveActivity(id, SurfaceSerializer.serializeState(state)); @@ -135,8 +150,10 @@ public void end(Map finalState) { /// platform linger on the final state public void end(Map finalState, boolean dismissImmediately) { if (!active) { + SurfaceDiagnostics.inertActivity("LiveActivity.end"); return; } + SurfaceDiagnostics.offEdtPreferred("LiveActivity.end"); active = false; SurfaceBridge b = Surfaces.bridgeInternal(); if (b != null) { diff --git a/CodenameOne/src/com/codename1/surfaces/SurfaceDiagnostics.java b/CodenameOne/src/com/codename1/surfaces/SurfaceDiagnostics.java new file mode 100644 index 00000000000..1cab4f12131 --- /dev/null +++ b/CodenameOne/src/com/codename1/surfaces/SurfaceDiagnostics.java @@ -0,0 +1,282 @@ +/* + * Copyright (c) 2026, Codename One and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Codename One designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Codename One through http://www.codenameone.com/ if you + * need additional information or have any questions. + */ +package com.codename1.surfaces; + +import com.codename1.io.Log; +import com.codename1.ui.Display; + +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +/// Simulator-only guard rails for the surfaces API. +/// +/// The surfaces wire format is deliberately cheap in the simulator: publishing serializes to JSON, +/// encodes PNG blobs through Java2D and writes them to the local filesystem, all in microseconds. +/// The same code on a device writes into a shared app-group container, hands the payload to +/// WidgetKit or ActivityKit over IPC and, when an image still has to be rasterized, blocks on the +/// platform UI thread to read pixels back off the GPU. Mistakes that are invisible in the +/// simulator therefore surface as an unresponsive app on hardware, which is the worst possible +/// place to discover them. +/// +/// These checks close that gap. They run ONLY in the simulator (or when a test forces them on with +/// [Surfaces#setDiagnosticsEnabled(Boolean)]), so they cost nothing in a shipped build. Conditions +/// that are certain to misbehave on a device throw [IllegalStateException] with a description of +/// the fix; conditions that merely degrade log a one-time warning. +/// +/// @see Surfaces#setDiagnosticsEnabled(Boolean) +final class SurfaceDiagnostics { + /// Rolling window for the republish rate warning. + private static final long RATE_WINDOW_MILLIS = 60000L; + + /// Republishes of one kind (or updates of one activity) per window before warning. WidgetKit + /// gives an app roughly 40-70 reloads per day per widget, so a steady stream this dense is + /// always a bug rather than a tight but legitimate loop. + private static final int RATE_LIMIT = 20; + + /// null = follow the platform (simulator on, everything else off), non-null = forced. + private static Boolean override; + + /// null = ask the real Display, non-null = forced (tests only, see [#setEdtForTests(Boolean)]). + private static Boolean edtForTests; + + private static final Set warnedOnce = new HashSet(); + + /// key -> {window start millis, count in window} + private static final Map rateWindows = new HashMap(); + + private SurfaceDiagnostics() { + } + + static void setEnabled(Boolean value) { + override = value; + clearCaches(); + } + + static boolean enabled() { + if (override != null) { + return override.booleanValue(); + } + // Display may not be initialized at all (unit tests, a static initializer racing init): + // absence of a platform is not a diagnostics failure, it just means there is nothing to + // check against. + try { + return Display.isInitialized() && Display.getInstance().isSimulator(); + } catch (Throwable t) { + return false; + } + } + + static void reset() { + override = null; + edtForTests = null; + clearCaches(); + } + + private static void clearCaches() { + synchronized (warnedOnce) { + warnedOnce.clear(); + } + synchronized (rateWindows) { + rateWindows.clear(); + } + } + + /// Fails when an image that still has to be rasterized is serialized on the EDT. Called only + /// for images that are not already `EncodedImage`s. + /// + /// `SurfaceSerializer` ships an `EncodedImage` by handing over its existing PNG bytes, but any + /// other `Image` has to go through `ImageIO.save`. On iOS that native call drains the render + /// queue and does a `dispatch_sync` onto the main thread to read the pixels back, so calling it + /// from the EDT stalls the UI thread on the platform UI thread for as long as the readback + /// takes. In the simulator the same call is a Java2D encode that returns immediately, which is + /// exactly why this only ever reproduces on hardware. + static void beforeRasterizingImageEncode() { + if (!enabled() || !isEdt()) { + return; + } + throw new IllegalStateException("Surfaces: a surface image is being encoded on the EDT. " + + "SurfaceImage was given a com.codename1.ui.Image that is not an EncodedImage, so " + + "publishing has to rasterize it to PNG. On a device (iOS in particular) that " + + "encode blocks the calling thread on the platform UI thread while the pixels are " + + "read back off the GPU, so doing it on the EDT freezes the app even though the " + + "simulator handles it instantly. Fix it either way: publish off the EDT, or hand " + + "SurfaceImage an EncodedImage - EncodedImage.create(\"/icon.png\") for a bundled " + + "resource, or EncodedImage.createFromImage(img, false) once for a generated one - " + + "which ships the PNG bytes with no native work at all. This check runs only in " + + "the simulator; see Surfaces.setDiagnosticsEnabled(Boolean)."); + } + + /// Fails when a timeline is published for a kind that was never registered. + /// + /// A widget kind has to be declared twice: at build time in `surfaces.json` (the native widget + /// galleries are compiled into the app) and at runtime with + /// [Surfaces#registerWidgetKind(WidgetKind)]. Publishing to an unregistered id silently + /// produces a timeline no renderer will ever pick up, and on a device that looks like "the + /// widget never appears" with nothing in the log. + static void requireRegisteredKind(String kindId) { + if (!enabled() || Surfaces.isKindRegistered(kindId)) { + return; + } + throw new IllegalStateException("Surfaces: publish(\"" + kindId + "\", ...) was called but " + + "no widget kind with that id is registered, so nothing will render it. Call " + + "Surfaces.registerWidgetKind(new WidgetKind(\"" + kindId + "\")...) once, " + + "typically from init(), and declare the same id in the project's surfaces.json " + + "so the native widget gallery is built for it. Registered kinds: " + + describeRegisteredKinds() + ". This check runs only in the simulator; see " + + "Surfaces.setDiagnosticsEnabled(Boolean)."); + } + + /// Warns once per API when a publishing call is made on the EDT. + /// + /// Unlike [#beforeRasterizingImageEncode()] this is not certain to hang: the surfaces API is + /// documented as callable from any thread and a single publish of an already-encoded payload + /// is quick. It is still the wrong thread. On a device the call writes JSON and PNG blobs into + /// the shared container and makes a native round trip (`Activity.request` is a synchronous XPC + /// hop on iOS), none of which the simulator's local filesystem makes you pay for. + static void offEdtPreferred(String api) { + if (!enabled() || !isEdt()) { + return; + } + warnOnce("edt:" + api, api + " was called on the EDT. Surface publishing is data only and " + + "is callable from any thread; on a device it writes the payload into the shared " + + "container and makes a synchronous native call, so running it on the EDT stalls " + + "the UI for as long as that takes. Move it to a background thread - there is no " + + "reason to wrap these calls in callSerially(). This warning appears only in the " + + "simulator."); + } + + /// Warns when one kind or activity is republished far too often. + /// + /// Both WidgetKit and the Android app-widget host throttle reloads against a daily budget, so + /// a republish-per-tick loop does not merely waste work: once the budget is gone the surface + /// stops updating for the rest of the day, on the device only. + /// + /// #### Parameters + /// + /// - `key`: identity of the thing being republished, used to scope the window + /// - `description`: how to name it in the warning + static void noteRepublish(String key, String description) { + if (!enabled()) { + return; + } + long now = System.currentTimeMillis(); + int count; + synchronized (rateWindows) { + long[] window = rateWindows.get(key); + if (window == null || now - window[0] > RATE_WINDOW_MILLIS) { + window = new long[]{now, 0L}; + rateWindows.put(key, window); + } + window[1]++; + count = (int) window[1]; + } + if (count != RATE_LIMIT) { + // Warn on the crossing only, so a genuinely busy app does not drown its own log. + return; + } + warn("Surfaces: " + description + " has been published " + RATE_LIMIT + " times in under " + + (RATE_WINDOW_MILLIS / 1000) + " seconds. WidgetKit and the Android app-widget " + + "host both throttle reloads against a daily budget, so on a device most of these " + + "updates are dropped and the surface then stops refreshing entirely. Publish " + + "only when the underlying data actually changes: SurfaceDynamicText timers and " + + "countdowns tick on the OS clock with no republish at all, and a WidgetTimeline " + + "can carry future entries the renderer applies on its own. This warning appears " + + "only in the simulator."); + } + + /// Warns once when an inert live activity handle is used. + /// + /// `LiveActivity.start` returns an inert handle rather than throwing when the platform refuses + /// (live activities disabled by the user, ActivityKit rejecting the request), and `update` and + /// `end` on that handle are documented no-ops. That is the right production behaviour and a + /// terrible debugging experience, because an app that never checks `isActive()` sees nothing at + /// all and usually goes on to start a second activity. + static void inertActivity(String api) { + if (!enabled()) { + return; + } + warnOnce("inert:" + api, api + " was called on an inert LiveActivity handle and did " + + "nothing. Either LiveActivity.start(...) never created an activity (the platform " + + "does not support live activities, or the user disabled them - check " + + "LiveActivity.isSupported() and the returned handle's isActive()), or end(...) " + + "already ran on this handle. Track the handle's isActive() rather than your own " + + "persisted flag, otherwise the next start(...) leaves an orphan activity the app " + + "can no longer end. This warning appears only in the simulator."); + } + + // --- internals ------------------------------------------------------------ + + /// Test seam: the checks that matter most are the ones that only fire on the EDT, and the + /// portable unit tests run with no platform at all. Null follows the real Display. + static void setEdtForTests(Boolean value) { + edtForTests = value; + } + + private static boolean isEdt() { + if (edtForTests != null) { + return edtForTests.booleanValue(); + } + try { + return Display.isInitialized() && Display.getInstance().isEdt(); + } catch (Throwable t) { + return false; + } + } + + private static void warnOnce(String key, String message) { + synchronized (warnedOnce) { + if (!warnedOnce.add(key)) { + return; + } + } + warn("Surfaces: " + message); + } + + /// A diagnostic must never be the thing that breaks the app, and `Log` needs a platform + /// implementation it does not always have (a test forcing diagnostics on, a warning raised + /// before `Display.init` finished). Falling back to stdout keeps the message rather than + /// trading it for a stack trace. + private static void warn(String message) { + try { + Log.p(message); + } catch (Throwable t) { + System.out.println(message); + } + } + + private static String describeRegisteredKinds() { + StringBuilder b = new StringBuilder(); + for (WidgetKind k : Surfaces.getRegisteredKinds()) { + if (b.length() > 0) { + b.append(", "); + } + b.append('"').append(k.getId()).append('"'); + } + if (b.length() == 0) { + return "none"; + } + return b.toString(); + } +} diff --git a/CodenameOne/src/com/codename1/surfaces/SurfaceSerializer.java b/CodenameOne/src/com/codename1/surfaces/SurfaceSerializer.java index 54b7044c27a..c32cb46a3bb 100644 --- a/CodenameOne/src/com/codename1/surfaces/SurfaceSerializer.java +++ b/CodenameOne/src/com/codename1/surfaces/SurfaceSerializer.java @@ -297,8 +297,15 @@ private static byte[] encode(Image img) { if (img == null) { return null; } + if (!(img instanceof EncodedImage)) { + // Ahead of the try: the diagnostic is a hard stop, and the catch below deliberately + // swallows everything so a bad image degrades to a missing picture rather than a + // failed publish. + SurfaceDiagnostics.beforeRasterizingImageEncode(); + } try { if (img instanceof EncodedImage) { + // The cheap path: the PNG bytes the image already holds, no native work at all. return ((EncodedImage) img).getImageData(); } ImageIO io = ImageIO.getImageIO(); diff --git a/CodenameOne/src/com/codename1/surfaces/Surfaces.java b/CodenameOne/src/com/codename1/surfaces/Surfaces.java index cde71a21127..b9c64cdbd04 100644 --- a/CodenameOne/src/com/codename1/surfaces/Surfaces.java +++ b/CodenameOne/src/com/codename1/surfaces/Surfaces.java @@ -114,6 +114,42 @@ public static List getRegisteredKinds() { return new ArrayList(registeredKinds); } + static boolean isKindRegistered(String kindId) { + for (WidgetKind k : registeredKinds) { + if (k.getId().equals(kindId)) { + return true; + } + } + return false; + } + + /// Overrides whether the simulator-only surface diagnostics run. They are on in the simulator + /// and off everywhere else, which is almost always what you want: they catch usage that works + /// in the simulator but stalls or silently does nothing on a device (rasterizing a surface + /// image on the EDT, publishing to a kind that was never registered, republishing far past the + /// platform's reload budget), and they cost nothing in a shipped build because they never run + /// there. Diagnostics that are certain to misbehave on a device throw `IllegalStateException`; + /// the rest log a one-time warning. + /// + /// Pass null to restore the default behaviour. Turning them off is a last resort for a case a + /// check gets wrong -- please report it if you hit one. + /// + /// #### Parameters + /// + /// - `enabled`: true to force diagnostics on, false to force them off, null for the default + public static void setDiagnosticsEnabled(Boolean enabled) { + SurfaceDiagnostics.setEnabled(enabled); + } + + /// Returns true when the simulator-only surface diagnostics are currently active. + /// + /// #### Returns + /// + /// true when diagnostics run for this process + public static boolean isDiagnosticsEnabled() { + return SurfaceDiagnostics.enabled(); + } + /// Publishes a widget kind's content, atomically replacing any previously published timeline /// and asking the platform to re-render the kind's widget instances. A no-op on platforms /// without widget support. @@ -125,16 +161,26 @@ public static List getRegisteredKinds() { /// callbacks while the app UI is not running (on Android the fetch runs in a background /// service with no Activity at all). Publishing is data-only: the timeline is serialized, /// persisted where the platform renderer can reach it and the renderer is poked - /// asynchronously; no step blocks on the EDT or the platform UI thread. Implementing - /// background fetch and re-publishing there is the intended way to keep widgets fresh; see - /// the `com.codename1.surfaces.spi` package documentation for the per-platform background - /// update story. + /// asynchronously. Implementing background fetch and re-publishing there is the intended way + /// to keep widgets fresh; see the `com.codename1.surfaces.spi` package documentation for the + /// per-platform background update story. + /// + /// A background thread is the RIGHT thread, not merely a permitted one. On a device this + /// writes the payload into the shared container and makes a synchronous native call, and any + /// `SurfaceImage` holding an `Image` that is not an `EncodedImage` is rasterized here -- on + /// iOS that encode blocks the caller on the platform UI thread while the pixels are read back + /// off the GPU. Publishing on the EDT therefore stalls the UI on hardware while looking + /// instantaneous in the simulator. Pass `EncodedImage`s and publish off the EDT; the simulator + /// diagnostics flag both mistakes (see [#setDiagnosticsEnabled(Boolean)]). /// /// #### Parameters /// /// - `kindId`: the widget kind id /// - `timeline`: the content to publish public static void publish(String kindId, WidgetTimeline timeline) { + SurfaceDiagnostics.requireRegisteredKind(kindId); + SurfaceDiagnostics.offEdtPreferred("Surfaces.publish"); + SurfaceDiagnostics.noteRepublish("kind:" + kindId, "widget kind \"" + kindId + "\""); SurfaceBridge b = bridgeInternal(); if (b == null || !b.areWidgetsSupported()) { return; @@ -274,6 +320,7 @@ static void reset() { pendingActions.clear(); } registeredKinds.clear(); + SurfaceDiagnostics.reset(); } private static void deliver(final SurfaceActionHandler h, final SurfaceActionEvent evt) { diff --git a/Samples/samples/SurfacesSample/SurfacesSample.java b/Samples/samples/SurfacesSample/SurfacesSample.java index ec3aeda601c..76b5b7d0202 100644 --- a/Samples/samples/SurfacesSample/SurfacesSample.java +++ b/Samples/samples/SurfacesSample/SurfacesSample.java @@ -44,6 +44,7 @@ import com.codename1.ui.CN; import com.codename1.ui.Dialog; import com.codename1.ui.Display; +import com.codename1.ui.EncodedImage; import com.codename1.ui.Form; import com.codename1.ui.Graphics; import com.codename1.ui.Image; @@ -98,6 +99,7 @@ public class SurfacesSample implements BackgroundFetch { private float activityProgress; private Label installedLabel; private Label activityLabel; + private EncodedImage courierAvatar; public void init(Object context) { theme = UIManager.initFirstTheme("/theme"); @@ -291,7 +293,7 @@ private SurfaceNode buildDeliveryLayout() { params.put("orderId", "CN1-12345"); return new SurfaceColumn().setSpacing(6).setPadding(12) .add(new SurfaceRow().setSpacing(10) - .add(new SurfaceImage(createCourierAvatar()) + .add(new SurfaceImage(courierAvatar()) .setSize(40, 40).setCornerRadius(20)) .add(new SurfaceColumn().setSpacing(2).setWeight(1) .add(new SurfaceText("${status}") @@ -323,7 +325,7 @@ private void startDeliveryActivity() { activityProgress = 0.25f; LiveActivityDescriptor descriptor = new LiveActivityDescriptor("delivery") .setContent(buildDeliveryLayout()) - .setCompactLeading(new SurfaceImage(createCourierAvatar()) + .setCompactLeading(new SurfaceImage(courierAvatar()) .setSize(24, 24).setCornerRadius(12)) .setCompactTrailing(new SurfaceDynamicText( SurfaceDynamicText.STYLE_TIMER_DOWN, "eta") @@ -388,16 +390,32 @@ private void showOrderForm(SurfaceActionEvent evt) { } /** - * A small generated mutable image used as the courier avatar. Generated images exercise the - * serializer's PNG encoding path; a real app would typically ship a bundled EncodedImage. + * The courier avatar, generated once and cached as an EncodedImage. + * + *

Surfaces ship art as PNG bytes. An EncodedImage already holds those bytes, so publishing + * just hands them over; any other Image has to be rasterized at publish time, and on iOS that + * encode blocks the calling thread on the platform UI thread while the pixels come back off + * the GPU. Doing it on the EDT therefore freezes a device while looking instant in the + * simulator, which is why the simulator diagnostics reject it outright. + * + *

Drawing into a mutable image is EDT work and encoding it is not, so this generates on the + * caller's thread and encodes inside invokeAndBlock. A real app usually skips all of this and + * ships a bundled EncodedImage.create("/courier.png"). */ - private Image createCourierAvatar() { - Image avatar = Image.createImage(40, 40, ACCENT_COLOR); - Graphics g = avatar.getGraphics(); - g.setColor(0xffffff); - g.fillArc(10, 6, 20, 20, 0, 360); - g.fillArc(6, 28, 28, 18, 0, 360); - return avatar; + private EncodedImage courierAvatar() { + if (courierAvatar == null) { + final Image avatar = Image.createImage(40, 40, ACCENT_COLOR); + Graphics g = avatar.getGraphics(); + g.setColor(0xffffff); + g.fillArc(10, 6, 20, 20, 0, 360); + g.fillArc(6, 28, 28, 18, 0, 360); + if (CN.isEdt()) { + CN.invokeAndBlock(() -> courierAvatar = EncodedImage.createFromImage(avatar, false)); + } else { + courierAvatar = EncodedImage.createFromImage(avatar, false); + } + } + return courierAvatar; } private Map deliveryState(String status, long eta, float progress) { diff --git a/docs/demos/common/src/main/java/com/codenameone/developerguide/surfaces/SurfacesSnippets.java b/docs/demos/common/src/main/java/com/codenameone/developerguide/surfaces/SurfacesSnippets.java index f5d552a6d23..262ebc88d59 100644 --- a/docs/demos/common/src/main/java/com/codenameone/developerguide/surfaces/SurfacesSnippets.java +++ b/docs/demos/common/src/main/java/com/codenameone/developerguide/surfaces/SurfacesSnippets.java @@ -1,3 +1,25 @@ +/* + * Copyright (c) 2026, Codename One and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Codename One designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Codename One through http://www.codenameone.com/ if you + * need additional information or have any questions. + */ package com.codenameone.developerguide.surfaces; import com.codename1.surfaces.LiveActivity; @@ -17,7 +39,7 @@ import com.codename1.surfaces.WidgetSize; import com.codename1.surfaces.WidgetTimeline; import com.codename1.ui.Dialog; -import com.codename1.ui.Image; +import com.codename1.ui.EncodedImage; import com.codename1.util.Callback; import java.util.Calendar; @@ -34,7 +56,10 @@ public class SurfacesSnippets { private static final int ACCENT_COLOR = 0xff6a1b9a; private LiveActivity activity; - private Image courierAvatar; + // An EncodedImage already holds the PNG bytes a surface ships, so publishing just hands them + // over. Any other Image is rasterized at publish time, and on iOS that encode blocks the + // caller on the platform UI thread -- fine off the EDT, a freeze on it. + private EncodedImage courierAvatar; public void registerKinds() { // tag::registerKind[] diff --git a/maven/core-unittests/src/test/java/com/codename1/surfaces/SurfaceTest.java b/maven/core-unittests/src/test/java/com/codename1/surfaces/SurfaceTest.java index 403b2cef333..fd605dcaaf7 100644 --- a/maven/core-unittests/src/test/java/com/codename1/surfaces/SurfaceTest.java +++ b/maven/core-unittests/src/test/java/com/codename1/surfaces/SurfaceTest.java @@ -677,6 +677,126 @@ public void execute() { }); } + // --- simulator diagnostics ------------------------------------------------ + // + // These guard the mistakes that work in the simulator and stall (or silently do nothing) on a + // device. They are inert without a platform, so every test here turns them on explicitly and + // Surfaces.reset() in tearDown puts them back. + + @Test + void diagnosticsAreInertWithoutAPlatform() { + assertFalse(Surfaces.isDiagnosticsEnabled()); + // no kind registered, and still no complaint: nothing to diagnose off-simulator + Surfaces.setBridge(new FakeBridge()); + Surfaces.publish("never_registered", new WidgetTimeline() + .setContent(new SurfaceText("x"))); + } + + @Test + void publishingAnUnregisteredKindFailsFast() { + Surfaces.setDiagnosticsEnabled(Boolean.TRUE); + Surfaces.setBridge(new FakeBridge()); + Surfaces.registerWidgetKind(new WidgetKind("registered_one")); + IllegalStateException e = assertThrows(IllegalStateException.class, + new org.junit.jupiter.api.function.Executable() { + public void execute() { + Surfaces.publish("typo_in_the_id", new WidgetTimeline() + .setContent(new SurfaceText("x"))); + } + }); + assertTrue(e.getMessage().contains("typo_in_the_id"), e.getMessage()); + assertTrue(e.getMessage().contains("registerWidgetKind"), e.getMessage()); + // the message names what IS registered, so the typo is obvious + assertTrue(e.getMessage().contains("registered_one"), e.getMessage()); + } + + @Test + void publishingARegisteredKindIsUnaffected() { + Surfaces.setDiagnosticsEnabled(Boolean.TRUE); + FakeBridge bridge = new FakeBridge(); + Surfaces.setBridge(bridge); + Surfaces.registerWidgetKind(new WidgetKind("delivery_status")); + Surfaces.publish("delivery_status", new WidgetTimeline() + .setContent(new SurfaceText("${statusLabel}"))); + assertEquals("delivery_status", bridge.publishedKind); + } + + @Test + void rasterizingAnImageOnTheEdtFailsFast() { + // Driven through the diagnostic rather than through SurfaceSerializer because building a + // non-EncodedImage com.codename1.ui.Image needs a platform Display, which this suite has + // no business requiring. SurfaceSerializer.encode() calls exactly this for any image that + // is not already encoded. + Surfaces.setDiagnosticsEnabled(Boolean.TRUE); + SurfaceDiagnostics.setEdtForTests(Boolean.TRUE); + IllegalStateException e = assertThrows(IllegalStateException.class, + new org.junit.jupiter.api.function.Executable() { + public void execute() { + SurfaceDiagnostics.beforeRasterizingImageEncode(); + } + }); + assertTrue(e.getMessage().contains("EncodedImage"), e.getMessage()); + assertTrue(e.getMessage().contains("EDT"), e.getMessage()); + } + + @Test + void rasterizingAnImageOffTheEdtIsAllowed() { + Surfaces.setDiagnosticsEnabled(Boolean.TRUE); + SurfaceDiagnostics.setEdtForTests(Boolean.FALSE); + SurfaceDiagnostics.beforeRasterizingImageEncode(); + } + + @Test + void encodedImagePayloadsSkipTheRasterizingPathEntirely() { + // registerImageBytes is the same entry point EncodedImage takes in encode(): bytes in, + // bytes out, so it stays legal on the EDT where a rasterizing encode does not. + Surfaces.setDiagnosticsEnabled(Boolean.TRUE); + SurfaceDiagnostics.setEdtForTests(Boolean.TRUE); + Map images = new LinkedHashMap(); + String name = SurfaceSerializer.registerImageBytes(pngBytes(7), images); + assertNotNull(name); + assertEquals(1, images.size()); + } + + @Test + void inertLiveActivityCallsStayNoOpsWhileDiagnosed() { + Surfaces.setDiagnosticsEnabled(Boolean.TRUE); + FakeBridge bridge = new FakeBridge(); + bridge.activitiesSupported = false; + Surfaces.setBridge(bridge); + LiveActivity inert = LiveActivity.start( + new LiveActivityDescriptor("delivery").setContent(new SurfaceText("x")), null); + assertFalse(inert.isActive()); + // documented no-ops: the diagnostic explains them, it must not change them + inert.update(new HashMap()); + inert.end(null); + assertTrue(bridge.updates.isEmpty()); + assertNull(bridge.endedId); + } + + @Test + void republishingPastTheRateLimitWarnsWithoutFailing() { + Surfaces.setDiagnosticsEnabled(Boolean.TRUE); + FakeBridge bridge = new FakeBridge(); + Surfaces.setBridge(bridge); + Surfaces.registerWidgetKind(new WidgetKind("chatty")); + for (int i = 0; i < 30; i++) { + Surfaces.publish("chatty", new WidgetTimeline().setContent(new SurfaceText("x"))); + } + assertEquals("chatty", bridge.publishedKind); + } + + @Test + void diagnosticsCanBeForcedOff() { + Surfaces.setDiagnosticsEnabled(Boolean.FALSE); + assertFalse(Surfaces.isDiagnosticsEnabled()); + SurfaceDiagnostics.setEdtForTests(Boolean.TRUE); + Surfaces.setBridge(new FakeBridge()); + SurfaceDiagnostics.beforeRasterizingImageEncode(); + Surfaces.publish("never_registered", new WidgetTimeline() + .setContent(new SurfaceText("x"))); + } + @Test void kindSerializationIncludesSizesAndDefaults() throws Exception { WidgetKind k = new WidgetKind("scores"); From daf7051afadf256dcba002f857f0c88d33d588d8 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Wed, 29 Jul 2026 21:16:34 +0300 Subject: [PATCH 2/2] Address PR review: lock the kind registry, fix the misleading remedy Two findings from the PR review bots, both correct. registeredKinds was a plain ArrayList that registerWidgetKind() mutated while the new isKindRegistered() (and registerWidgetKind itself) walked it, so a registration racing a publish could throw ConcurrentModificationException. The API is explicitly callable from any thread, so guard every touch of the list with its own monitor and let readers copy out rather than iterate live. This also closes the same hazard that already existed in registerWidgetKind before this branch. The EDT image diagnostic suggested EncodedImage.createFromImage(img, false) as the remedy without saying where to run it. Following that advice at the same call site performs the identical ImageIO.save and pays the very stall the check exists to prevent -- and then hides it, because the serializer afterwards sees an EncodedImage. The message now says to convert ONCE, off the EDT, and cache the result, which is what the updated sample does. Adds a regression test that pads the registry and looks the target up last so the lookup genuinely overlaps the writer. Being a race it reproduces the unsynchronized failure about one run in three; it never false-fails with the locking in place (5 consecutive clean runs). SurfaceTest 29/29. Co-Authored-By: Claude Opus 5 (1M context) --- .../surfaces/SurfaceDiagnostics.java | 12 +++-- .../src/com/codename1/surfaces/Surfaces.java | 32 +++++++---- .../com/codename1/surfaces/SurfaceTest.java | 54 +++++++++++++++++++ 3 files changed, 84 insertions(+), 14 deletions(-) diff --git a/CodenameOne/src/com/codename1/surfaces/SurfaceDiagnostics.java b/CodenameOne/src/com/codename1/surfaces/SurfaceDiagnostics.java index 1cab4f12131..6d31bdcd87e 100644 --- a/CodenameOne/src/com/codename1/surfaces/SurfaceDiagnostics.java +++ b/CodenameOne/src/com/codename1/surfaces/SurfaceDiagnostics.java @@ -122,10 +122,14 @@ static void beforeRasterizingImageEncode() { + "encode blocks the calling thread on the platform UI thread while the pixels are " + "read back off the GPU, so doing it on the EDT freezes the app even though the " + "simulator handles it instantly. Fix it either way: publish off the EDT, or hand " - + "SurfaceImage an EncodedImage - EncodedImage.create(\"/icon.png\") for a bundled " - + "resource, or EncodedImage.createFromImage(img, false) once for a generated one - " - + "which ships the PNG bytes with no native work at all. This check runs only in " - + "the simulator; see Surfaces.setDiagnosticsEnabled(Boolean)."); + + "SurfaceImage an EncodedImage, which ships the PNG bytes with no native work at " + + "all. For bundled art that is EncodedImage.create(\"/icon.png\"). For generated " + + "art, convert ONCE with EncodedImage.createFromImage(img, false) and cache the " + + "result - and run that conversion off the EDT (inside invokeAndBlock or on a " + + "background thread), because it performs this very same encode: converting here " + + "on the EDT would pay the stall this check is stopping and hide it from the " + + "check. This check runs only in the simulator; see " + + "Surfaces.setDiagnosticsEnabled(Boolean)."); } /// Fails when a timeline is published for a kind that was never registered. diff --git a/CodenameOne/src/com/codename1/surfaces/Surfaces.java b/CodenameOne/src/com/codename1/surfaces/Surfaces.java index b9c64cdbd04..bc02026284a 100644 --- a/CodenameOne/src/com/codename1/surfaces/Surfaces.java +++ b/CodenameOne/src/com/codename1/surfaces/Surfaces.java @@ -96,13 +96,19 @@ public static void registerWidgetKind(WidgetKind kind) { if (kind == null) { return; } - for (WidgetKind k : registeredKinds) { - if (k.getId().equals(kind.getId())) { - registeredKinds.remove(k); - break; + // The whole API is callable from any thread, so a registration racing a publish (or + // another registration) must not leave a reader walking a list that is being mutated + // underneath it. Every touch of registeredKinds holds this lock, and readers copy out + // rather than iterate the live list. + synchronized (registeredKinds) { + for (WidgetKind k : registeredKinds) { + if (k.getId().equals(kind.getId())) { + registeredKinds.remove(k); + break; + } } + registeredKinds.add(kind); } - registeredKinds.add(kind); SurfaceBridge b = bridgeInternal(); if (b != null) { b.registerWidgetKind(SurfaceSerializer.serializeKind(kind)); @@ -111,13 +117,17 @@ public static void registerWidgetKind(WidgetKind kind) { /// Returns the widget kinds registered so far. public static List getRegisteredKinds() { - return new ArrayList(registeredKinds); + synchronized (registeredKinds) { + return new ArrayList(registeredKinds); + } } static boolean isKindRegistered(String kindId) { - for (WidgetKind k : registeredKinds) { - if (k.getId().equals(kindId)) { - return true; + synchronized (registeredKinds) { + for (WidgetKind k : registeredKinds) { + if (k.getId().equals(kindId)) { + return true; + } } } return false; @@ -319,7 +329,9 @@ static void reset() { actionHandler = null; pendingActions.clear(); } - registeredKinds.clear(); + synchronized (registeredKinds) { + registeredKinds.clear(); + } SurfaceDiagnostics.reset(); } diff --git a/maven/core-unittests/src/test/java/com/codename1/surfaces/SurfaceTest.java b/maven/core-unittests/src/test/java/com/codename1/surfaces/SurfaceTest.java index fd605dcaaf7..bda213545c5 100644 --- a/maven/core-unittests/src/test/java/com/codename1/surfaces/SurfaceTest.java +++ b/maven/core-unittests/src/test/java/com/codename1/surfaces/SurfaceTest.java @@ -786,6 +786,60 @@ void republishingPastTheRateLimitWarnsWithoutFailing() { assertEquals("chatty", bridge.publishedKind); } + @Test + void kindRegistryToleratesConcurrentRegistrationAndPublish() throws Exception { + // The surfaces API is callable from any thread, so registering a kind can overlap a + // publish; the diagnostics kind lookup must not walk the live list while it mutates. + // Being a race, this reproduces the unsynchronized failure roughly one run in three -- + // it can under-report a regression but never false-fails once the locking is correct. + Surfaces.setDiagnosticsEnabled(Boolean.TRUE); + Surfaces.setBridge(new FakeBridge()); + // Pad the registry and look the target up LAST, so the lookup walks the whole list and + // genuinely overlaps the writer instead of hitting on element zero. + for (int i = 0; i < 200; i++) { + Surfaces.registerWidgetKind(new WidgetKind("filler" + i)); + } + Surfaces.registerWidgetKind(new WidgetKind("hot")); + final List failures = + java.util.Collections.synchronizedList(new ArrayList()); + final java.util.concurrent.CountDownLatch go = + new java.util.concurrent.CountDownLatch(1); + Thread writer = new Thread(new Runnable() { + public void run() { + try { + go.await(); + for (int i = 0; i < 2000; i++) { + // re-registering an existing id removes then re-adds: two mutations per + // call, and the removal shifts every element after it + Surfaces.registerWidgetKind(new WidgetKind("filler" + (i % 200))); + } + } catch (Throwable t) { + failures.add(t); + } + } + }); + Thread reader = new Thread(new Runnable() { + public void run() { + try { + go.await(); + for (int i = 0; i < 2000; i++) { + Surfaces.publish("hot", new WidgetTimeline() + .setContent(new SurfaceText("x"))); + Surfaces.getRegisteredKinds(); + } + } catch (Throwable t) { + failures.add(t); + } + } + }); + writer.start(); + reader.start(); + go.countDown(); + writer.join(); + reader.join(); + assertTrue(failures.isEmpty(), String.valueOf(failures)); + } + @Test void diagnosticsCanBeForcedOff() { Surfaces.setDiagnosticsEnabled(Boolean.FALSE);