Improve RPC message encoding (eclipse-theia#11294)

* Improve RPC message encoding

- Refactor `RPCMessageEncoder` to enable detection of circular object structures. Circular structures can not be serialized. The detection enables us to fail early instead of indirectly failing once the maximum stack frame limit is reached.
- Add a custom encoder for functions. This ensures that direct function entries in arrays can be serialized.
- Add custom encoder for common collection types (maps, sets). Enables direct use of collection types as rpc call arguments (e.g. logging statements)
- Restore behavior of the old protocol if a log RPC call can not be sent to the backend due to an encoding error 
   - Add custom `EncodingError` type to explicitly catch these errors
   - Refactor `loggerFrontendModule` to catch encoding errors and continue execution normally. i.e. log call is just logged in frontend console and not sent to backend

Fixes eclipse-theia#11249

Contributed on behalf of STMicroelectronics

* Address review feedback

- Remove default value for `visitedObjects` parameter
- Move circular reference check from the `object` value encoder  into `wirteTypeValue` to ensure that all the ciruclar reference check is applied on all object references

packages/core/src/browser/logger-frontend-module.ts

@@ -20,6 +20,7 @@ import { ILogger, Logger, LoggerFactory, setRootLogger, LoggerName, rootLoggerNa
 import { LoggerWatcher } from '../common/logger-watcher';
 import { WebSocketConnectionProvider } from './messaging';
 import { FrontendApplicationContribution } from './frontend-application';
+import { EncodingError } from '../common/message-rpc/rpc-message-encoder';
 
 export const loggerFrontendModule = new ContainerModule(bind => {
     bind(FrontendApplicationContribution).toDynamicValue(ctx => ({
@@ -39,7 +40,13 @@ export const loggerFrontendModule = new ContainerModule(bind => {
             if (property === 'log') {
                 return (name, logLevel, message, params) => {
                     ConsoleLogger.log(name, logLevel, message, params);
-                    return target.log(name, logLevel, message, params);
+                    return target.log(name, logLevel, message, params).catch(err => {
+                        if (err instanceof EncodingError) {
+                            // In case of an EncodingError no RPC call is sent to the backend `ILoggerServer`. Nevertheless, we want to continue normally.
+                            return;
+                        }
+                        throw err;
+                    });
                 };
             }
             return target[property];

packages/core/src/common/message-rpc/rpc-message-encoder.spec.ts

@@ -16,7 +16,7 @@
 
 import { expect } from 'chai';
 import { Uint8ArrayReadBuffer, Uint8ArrayWriteBuffer } from './uint8-array-message-buffer';
-import { RpcMessageDecoder, RpcMessageEncoder } from './rpc-message-encoder';
+import { EncodingError, RpcMessageDecoder, RpcMessageEncoder } from './rpc-message-encoder';
 
 describe('PPC Message Codex', () => {
     describe('RPC Message Encoder & Decoder', () => {
@@ -26,8 +26,11 @@ describe('PPC Message Codex', () => {
 
             const encoder = new RpcMessageEncoder();
             const jsonMangled = JSON.parse(JSON.stringify(encoder));
+            // The RpcMessageEncoder can decode/encode collections, whereas JSON.parse can't. => We have to manually restore the set
+            // eslint-disable-next-line @typescript-eslint/no-explicit-any
+            jsonMangled.registeredTags = (encoder as any).registeredTags;
 
-            encoder.writeTypedValue(writer, encoder);
+            encoder.writeTypedValue(writer, encoder, new WeakSet());
 
             const written = writer.getCurrentContents();
 
@@ -38,5 +41,15 @@ describe('PPC Message Codex', () => {
 
             expect(decoded).deep.equal(jsonMangled);
         });
+        it('should fail with an EncodingError when trying to encode the object ', () => {
+            const x = new Set();
+            const y = new Set();
+            x.add(y);
+            y.add(x);
+            const encoder = new RpcMessageEncoder();
+
+            const writer = new Uint8ArrayWriteBuffer();
+            expect(() => encoder.writeTypedValue(writer, x, new WeakSet())).to.throw(EncodingError);
+        });
     });
 });

packages/core/src/common/message-rpc/rpc-message-encoder.ts

@@ -99,15 +99,18 @@ export enum ObjectType {
     Number = 7,
     // eslint-disable-next-line @typescript-eslint/no-shadow
     ResponseError = 8,
-    Error = 9
+    Error = 9,
+    Map = 10,
+    Set = 11,
+    Function = 12
 
 }
 
 /**
  * A value encoder writes javascript values to a write buffer. Encoders will be asked
  * in turn (ordered by their tag value, descending) whether they can encode a given value
  * This means encoders with higher tag values have priority. Since the default encoders
- * have tag values from 1-7, they can be easily overridden.
+ * have tag values from 1-12, they can be easily overridden.
  */
 export interface ValueEncoder {
     /**
@@ -119,11 +122,12 @@ export interface ValueEncoder {
      * Write the given value to the buffer. Will only be called if {@link is(value)} returns true.
      * @param buf The buffer to write to
      * @param value The value to be written
+     * @param visitedObjects The collection of already visited (i.e. encoded) objects. Used to detect circular references
      * @param recursiveEncode A function that will use the encoders registered on the {@link MessageEncoder}
      * to write a value to the underlying buffer. This is used mostly to write structures like an array
      * without having to know how to encode the values in the array
      */
-    write(buf: WriteBuffer, value: any, recursiveEncode?: (buf: WriteBuffer, value: any) => void): void;
+    write(buf: WriteBuffer, value: any, visitedObjects: WeakSet<object>, recursiveEncode?: (buf: WriteBuffer, value: any, visitedObjects: WeakSet<object>) => void): void;
 }
 
 /**
@@ -141,6 +145,16 @@ export interface ValueDecoder {
     read(buf: ReadBuffer, recursiveDecode: (buf: ReadBuffer) => unknown): unknown;
 }
 
+/**
+ * Custom error thrown by the {@link RpcMessageEncoder} if an error occurred during the encoding and the
+ * object could not be written to the given {@link WriteBuffer}
+ */
+export class EncodingError extends Error {
+    constructor(msg: string) {
+        super(msg);
+    }
+}
+
 /**
  * A `RpcMessageDecoder` parses a a binary message received via {@link ReadBuffer} into a {@link RpcMessage}
  */
@@ -149,6 +163,10 @@ export class RpcMessageDecoder {
     protected decoders: Map<number, ValueDecoder> = new Map();
 
     constructor() {
+        this.registerDecoders();
+    }
+
+    registerDecoders(): void {
         this.registerDecoder(ObjectType.JSON, {
             read: buf => {
                 const json = buf.readString();
@@ -207,6 +225,18 @@ export class RpcMessageDecoder {
             read: buf => buf.readNumber()
         });
 
+        this.registerDecoder(ObjectType.Map, {
+            read: buf => new Map(this.readArray(buf))
+        });
+
+        this.registerDecoder(ObjectType.Set, {
+            read: buf => new Set(this.readArray(buf))
+        });
+
+        this.registerDecoder(ObjectType.Function, {
+            read: () => ({})
+        });
+
     }
 
     /**
@@ -257,9 +287,7 @@ export class RpcMessageDecoder {
     protected parseRequest(msg: ReadBuffer): RequestMessage {
         const callId = msg.readUint32();
         const method = msg.readString();
-        let args = this.readArray(msg);
-        // convert `null` to `undefined`, since we don't use `null` in internal plugin APIs
-        args = args.map(arg => arg === null ? undefined : arg); // eslint-disable-line no-null/no-null
+        const args = this.readArray(msg);
 
         return {
             type: RpcMessageType.Request,
@@ -272,9 +300,7 @@ export class RpcMessageDecoder {
     protected parseNotification(msg: ReadBuffer): NotificationMessage {
         const callId = msg.readUint32();
         const method = msg.readString();
-        let args = this.readArray(msg);
-        // convert `null` to `undefined`, since we don't use `null` in internal plugin APIs
-        args = args.map(arg => arg === null ? undefined : arg); // eslint-disable-line no-null/no-null
+        const args = this.readArray(msg);
 
         return {
             type: RpcMessageType.Notification,
@@ -348,9 +374,14 @@ export class RpcMessageEncoder {
             }
         });
 
+        this.registerEncoder(ObjectType.Function, {
+            is: value => typeof value === 'function',
+            write: () => { }
+        });
+
         this.registerEncoder(ObjectType.Object, {
             is: value => typeof value === 'object',
-            write: (buf, object, recursiveEncode) => {
+            write: (buf, object, visitedObjects, recursiveEncode) => {
                 const properties = Object.keys(object);
                 const relevant = [];
                 for (const property of properties) {
@@ -363,7 +394,7 @@ export class RpcMessageEncoder {
                 buf.writeLength(relevant.length);
                 for (const [property, value] of relevant) {
                     buf.writeString(property);
-                    recursiveEncode?.(buf, value);
+                    recursiveEncode?.(buf, value, visitedObjects);
                 }
             }
         });
@@ -388,6 +419,16 @@ export class RpcMessageEncoder {
             write: (buf, value) => buf.writeString(JSON.stringify(value))
         });
 
+        this.registerEncoder(ObjectType.Map, {
+            is: value => value instanceof Map,
+            write: (buf, value: Map<any, any>, visitedObjects) => this.writeArray(buf, Array.from(value.entries()), visitedObjects)
+        });
+
+        this.registerEncoder(ObjectType.Set, {
+            is: value => value instanceof Set,
+            write: (buf, value: Set<any>, visitedObjects) => this.writeArray(buf, [...value], visitedObjects)
+        });
+
         this.registerEncoder(ObjectType.Undefined, {
             // eslint-disable-next-line no-null/no-null
             is: value => value == null,
@@ -396,8 +437,8 @@ export class RpcMessageEncoder {
 
         this.registerEncoder(ObjectType.ObjectArray, {
             is: value => Array.isArray(value),
-            write: (buf, value) => {
-                this.writeArray(buf, value);
+            write: (buf, value, visitedObjects) => {
+                this.writeArray(buf, value, visitedObjects);
             }
         });
 
@@ -454,44 +495,58 @@ export class RpcMessageEncoder {
         buf.writeUint8(RpcMessageType.Notification);
         buf.writeUint32(requestId);
         buf.writeString(method);
-        this.writeArray(buf, args);
+        this.writeArray(buf, args, new WeakSet());
     }
 
     request(buf: WriteBuffer, requestId: number, method: string, args: any[]): void {
         buf.writeUint8(RpcMessageType.Request);
         buf.writeUint32(requestId);
         buf.writeString(method);
-        this.writeArray(buf, args);
+        this.writeArray(buf, args, new WeakSet());
     }
 
     replyOK(buf: WriteBuffer, requestId: number, res: any): void {
         buf.writeUint8(RpcMessageType.Reply);
         buf.writeUint32(requestId);
-        this.writeTypedValue(buf, res);
+        this.writeTypedValue(buf, res, new WeakSet());
     }
 
     replyErr(buf: WriteBuffer, requestId: number, err: any): void {
         buf.writeUint8(RpcMessageType.ReplyErr);
         buf.writeUint32(requestId);
-        this.writeTypedValue(buf, err);
+        this.writeTypedValue(buf, err, new WeakSet());
     }
 
-    writeTypedValue(buf: WriteBuffer, value: any): void {
-        for (let i: number = this.encoders.length - 1; i >= 0; i--) {
-            if (this.encoders[i][1].is(value)) {
-                buf.writeUint8(this.encoders[i][0]);
-                this.encoders[i][1].write(buf, value, (innerBuffer, innerValue) => {
-                    this.writeTypedValue(innerBuffer, innerValue);
-                });
-                return;
+    writeTypedValue(buf: WriteBuffer, value: any, visitedObjects: WeakSet<object>): void {
+        if (value && typeof value === 'object') {
+            if (visitedObjects.has(value)) {
+                throw new EncodingError('Object to encode contains circular references!');
+            }
+            visitedObjects.add(value);
+        }
+
+        try {
+            for (let i: number = this.encoders.length - 1; i >= 0; i--) {
+                if (this.encoders[i][1].is(value)) {
+                    buf.writeUint8(this.encoders[i][0]);
+                    this.encoders[i][1].write(buf, value, visitedObjects, (innerBuffer, innerValue, _visitedObjects) => {
+                        this.writeTypedValue(innerBuffer, innerValue, _visitedObjects);
+                    });
+                    return;
+                }
+            }
+            throw new EncodingError(`No suitable value encoder found for ${value}`);
+        } finally {
+            if (value && typeof value === 'object') {
+                visitedObjects.delete(value);
             }
         }
     }
 
-    writeArray(buf: WriteBuffer, value: any[]): void {
+    writeArray(buf: WriteBuffer, value: any[], visitedObjects: WeakSet<object>): void {
         buf.writeLength(value.length);
         for (let i = 0; i < value.length; i++) {
-            this.writeTypedValue(buf, value[i]);
+            this.writeTypedValue(buf, value[i], visitedObjects);
         }
     }
 }