fix(zone.js): read Symbol.species safely (#45369)

arturovt · dylhunn · commit e2eaac34b06a · 2022-03-24T18:56:36.000-07:00
We must read `Symbol.species` safely because `this` may be anything. For instance, `this` may be an object without a prototype (created through `Object.create(null)`); thus `this.constructor` will be undefined. One of the use cases is SystemJS creating prototype-less objects (modules) via `Object.create(null)`. The SystemJS creates an empty object and copies promise properties into that object (within the `getOrCreateLoad` function). The zone.js then checks if the resolved value has the `then` method and invokes it with the `value` context. Otherwise, this will throw an error: `TypeError: Cannot read properties of undefined (reading 'Symbol(Symbol.species)')`. PR Close #45369
diff --git a/packages/zone.js/lib/common/promise.ts b/packages/zone.js/lib/common/promise.ts
@@ -463,7 +463,15 @@ Zone.__load_patch('ZoneAwarePromise', (global: any, Zone: ZoneType, api: _ZonePr
         onFulfilled?: ((value: R) => TResult1 | PromiseLike<TResult1>)|undefined|null,
         onRejected?: ((reason: any) => TResult2 | PromiseLike<TResult2>)|undefined|
         null): Promise<TResult1|TResult2> {
-      let C = (this.constructor as any)[Symbol.species];
+      // We must read `Symbol.species` safely because `this` may be anything. For instance, `this`
+      // may be an object without a prototype (created through `Object.create(null)`); thus
+      // `this.constructor` will be undefined. One of the use cases is SystemJS creating
+      // prototype-less objects (modules) via `Object.create(null)`. The SystemJS creates an empty
+      // object and copies promise properties into that object (within the `getOrCreateLoad`
+      // function). The zone.js then checks if the resolved value has the `then` method and invokes
+      // it with the `value` context. Otherwise, this will throw an error: `TypeError: Cannot read
+      // properties of undefined (reading 'Symbol(Symbol.species)')`.
+      let C = (this.constructor as any)?.[Symbol.species];
       if (!C || typeof C !== 'function') {
         C = this.constructor || ZoneAwarePromise;
       }
@@ -483,7 +491,8 @@ Zone.__load_patch('ZoneAwarePromise', (global: any, Zone: ZoneType, api: _ZonePr
     }
 
     finally<U>(onFinally?: () => U | PromiseLike<U>): Promise<R> {
-      let C = (this.constructor as any)[Symbol.species];
+      // See comment on the call to `then` about why thee `Symbol.species` is safely accessed.
+      let C = (this.constructor as any)?.[Symbol.species];
       if (!C || typeof C !== 'function') {
         C = ZoneAwarePromise;
       }
diff --git a/packages/zone.js/test/common/Promise.spec.ts b/packages/zone.js/test/common/Promise.spec.ts
@@ -116,6 +116,32 @@ describe(
         expect(new MyPromise(() => {}).then(() => null) instanceof MyPromise).toBe(true);
       });
 
+      it('should allow subclassing without Symbol.species if properties are copied (SystemJS case)',
+         () => {
+           let value: any = null;
+           const promise = Promise.resolve();
+           const systemjsModule = Object.create(null);
+
+           // We only copy properties from the `promise` instance onto the `systemjsModule` object.
+           // This is what SystemJS is doing internally:
+           // https://github.com/systemjs/systemjs/blob/main/src/system-core.js#L107-L113
+           for (const property in promise) {
+             const value: any = promise[property as keyof typeof promise];
+             if (!(value in systemjsModule) || systemjsModule[property] !== value) {
+               systemjsModule[property] = value;
+             }
+           }
+
+           queueZone.run(() => {
+             Promise.resolve().then(() => systemjsModule).then((v) => (value = v));
+             flushMicrotasks();
+             // Note: we want to ensure that the promise has been resolved. In this specific case
+             // the promise may resolve to different values in the browser and on the Node.js side.
+             // SystemJS runs only in the browser and it only needs the promise to be resolved.
+             expect(value).not.toEqual(null);
+           });
+         });
+
       it('should allow subclassing with Symbol.species', () => {
         class MyPromise extends Promise<any> {
           constructor(fn: any) {
