From 94a6ba4afb346f39f47c5ec496aaf57dd30bb2c6 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 28 Jul 2026 12:09:40 +0000 Subject: [PATCH] :bug: Fix crash when a nil shape id reaches WASM modifiers A modif-tree containing a nil shape id crashed the WASM heap write with "Cannot read properties of null (reading '__u32_buffer')" in propagate-modifiers. Production builds elide the upstream asserts, so a nil id (e.g. update-dimensions called with a missing shape's parent id in the variants flow) reached the uuid heap write. Drop nil-keyed modif-tree entries in apply-wasm-modifiers and set-wasm-modifiers before they reach propagate-modifiers and set-structure-modifiers, filter nil ids at the parse level, and guard the variant reposition/creation events against missing shapes. AI-assisted-by: hy3 --- .../app/main/data/workspace/modifiers.cljs | 266 ++++++++++-------- .../src/app/main/data/workspace/variants.cljs | 74 ++--- .../logic/wasm_modifiers_nil_id_test.cljs | 109 +++++++ frontend/test/frontend_tests/runner.cljs | 2 + 4 files changed, 290 insertions(+), 161 deletions(-) create mode 100644 frontend/test/frontend_tests/logic/wasm_modifiers_nil_id_test.cljs diff --git a/frontend/src/app/main/data/workspace/modifiers.cljs b/frontend/src/app/main/data/workspace/modifiers.cljs index cb8be131cd..fc8ae7630e 100644 --- a/frontend/src/app/main/data/workspace/modifiers.cljs +++ b/frontend/src/app/main/data/workspace/modifiers.cljs @@ -15,6 +15,7 @@ [app.common.geom.point :as gpt] [app.common.geom.rect :as grc] [app.common.geom.shapes :as gsh] + [app.common.logging :as log] [app.common.math :as mth] [app.common.types.component :as ctk] [app.common.types.container :as ctn] @@ -579,44 +580,57 @@ modifiers (calculate-modifiers state ignore-constraints ignore-snap-pixel modif-tree page-id params)] (assoc state :workspace-modifiers modifiers)))))) +(defn- without-nil-ids + "Drop nil-keyed entries from a modif-tree. A nil shape id (possible in + production builds, where the upstream asserts are elided) would crash + the WASM heap write with `uuid/get-u32` being called on nil." + [modif-tree] + (if (contains? modif-tree nil) + (do (log/warn :hint "modif-tree contains a nil shape id; ignoring entry") + (dissoc modif-tree nil)) + modif-tree)) + (defn- parse-structure-modifiers [modif-tree] (into [] - (mapcat - (fn [[parent-id data]] - (when (ctm/has-structure? (:modifiers data)) - (->> (concat - (get-in data [:modifiers :structure-parent]) - (get-in data [:modifiers :structure-child])) - (mapcat - (fn [modifier] - (case (:type modifier) - :remove-children - (->> (:value modifier) - (map (fn [child-id] - {:type :remove-children - :parent parent-id - :id child-id - :index 0 - :value 0}))) + (comp + (mapcat + (fn [[parent-id data]] + (when (ctm/has-structure? (:modifiers data)) + (->> (concat + (get-in data [:modifiers :structure-parent]) + (get-in data [:modifiers :structure-child])) + (mapcat + (fn [modifier] + (case (:type modifier) + :remove-children + (->> (:value modifier) + (map (fn [child-id] + {:type :remove-children + :parent parent-id + :id child-id + :index 0 + :value 0}))) - :add-children - (->> (:value modifier) - (map (fn [child-id] - {:type :add-children - :parent parent-id - :id child-id - :index (:index modifier) - :value 0}))) + :add-children + (->> (:value modifier) + (map (fn [child-id] + {:type :add-children + :parent parent-id + :id child-id + :index (:index modifier) + :value 0}))) - :scale-content - [{:type :scale-content - :parent parent-id - :id parent-id - :index 0 - :value (:value modifier)}] - nil))))))) + :scale-content + [{:type :scale-content + :parent parent-id + :id parent-id + :index 0 + :value (:value modifier)}] + nil))))))) + (filter (fn [{:keys [id parent]}] + (and (some? id) (some? parent))))) modif-tree)) @@ -624,7 +638,7 @@ (let [default-transform (gmt/matrix)] (keep (fn [[id data]] (cond - (= id uuid/zero) + (or (nil? id) (= id uuid/zero)) nil (ctm/has-geometry? (:modifiers data)) @@ -693,26 +707,27 @@ subtree-ids-by-id selection-rect-cache] :or {ignore-constraints false ignore-snap-pixel false} :as params}] - (ptk/reify ::set-wasm-modifiers - ptk/UpdateEvent - (update [_ state] - (let [property-changes (extract-property-changes modif-tree)] - (if (d/not-empty? property-changes) - (-> state - (assoc :prev-wasm-props (:wasm-props state)) - (assoc :wasm-props property-changes)) - state))) + (let [modif-tree (without-nil-ids modif-tree)] + (ptk/reify ::set-wasm-modifiers + ptk/UpdateEvent + (update [_ state] + (let [property-changes (extract-property-changes modif-tree)] + (if (d/not-empty? property-changes) + (-> state + (assoc :prev-wasm-props (:wasm-props state)) + (assoc :wasm-props property-changes)) + state))) - ptk/WatchEvent - (watch [_ state _] - ;; Entering an interactive transform (drag/resize/rotate). Flip - ;; the renderer into fast + atlas-backdrop mode so the live - ;; preview is cheap, tiles never appear sequentially and the main - ;; thread is not blocked. The pair is closed in - ;; `clear-local-transform`. - (ensure-interactive-transform-start!) - (let [snap-pixel? (and (not ignore-snap-pixel) (contains? (:workspace-layout state) :snap-pixel-grid)) - translation? (every? #(ctm/only-move? (:modifiers %)) (vals modif-tree))] + ptk/WatchEvent + (watch [_ state _] + ;; Entering an interactive transform (drag/resize/rotate). Flip + ;; the renderer into fast + atlas-backdrop mode so the live + ;; preview is cheap, tiles never appear sequentially and the main + ;; thread is not blocked. The pair is closed in + ;; `clear-local-transform`. + (ensure-interactive-transform-start!) + (let [snap-pixel? (and (not ignore-snap-pixel) (contains? (:workspace-layout state) :snap-pixel-grid)) + translation? (every? #(ctm/only-move? (:modifiers %)) (vals modif-tree))]) (if translation? ;; Pure translation: no structure changes needed. If structure @@ -782,43 +797,59 @@ subtree-ids-by-id] :or {ignore-constraints false ignore-snap-pixel false snap-ignore-axis nil undo-transation? true} :as params}] - (ptk/reify ::apply-wasm-modifiers - ptk/WatchEvent - (watch [_ state _] - (let [translation? - (every? #(ctm/only-move? (:modifiers %)) (vals modif-tree))] - (wasm.api/clean-modifiers) - (when-not translation? - (wasm.api/set-structure-modifiers (parse-structure-modifiers modif-tree))) + (let [modif-tree (without-nil-ids modif-tree)] + (ptk/reify ::apply-wasm-modifiers + ptk/WatchEvent + (watch [_ state _] + (let [translation? + (every? #(ctm/only-move? (:modifiers %)) (vals modif-tree))] + (wasm.api/clean-modifiers) + (when-not translation? + (wasm.api/set-structure-modifiers (parse-structure-modifiers modif-tree))) - ;; Apply property changes (e.g. grow-type) to WASM shapes before - ;; propagating geometry, so propagate_modifiers sees the updated state. - (doseq [[id {:keys [property value]}] (extract-property-changes modif-tree)] - (when (= property :grow-type) - (wasm.api/use-shape id) - (wasm.api/set-shape-grow-type value))) + ;; Apply property changes (e.g. grow-type) to WASM shapes before + ;; propagating geometry, so propagate_modifiers sees the updated state. + (doseq [[id {:keys [property value]}] (extract-property-changes modif-tree)] + (when (= property :grow-type) + (wasm.api/use-shape id) + (wasm.api/set-shape-grow-type value))) - (let [objects (dsh/lookup-page-objects state) + (let [objects (dsh/lookup-page-objects state) - geometry-entries - (parse-geometry-modifiers modif-tree) + geometry-entries + (parse-geometry-modifiers modif-tree) - snap-pixel? - (and (not ignore-snap-pixel) (contains? (:workspace-layout state) :snap-pixel-grid)) + snap-pixel? + (and (not ignore-snap-pixel) (contains? (:workspace-layout state) :snap-pixel-grid))] - transforms - (cond - (and translation? (not snap-pixel?)) - ;; Mirror WASM `propagate_modifiers` in CLJS: splat the - ;; translation matrix onto every descendant. Without - ;; this step the commit would only touch the dragged - ;; primaries and descendants would snap back to their - ;; pre-drag positions on drop. - ;; - ;; Skipped when `snap-pixel?` is on: WASM applies - ;; per-shape pixel correction (different scale/translate - ;; per descendant) which we can't replicate cheaply on - ;; the CLJS side. + transforms + (cond + (and translation? (not snap-pixel?)) + ;; Mirror WASM `propagate_modifiers` in CLJS: splat the + ;; translation matrix onto every descendant. Without + ;; this step the commit would only touch the dragged + ;; primaries and descendants would snap back to their + ;; pre-drag positions on drop. + ;; + ;; Skipped when `snap-pixel?` is on: WASM applies + ;; per-shape pixel correction (different scale/translate + ;; per descendant) which we can't replicate cheaply on + ;; the CLJS side. + (reduce + (fn [acc [id data]] + (let [t (:transform data) + subtree-ids + (or (get subtree-ids-by-id id) + (cfh/get-children-ids-with-self objects id))] + (reduce (fn [a sid] (assoc a sid t)) acc subtree-ids))) + {} + geometry-entries) + + ;; Context lost / mid-reload: do not call into WASM. Use + ;; root transforms (and splat translation onto descendants + ;; when we can) so the commit still lands in file data. + (not (wasm.api/initialized?)) + (if translation? (reduce (fn [acc [id data]] (let [t (:transform data) @@ -828,27 +859,12 @@ (reduce (fn [a sid] (assoc a sid t)) acc subtree-ids))) {} geometry-entries) + (into {} + (map (fn [[id data]] [id (:transform data)])) + geometry-entries)) - ;; Context lost / mid-reload: do not call into WASM. Use - ;; root transforms (and splat translation onto descendants - ;; when we can) so the commit still lands in file data. - (not (wasm.api/initialized?)) - (if translation? - (reduce - (fn [acc [id data]] - (let [t (:transform data) - subtree-ids - (or (get subtree-ids-by-id id) - (cfh/get-children-ids-with-self objects id))] - (reduce (fn [a sid] (assoc a sid t)) acc subtree-ids))) - {} - geometry-entries) - (into {} - (map (fn [[id data]] [id (:transform data)])) - geometry-entries)) - - :else - (into {} (wasm.api/propagate-modifiers geometry-entries snap-pixel?))) + :else + (into {} (wasm.api/propagate-modifiers geometry-entries snap-pixel?)) ignore-tree (calculate-ignore-tree-wasm transforms objects) @@ -885,29 +901,29 @@ (map :id)) ids) - undo-id (js/Symbol)] - (rx/concat - (if undo-transation? - (rx/of (dwu/start-undo-transaction undo-id)) - (rx/empty)) - (rx/of - (clear-local-transform) - (ptk/event ::dwg/move-frame-guides {:ids ids :transforms transforms}) - (ptk/event ::dwcm/move-frame-comment-threads transforms) - (dwsh/update-shapes ids update-shape options) + undo-id (js/Symbol)) + (rx/concat + (if undo-transation? + (rx/of (dwu/start-undo-transaction undo-id)) + (rx/empty)) + (rx/of + (clear-local-transform) + (ptk/event ::dwg/move-frame-guides {:ids ids :transforms transforms}) + (ptk/event ::dwcm/move-frame-comment-threads transforms) + (dwsh/update-shapes ids update-shape options) - ;; The update to the bool path needs to be in a different operation because it - ;; needs to have the updated children info. - ;; `update-layout? false`: recalculating a bool path can never change - ;; `:hidden`, and the layout check would recompute the whole boolean - ;; path in WASM once per bool shape just to find that out. - (dwsh/update-shapes bool-ids path/update-bool-shape (assoc options - :with-objects? true - :update-layout? false))) + ;; The update to the bool path needs to be in a different operation because it + ;; needs to have the updated children info. + ;; `update-layout? false`: recalculating a bool path can never change + ;; `:hidden`, and the layout check would recompute the whole boolean + ;; path in WASM once per bool shape just to find that out. + (dwsh/update-shapes bool-ids path/update-bool-shape (assoc options + :with-objects? true + :update-layout? false))) - (if undo-transation? - (rx/of (dwu/commit-undo-transaction undo-id)) - (rx/empty)))))))) + (if undo-transation? + (rx/of (dwu/commit-undo-transaction undo-id)) + (rx/empty))))))))) (def ^:private xf-rotation-shape diff --git a/frontend/src/app/main/data/workspace/variants.cljs b/frontend/src/app/main/data/workspace/variants.cljs index 2d224facc2..8bf4efb3f4 100644 --- a/frontend/src/app/main/data/workspace/variants.cljs +++ b/frontend/src/app/main/data/workspace/variants.cljs @@ -335,14 +335,15 @@ (let [page-id (:current-page-id state) objects (dsh/lookup-page-objects state page-id) shape (get objects shape-id) - container (get objects (:parent-id shape)) - width (+ (:width container) (:width shape) 20) ;; 20 is the default gap for variants - x (- width (+ (:width shape) 30))] ;; 30 is the default margin for variants - (rx/of - (dwt/update-dimensions [(:parent-id shape)] :width width) - (dwt/update-position shape-id - {:x x} - {:absolute? false})))))) + container (get objects (:parent-id shape))] + (when (and (some? shape) (some? container)) + (let [width (+ (:width container) (:width shape) 20) ;; 20 is the default gap for variants + x (- width (+ (:width shape) 30))] ;; 30 is the default margin for variants + (rx/of + (dwt/update-dimensions [(:parent-id shape)] :width width) + (dwt/update-position shape-id + {:x x} + {:absolute? false})))))))) (defn add-new-variant "Create a new variant and add it to the variant-container" @@ -359,39 +360,40 @@ shape (get objects shape-id) shape (if (ctc/is-variant-container? shape) (get objects (last (:shapes shape))) - shape) - component-id (:component-id shape) - component (ctkl/get-component data component-id) + shape)] + (when (some? shape) + (let [component-id (:component-id shape) + component (ctkl/get-component data component-id) - container-id (:parent-id shape) - variant-container (get objects container-id) - has-layout? (ctsl/any-layout? variant-container) + container-id (:parent-id shape) + variant-container (get objects container-id) + has-layout? (ctsl/any-layout? variant-container) - new-component-id (uuid/next) - new-shape-id (uuid/next) + new-component-id (uuid/next) + new-shape-id (uuid/next) - prop-num (dec (count (:variant-properties component))) + prop-num (dec (count (:variant-properties component))) - changes (-> (pcb/empty-changes it page-id) - (pcb/with-library-data data) - (pcb/with-objects objects) - (pcb/with-page-id page-id) - (clv/generate-add-new-variant shape (:variant-id component) new-component-id new-shape-id prop-num)) + changes (-> (pcb/empty-changes it page-id) + (pcb/with-library-data data) + (pcb/with-objects objects) + (pcb/with-page-id page-id) + (clv/generate-add-new-variant shape (:variant-id component) new-component-id new-shape-id prop-num)) - undo-id (js/Symbol)] - (rx/concat - (rx/of - (dwu/start-undo-transaction undo-id) - (dch/commit-changes changes) - (when-not has-layout? - (resposition-and-resize-variant new-shape-id)) - (dwu/commit-undo-transaction undo-id) - (ptk/data-event :layout/update {:ids [(:parent-id shape)]}) - (if multiselect? - (dws/shift-select-shapes new-shape-id) - (dws/select-shape new-shape-id))) - (->> (rx/of (focus-property (:id variant-container))) - (rx/delay 250)))))))) + undo-id (js/Symbol)] + (rx/concat + (rx/of + (dwu/start-undo-transaction undo-id) + (dch/commit-changes changes) + (when-not has-layout? + (resposition-and-resize-variant new-shape-id)) + (dwu/commit-undo-transaction undo-id) + (ptk/data-event :layout/update {:ids [(:parent-id shape)]}) + (if multiselect? + (dws/shift-select-shapes new-shape-id) + (dws/select-shape new-shape-id))) + (->> (rx/of (focus-property (:id variant-container))) + (rx/delay 250)))))))))) (defn transform-in-variant "Given the id of a main shape of a component, creates a variant structure for diff --git a/frontend/test/frontend_tests/logic/wasm_modifiers_nil_id_test.cljs b/frontend/test/frontend_tests/logic/wasm_modifiers_nil_id_test.cljs new file mode 100644 index 0000000000..7e39bf91ee --- /dev/null +++ b/frontend/test/frontend_tests/logic/wasm_modifiers_nil_id_test.cljs @@ -0,0 +1,109 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC Sucursal en España SL + +(ns frontend-tests.logic.wasm-modifiers-nil-id-test + "Reproduces the production crash \"Cannot read properties of null + (reading '__u32_buffer')\". + + A modif-tree containing a nil shape id (production builds elide the + asserts that catch this upstream, e.g. `update-dimensions` called + with `[(:parent-id shape)]` when `shape` is missing) reached + `wasm.api/propagate-modifiers` / `wasm.api/set-structure-modifiers`, + and `mem.h32/write-uuid` crashed calling `uuid/get-u32` on nil while + writing to the WASM heap. + + These tests assert that no nil id ever crosses the WASM boundary and + that valid shapes in the same modif-tree are still processed." + (:require + [app.common.geom.rect :as grc] + [app.common.math :as mth] + [app.common.test-helpers.compositions :as ctho] + [app.common.test-helpers.files :as cthf] + [app.common.test-helpers.ids-map :as cthi] + [app.common.test-helpers.shapes :as cths] + [app.common.types.modifiers :as ctm] + [app.common.uuid :as uuid] + [app.main.data.workspace.modifiers :as dwm] + [app.render-wasm.api :as wasm.api] + [cljs.test :as t :include-macros true] + [frontend-tests.helpers.state :as ths] + [frontend-tests.helpers.wasm :as thw])) + +(def ^:private captured-geometry-entries + "Entries passed to `wasm.api/propagate-modifiers` during a test." + (atom [])) + +(def ^:private captured-structure-entries + "Entries passed to `wasm.api/set-structure-modifiers` during a test." + (atom [])) + +(defn- install-capturing-spies! + "Replace the plain WASM mocks with variants that record their input. + Must run after `thw/setup-wasm-mocks!` so teardown still restores + the real implementations." + [] + (set! wasm.api/propagate-modifiers + (fn [entries _pixel-precision] + (swap! captured-geometry-entries into entries) + (into [] + (map (fn [[id data]] [id (:transform data)])) + entries))) + (set! wasm.api/set-structure-modifiers + (fn [entries] + (swap! captured-structure-entries into entries) + nil))) + +(t/use-fixtures :each + {:before (fn [] + (cthi/reset-idmap!) + (reset! captured-geometry-entries []) + (reset! captured-structure-entries []) + (thw/setup-wasm-mocks!) + (install-capturing-spies!)) + :after (fn [] + (thw/teardown-wasm-mocks!))}) + +(t/deftest nil-id-does-not-reach-propagate-modifiers + ;; A nil-keyed entry must be dropped before the WASM heap write while + ;; the valid entry is still resized. + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (ctho/add-rect :rect1 :x 10 :y 20 :width 100 :height 50)) + store (ths/setup-store file) + rect (cths/get-shape file :rect1) + resize (ctm/change-dimensions-modifiers rect :width 200) + modif-tree {nil {:modifiers resize} + (:id rect) {:modifiers resize}} + events [(dwm/apply-wasm-modifiers modif-tree {:ignore-snap-pixel true})]] + (ths/run-store + store done events + (fn [new-state] + (let [entry-ids (into #{} (map first) @captured-geometry-entries) + file' (ths/get-file-from-state new-state) + rect' (cths/get-shape file' :rect1) + width (-> rect' :points grc/points->rect :width)] + (t/is (not (contains? entry-ids nil))) + (t/is (contains? entry-ids (:id rect))) + (t/is (mth/close? 200 width)))))))) + +(t/deftest nil-id-does-not-reach-set-structure-modifiers + ;; A nil-keyed entry with structure modifiers must not produce + ;; structure entries with a nil :parent or :id. + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (ctho/add-rect :rect1 :x 10 :y 20 :width 100 :height 50)) + store (ths/setup-store file) + rect (cths/get-shape file :rect1) + modif-tree {nil {:modifiers (ctm/add-children nil [(uuid/next)] 0)} + (:id rect) {:modifiers (ctm/change-dimensions-modifiers rect :width 200)}} + events [(dwm/apply-wasm-modifiers modif-tree {:ignore-snap-pixel true})]] + (ths/run-store + store done events + (fn [_new-state] + (t/is (every? #(some? (:parent %)) @captured-structure-entries)) + (t/is (every? #(some? (:id %)) @captured-structure-entries))))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 21c7d23c06..be9e134939 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -34,6 +34,7 @@ [frontend-tests.logic.groups-test] [frontend-tests.logic.nudge-selected-shapes-test] [frontend-tests.logic.pasting-in-containers-test] + [frontend-tests.logic.wasm-modifiers-nil-id-test] [frontend-tests.main-errors-test] [frontend-tests.plugins.comments-test] [frontend-tests.plugins.context-shapes-test] @@ -126,6 +127,7 @@ 'frontend-tests.logic.nudge-selected-shapes-test 'frontend-tests.logic.pasting-in-containers-test 'frontend-tests.main-errors-test + 'frontend-tests.logic.wasm-modifiers-nil-id-test 'frontend-tests.plugins.comments-test 'frontend-tests.plugins.context-shapes-test 'frontend-tests.plugins.file-test