Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions src/extensions/scenegraph/SGRoot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,35 @@ export class SGRoot {
return this._crossThreadNodes.get(address)?.deref();
}

/**
* Resolves an address to the instance of that node this thread should actually use.
*
* Live reachability wins over the cross-thread registry: `toSGNode` copies `_address_` when it
* rebuilds a node, so repeated serializations of one logical node mint instances sharing its
* address and the registry keeps only the latest. Walking the trees first means a node still
* wired into the scene, the global node or a task always resolves to its authoritative copy,
* with the registry left as the fallback for a true orphan held only by the other thread.
* Mirrors the ordering in `Task.resolveNode` — see the regression it documents.
* @param address Node address to resolve.
* @returns The live node, or undefined when this thread has no instance for that address.
*/
resolveLiveNode(address: string): Node | undefined {
if (!address) {
return undefined;
}
const roots: (Node | undefined)[] = [this._scene, this._mGlobal];
for (const thread of this._threads.values()) {
roots.push(thread.task);
}
for (const root of roots) {
const found = root?.findNodeByAddress(root, address, true);
if (found) {
return found;
}
}
return this.getCrossThreadNode(address);
}

/**
* Adds a new task thread to the SGRoot.
* @param task Task instance to add
Expand Down
83 changes: 66 additions & 17 deletions src/extensions/scenegraph/factory/Serializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,47 @@
return newNode;
}

/**
* Serializes a custom component's script-scope `m` for a node that is changing owner.
*
* Node-valued entries are emitted as address references, not copies. Device-confirmed: `m` holds a
* *live* reference — mutating the referenced node on the receiving thread is visible through `m` —
* so a deep copy is both wrong (a detached snapshot that silently stops tracking) and expensive: a
* component caching `m.scene`/`m.parentScreen` dragged the whole scene tree into every payload,
* measured at 44 KB against 0.2 KB for the same node with scalar-only `m`.
* @param node Node whose `m` is being serialized.
* @param host Optional host node for observing context.
* @param visited Visited set of the enclosing pass.
* @returns The serialized `m` entries, excluding `top`/`global`.
*/
function serializeScriptScopeM(node: Node, host: Node | undefined, visited: WeakSet<object>): FlexObject {
const mData: FlexObject = {};
// Scoped so the values serialized here don't stay marked as visited: the enclosing pass may
// reach the same values by another route and must serialize them in full (see ScopedVisited).
const scoped = new ScopedVisited(visited);
const scopedVisited = scoped as unknown as WeakSet<object>;
try {
for (const [key, value] of node.m.elements) {
const lowerKey = key.toLowerCase();
if (lowerKey === "top" || lowerKey === "global") {
continue;
}
if (value instanceof Node) {
sgRoot.registerCrossThreadNode(value);
mData[key] = { _mref_: value.getAddress() };
continue;
}
mData[key] = jsValueOf(value, true, host, scopedVisited);
}
} finally {
scoped.rollback();
}
return mData;
}

/** One-shot warning when a script-scope node reference has no instance on the receiving thread. */
let warnedUnresolvedScriptScopeRef = false;

/**
* Merges a serialized script-scope `m` (see fromSGNode's `_m_` entry) into a node's `m`,
* preserving the locally built `top`/`global` entries. No-op for payloads without `_m_`.
Expand All @@ -541,6 +582,20 @@
return;
}
for (const [key, value] of Object.entries(serializedM)) {
const address: unknown = (value as FlexObject)?.["_mref_"];
if (typeof address === "string") {
// Resolve to this thread's own instance so `m` keeps referencing the live node.
const live = nodeMap?.get(address) ?? sgRoot.resolveLiveNode(address);
if (live) {
node.m.set(new BrsString(key), live, true);
} else if (!warnedUnresolvedScriptScopeRef) {
warnedUnresolvedScriptScopeRef = true;
BrsDevice.stderr.write(
`warning,[sg] Dropped script-scope reference "${key}": the node it points at has not crossed to this thread`
);
}
continue;
}
node.m.set(new BrsString(key), brsValueOf(value, undefined, nodeMap), true);
}
}
Expand Down Expand Up @@ -732,7 +787,13 @@
* @param visited Optional WeakSet to track visited nodes and prevent circular references.
* @returns A JavaScript object with the converted fields.
*/
export function fromSGNode(node: Node, deep: boolean = true, host?: Node, visited?: WeakSet<object>): FlexObject {
export function fromSGNode(

Check failure on line 790 in src/extensions/scenegraph/factory/Serializer.ts

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this function to reduce its Cognitive Complexity from 30 to the 25 allowed.

See more on https://sonarcloud.io/project/issues?id=lvcabral_brs-emu&issues=AZ-q6j574kWnqMUCPX3Z&open=AZ-q6j574kWnqMUCPX3Z&pullRequest=1098
node: Node,
deep: boolean = true,
host?: Node,
visited?: WeakSet<object>,
scriptScope: boolean = false
): FlexObject {
visited ??= new WeakSet<object>();
if (visited.has(node)) {
return {
Expand Down Expand Up @@ -770,7 +831,7 @@
}
}
if (fieldValue instanceof Node) {
result[name] = fromSGNode(fieldValue, deep, host, visited);
result[name] = fromSGNode(fieldValue, deep, host, visited, scriptScope);
continue;
}
const serialized = jsValueOf(fieldValue, deep, host, visited);
Expand All @@ -793,20 +854,8 @@
// callFunc there would otherwise read init()-set variables back as invalid. `top`/`global`
// are excluded — the receiver rebuilds them locally, and serializing them here would re-walk
// the node and ship the entire global tree.
if (deep && sgRoot.nodeDefMap.has(node.nodeSubtype.toLowerCase())) {
const mData: FlexObject = {};
// Scoped so the values serialized here don't stay marked as visited: the enclosing pass may
// reach the same values by another route and must serialize them in full (see ScopedVisited).
const scoped = new ScopedVisited(visited);
const scopedVisited = scoped as unknown as WeakSet<object>;
for (const [key, value] of node.m.elements) {
const lowerKey = key.toLowerCase();
if (lowerKey === "top" || lowerKey === "global") {
continue;
}
mData[key] = jsValueOf(value, deep, host, scopedVisited);
}
scoped.rollback();
if (deep && scriptScope && sgRoot.nodeDefMap.has(node.nodeSubtype.toLowerCase())) {
const mData = serializeScriptScopeM(node, host, visited);
if (Object.keys(mData).length) {
result["_m_"] = mData;
}
Expand All @@ -815,7 +864,7 @@
if (deep && children.length > 0 && node.serializesChildren()) {
result["_children_"] = children.map((child: BrsType) => {
if (child instanceof Node) {
return fromSGNode(child, deep, host, visited);
return fromSGNode(child, deep, host, visited, scriptScope);
}
return { _invalid_: null };
});
Expand Down
9 changes: 8 additions & 1 deletion src/extensions/scenegraph/nodes/Task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,14 @@ export class Task extends Node {
if (this.threadId < 0 || !this.active) {
return;
}
const value = fieldValue instanceof Node ? fromSGNode(fieldValue, true) : jsValueOf(fieldValue);
// A node crossing task → render changes owner (below), so the receiver runs callFunc against
// its own copy and needs the script-scope `m` that init() populated here. Every other path
// leaves the owner unchanged, and a callFunc on a node owned elsewhere rendezvouses to that
// owner (device-confirmed), so shipping `m` there would only bloat the payload.
const value =
fieldValue instanceof Node
? fromSGNode(fieldValue, true, undefined, undefined, this.inThread)
: jsValueOf(fieldValue);
if (fieldValue instanceof Node) {
// Re-own to the render thread only when the node actually crosses task → render (a task
// setting a field). On the render side this is a fan-out (render → task): the node stays
Expand Down
64 changes: 58 additions & 6 deletions test/extensions/scenegraph/NodeSerialization.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const scenegraph = require("../../../packages/scenegraph/lib/brs-sg.node.js");
const core = require("../../../packages/node/bin/brs.node.js");

const { Node, fromSGNode, toSGNode, updateSGNode, jsValueOf, fromAssociativeArray } = scenegraph;
const { ComponentDefinition, sgRoot, createFlatNode } = scenegraph;
const { ComponentDefinition, sgRoot, createFlatNode, SGNodeFactory } = scenegraph;
const { BrsInvalid, isInvalid, BrsString, BrsBoolean, RoAssociativeArray, RoArray } = core;

/** Simulates the structured/JSON round-trip a node undergoes when sent to a Task thread. */
Expand Down Expand Up @@ -116,6 +116,9 @@ describe("SceneGraph node serialization", () => {
// Mirrors the device contract: a node created on a Task thread runs init() there
// (populating its script-scope `m`), and a later callFunc on the receiving thread must
// still see that state — init() is never re-run on the other side.
//
// `m` only travels on the ownership-transfer path, so these serialize with scriptScope on.
const withScope = (node, deep = true) => fromSGNode(node, deep, undefined, undefined, true);
beforeEach(() => {
const def = new ComponentDefinition("pkg:/components/CustomHelper.xml");
def.name = "CustomHelper";
Expand All @@ -142,12 +145,12 @@ describe("SceneGraph node serialization", () => {
}

test("serializes init()-set m entries, excluding top and global", () => {
const serialized = fromSGNode(makeInitializedNode(), true);
const serialized = withScope(makeInitializedNode());
expect(serialized._m_).toEqual({ setupCalled: false, label: "ready" });
});

test("restores m on the receiving thread so callFunc-visible state survives", () => {
const serialized = fromSGNode(makeInitializedNode(), true);
const serialized = withScope(makeInitializedNode());
const target = toSGNode(transfer(serialized), "Node", "CustomHelper");
expect(mValue(target, "setupCalled")).toBe(false);
expect(mValue(target, "label")).toBe("ready");
Expand All @@ -170,20 +173,69 @@ describe("SceneGraph node serialization", () => {
const serialized = fromAssociativeArray(m, true, taskNode);
expect(serialized.mynode._circular_).toBeUndefined();
expect(serialized.mynode.title).toBe("hello");

// Same guarantee on the transfer path, where `_m_` is actually emitted.
const withM = fromSGNode(taskNode, true, taskNode, undefined, true);
expect(withM._m_.mynode._mref_).toBe(content.getAddress());
});

test("a built-in node never emits _m_", () => {
const node = new Node([], "Node");
node.m.set(new BrsString("stuff"), new BrsString("internal"));
expect(fromSGNode(node, true)._m_).toBeUndefined();
expect(withScope(node)._m_).toBeUndefined();
});

test("emits _m_ only for an ownership transfer, not for an ordinary serialization", () => {
// A node keeping its owner is read back through a rendezvous to that owner, so its `m`
// never travels — shipping it would only bloat every field write and callFunc arg.
expect(fromSGNode(makeInitializedNode(), true)._m_).toBeUndefined();
});

test("stores a node in m as a live reference, not a copy of its subtree", () => {
const scene = SGNodeFactory.createNode("Scene");
const shared = new Node([], "ContentNode");
shared.setValue("id", new BrsString("SharedContent"), false);
shared.setValueSilent("title", new BrsString("set-in-task"));
scene.appendChildToParent(shared);
sgRoot.setScene(scene);

const node = makeInitializedNode();
node.m.set(new BrsString("stashed"), shared, true);

// Device-confirmed: `m` holds a live reference. It crosses as an address, not a subtree.
const serialized = withScope(node);
expect(serialized._m_.stashed).toEqual({ _mref_: shared.getAddress() });

// On the receiving side it resolves back to this thread's own instance, so a later
// mutation of that node is visible through `m` — a copy would have frozen the value.
const target = toSGNode(transfer(serialized), "Node", "CustomHelper");
const restored = target.m.get(new BrsString("stashed"));
expect(restored).toBe(shared);
shared.setValueSilent("title", new BrsString("changed-on-render"));
expect(jsValueOf(restored.getValue("title"))).toBe("changed-on-render");
});

test("drops a script-scope reference whose node has not crossed", () => {
sgRoot.setScene(SGNodeFactory.createNode("Scene"));
const node = makeInitializedNode();
const orphan = new Node([], "ContentNode");
node.m.set(new BrsString("stashed"), orphan, true);

const serialized = transfer(withScope(node));
// Force an address this thread cannot resolve to any live instance.
serialized._m_.stashed._mref_ = "DEADBEEF000000";
const target = toSGNode(serialized, "Node", "CustomHelper");
expect(isInvalid(target.m.get(new BrsString("stashed")))).toBe(true);
// The rest of `m` still restores.
expect(mValue(target, "label")).toBe("ready");
});

test("a shallow serialization (deep = false) skips m", () => {
expect(fromSGNode(makeInitializedNode(), false)._m_).toBeUndefined();
expect(withScope(makeInitializedNode(), false)._m_).toBeUndefined();
});

test("updateSGNode populates m on a flat node but never clobbers local state", () => {
const serialized = transfer(fromSGNode(makeInitializedNode(), true));
const serialized = transfer(withScope(makeInitializedNode()));

const flat = createFlatNode("Node", "CustomHelper");
updateSGNode(serialized, flat);
Expand Down
Loading