Keep what a call answered, so a path through it leads somewhere

A reference which came out of the result of a method call named a place inside
that result, and there was nowhere to resolve it: the path was walked again from
the event, landed on the method, and was told the property did not exist. The
server was left holding a reference to something the page could never reach.

An object now keeps what each path called on it answered, for as long as the
event itself is kept and up to a small number of the most recent calls. Walking
a path which leads through a method steps through that answer rather than the
function, and the method is not run again to get it - it may have side effects,
and by then it may not even be there.

Author: Archmonger

CHANGELOG.md

@@ -45,6 +45,7 @@ Don't forget to remove deprecated code on each major release!
     - Properties which are in flight at the same moment go out as a single message, and `await event.read("target.value", "key")` reads several at once.
     - A read belongs to the moment it is awaited: it reports the current state of the browser object, not the state it held when the event was dispatched.
     - Whatever a read found is remembered against the event, so reading one property twice costs one round trip - and a handler which wants what the browser holds by now asks again with `await event.read("target.value", refresh=True)`.
+    - Whatever comes back out of a method call can be walked too: the page keeps what the call answered for as long as it keeps the event, so a reference into a result resolves against that answer rather than against the method.
     - Unanswered requests are bounded by `reactpy.config.REACTPY_EVENT_PROXY_TIMEOUT`, `reactpy.config.REACTPY_EVENT_PROXY_MAX_TIMEOUTS`, and `reactpy.config.REACTPY_EVENT_PROXY_MAX_DEPTH`.
 
 ### Changed

src/js/packages/@reactpy/client/src/event-proxy.ts

@@ -406,22 +406,94 @@ export function readProperty(node: any, name: string): any {
   }
 }
 
-/** Follow a full property path, refusing anything blocked along the way */
-export function readPath(root: any, path: string[]): any {
+/**
+ * Follow a full property path, refusing anything blocked along the way.
+ *
+ * ``calls`` is what this object has already answered to for a method on it, which is
+ * what lets a path leading through that method be resolved against the answer rather
+ * than against the function itself.
+ */
+export function readPath(
+  root: any,
+  path: string[],
+  calls?: ReadonlyMap<string, unknown>,
+): any {
   let node = root;
-  for (const segment of path) node = readProperty(node, segment);
+  for (let index = 0; index < path.length; index++)
+    node = stepDown(node, path, index, calls);
   return node;
 }
 
-function parentAndKey(root: any, path: string[]): { parent: any; key: string } {
+function parentAndKey(
+  root: any,
+  path: string[],
+  calls?: ReadonlyMap<string, unknown>,
+): { parent: any; key: string } {
   if (path.length === 0)
     throw new ProxyAccessError("an empty path cannot be resolved");
   let node = root;
-  for (const segment of path.slice(0, -1)) node = readProperty(node, segment);
+  for (let index = 0; index < path.length - 1; index++)
+    node = stepDown(node, path, index, calls);
   return { parent: node, key: path[path.length - 1] };
 }
 
-type LiveObject = { value: any; added: number };
+type LiveObject = {
+  value: any;
+  added: number;
+  /**
+   * What each path called on this object answered with, most recent last.
+   *
+   * A reference which came back from inside the result of a call names a place inside
+   * that result, and the result is the only thing it can be resolved against. Keeping
+   * it is what makes such a path walkable; replaying the call is not an option, since a
+   * method may have side effects and may not even be there any more.
+   */
+  calls: Map<string, unknown>;
+};
+
+/** How many call results one object keeps around to walk paths through */
+export const MAX_CALL_RESULTS_PER_OBJECT = 16;
+
+/** What a recorded call is filed under - its own path, and nothing else's */
+function callKey(path: string[]): string {
+  return JSON.stringify(path);
+}
+
+/** Keep what a call answered, so that a path which leads through it still resolves */
+function rememberCall(entry: LiveObject, path: string[], value: unknown): void {
+  if (
+    value === null ||
+    (typeof value !== "object" && typeof value !== "function")
+  )
+    return; // a primitive has nothing inside it to walk into
+  entry.calls.set(callKey(path), value);
+  while (entry.calls.size > MAX_CALL_RESULTS_PER_OBJECT) {
+    const oldest = entry.calls.keys().next();
+    if (oldest.done === true) break;
+    entry.calls.delete(oldest.value);
+  }
+}
+
+/**
+ * Step one segment deeper into a live value.
+ *
+ * A path which leads through a method is stepped through what that method answered
+ * instead, whenever the page has been asked to call it before: the reference the server
+ * is holding came out of that answer, and asking the method again would be both a second
+ * side effect and a different answer.
+ */
+function stepDown(
+  node: any,
+  path: string[],
+  index: number,
+  calls?: ReadonlyMap<string, unknown>,
+): any {
+  if (typeof node === "function" && calls) {
+    const answer = calls.get(callKey(path.slice(0, index)));
+    if (answer !== undefined) node = answer;
+  }
+  return readProperty(node, path[index]);
+}
 
 /**
  * Keeps the objects which recently triggered an event around so the server can walk
@@ -431,6 +503,9 @@ type LiveObject = { value: any; added: number };
  * about them is sent when the event is dispatched, so forgetting one is not a slowdown
  * but a dead end - every later request for it is answered with "not-found". Entries are
  * still bounded in count and in age, since a page cannot hold on to events forever.
+ * Whatever a method on one of them answered is kept alongside it, for as long as the
+ * entry lasts and up to a small number of calls, because a reference which came out of
+ * such an answer names a place inside it and has nowhere else to be resolved against.
  * Every value which leaves this registry goes through ``serializeValue``, so functions
  * and anything which could be used to escape the page never do.
  */
@@ -451,7 +526,7 @@ export class EventProxyRegistry {
   /** Remember ``value`` and return the id the server will address it by */
   register(value: any): string {
     const eventId = `${this.clock().toString(36)}-${(this.nextId++).toString(36)}`;
-    this.live.set(eventId, { value, added: this.clock() });
+    this.live.set(eventId, { value, added: this.clock(), calls: new Map() });
     this.prune();
     return eventId;
   }
@@ -588,7 +663,11 @@ export class EventProxyRegistry {
           path,
           message.args || [],
           maxDepth,
+          entry.calls,
         );
+        // Whatever the server was handed may have come out of this value, so it has to
+        // stay around or the path to it leads nowhere.
+        rememberCall(entry, path, value);
         return reply(true, {
           value: serializeValue(value, eventId, path, options),
         });
@@ -599,8 +678,7 @@ export class EventProxyRegistry {
           value: serializeValue(entry.value, eventId, path, options),
         });
 
-      const { parent, key } = parentAndKey(entry.value, path);
-      const value = readProperty(parent, key);
+      const value = readPath(entry.value, path, entry.calls);
       if (typeof value === "function")
         return reply(true, { value: reference(eventId, path, true) });
       return reply(true, {
@@ -620,10 +698,11 @@ export class EventProxyRegistry {
     path: string[],
     args: any[],
     maxDepth: number,
+    calls?: ReadonlyMap<string, unknown>,
   ): any {
     if (path.length === 0)
       throw new ProxyAccessError("the event object itself cannot be called");
-    const { parent, key } = parentAndKey(root, path);
+    const { parent, key } = parentAndKey(root, path, calls);
     const method = readProperty(parent, key);
     if (typeof method !== "function")
       throw new ProxyAccessError(`${JSON.stringify(key)} is not a function`);
@@ -657,7 +736,7 @@ export class EventProxyRegistry {
           `nothing is registered for event ${JSON.stringify(ref.eventId)}`,
         );
       entry.added = this.clock();
-      return readPath(entry.value, ref.path);
+      return readPath(entry.value, ref.path, entry.calls);
     }
     if (Array.isArray(argument))
       return argument.map((item) =>

src/js/packages/@reactpy/client/tests/event-proxy.test.ts

@@ -15,6 +15,7 @@ import {
   type EventProxyResponse,
   LIVE_OBJECT_TTL_MS,
   MAX_ALLOWED_DEPTH,
+  MAX_CALL_RESULTS_PER_OBJECT,
   MAX_LIVE_OBJECTS,
   MAX_REQUESTS_PER_MESSAGE,
   ProxyAccessError,
@@ -540,6 +541,104 @@ test("calling refuses the event itself, non-functions, and blocked members", ()
   ).toBe("access");
 });
 
+test("a path which leads through a call result is still walkable", () => {
+  // What comes back out of a call is named by a path leading through it, and the only
+  // thing such a path can be resolved against is the answer the page already gave. It
+  // used to lead to a function with no such property, and answer "not-found" - leaving
+  // the server holding a reference to something it could never reach.
+  const { registry } = make_registry();
+  const note: Record<string, unknown> = { text: "written by the browser" };
+  note["about"] = note; // a cycle, which is handed over as a reference
+  const eventId = registry.register({
+    reveal() {
+      return note;
+    },
+  });
+
+  const called = registry.resolve(
+    request({ requestId: "call", eventId, path: ["reveal"], call: true }),
+  );
+  expect(called.ok).toBe(true);
+  expect(readReference(called.value["about"])).toEqual({
+    eventId,
+    path: ["reveal", "about"],
+    callable: false,
+  });
+
+  const walked = registry.resolve(
+    request({ requestId: "walk", eventId, path: ["reveal", "about", "text"] }),
+  );
+  expect(walked.ok).toBe(true);
+  expect(walked.value).toBe("written by the browser");
+});
+
+test("walking through a call result does not run the method again", () => {
+  // The answer is kept rather than the call being made again: a method may have side
+  // effects, and by now it may not even be there.
+  const { registry } = make_registry();
+  let runs = 0;
+  const eventId = registry.register({
+    make() {
+      runs++;
+      return { deep: { value: 42 } };
+    },
+  });
+
+  const called = registry.resolve(
+    request({
+      requestId: "call",
+      eventId,
+      path: ["make"],
+      call: true,
+      maxDepth: 1,
+    }),
+  );
+  expect(runs).toBe(1);
+  expect(readReference(called.value["deep"])?.path).toEqual(["make", "deep"]);
+
+  const walked = registry.resolve(
+    request({ requestId: "walk", eventId, path: ["make", "deep", "value"] }),
+  );
+  expect(walked.ok).toBe(true);
+  expect(walked.value).toBe(42);
+  expect(runs).toBe(1);
+});
+
+test("only the most recent call results are kept to walk through", () => {
+  // Keeping results is what keeps a page's memory bounded, so the oldest make room for
+  // the newest rather than being held for as long as the event itself is.
+  const { registry } = make_registry();
+  const methods: Record<string, () => unknown> = {};
+  for (let index = 0; index <= MAX_CALL_RESULTS_PER_OBJECT; index++)
+    methods[`item${index}`] = () => ({ at: index });
+  const eventId = registry.register(methods);
+
+  for (let index = 0; index <= MAX_CALL_RESULTS_PER_OBJECT; index++)
+    registry.resolve(
+      request({
+        requestId: `call-${index}`,
+        eventId,
+        path: [`item${index}`],
+        call: true,
+      }),
+    );
+
+  const kept = registry.resolve(
+    request({
+      eventId,
+      path: [`item${MAX_CALL_RESULTS_PER_OBJECT}`, "at"],
+    }),
+  );
+  expect(kept.ok).toBe(true);
+  expect(kept.value).toBe(MAX_CALL_RESULTS_PER_OBJECT);
+
+  const forgotten = registry.resolve(
+    request({ eventId, path: ["item0", "at"] }),
+  );
+  expect(forgotten.ok).toBe(false);
+  expect(forgotten.code).toBe("not-found");
+});
+
 test("arguments which are references are resolved on the page", () => {
   const { registry } = make_registry();
   const received: unknown[] = [];