From 06bf3a30c65d85f18aeb2ef2b800e918c8d36979 Mon Sep 17 00:00:00 2001
From: Nicholas Jamieson <nicholas@cartant.com>
Date: Fri, 3 Apr 2020 11:11:49 +1000
Subject: [PATCH] fix(fetch): don't leak event listeners added to passed-in
 signals (#5305)

* test(fetch): add failing test

* fix(fetch): don't leak signal listeners

* chore: improve an existing test's description
---
 spec/observables/dom/fetch-spec.ts   | 17 ++++++++++++++++-
 src/internal/observable/dom/fetch.ts | 23 ++++++++++++++---------
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/spec/observables/dom/fetch-spec.ts b/spec/observables/dom/fetch-spec.ts
index da9e6bb4d2..4a5d97f262 100644
--- a/spec/observables/dom/fetch-spec.ts
+++ b/spec/observables/dom/fetch-spec.ts
@@ -204,7 +204,7 @@ describe('fromFetch', () => {
     expect(mockFetch.calls[0].init.method).to.equal('HEAD');
   });
 
-  it('should pass in a signal with the init object without mutating the init', done => {
+  it('should add a signal to internal init object without mutating the passed init object', done => {
     const myInit = {method: 'DELETE'};
     const fetch$ = fromFetch('/bar', myInit);
     fetch$.subscribe({
@@ -247,4 +247,19 @@ describe('fromFetch', () => {
     // The subscription will not be closed until the error fires when the promise resolves.
     expect(subscription.closed).to.be.false;
   });
+
+  it('should not leak listeners added to the passed in signal', done => {
+    const controller = new MockAbortController();
+    const signal = controller.signal as any;
+    const fetch$ = fromFetch('/foo', { signal });
+    const subscription = fetch$.subscribe();
+    subscription.add(() => {
+      try {
+        expect(signal._listeners).to.be.empty;
+        done();
+      } catch (error) {
+        done(error);
+      }
+    });
+  });
 });
diff --git a/src/internal/observable/dom/fetch.ts b/src/internal/observable/dom/fetch.ts
index 50c9df010d..0a7e6ab2b6 100644
--- a/src/internal/observable/dom/fetch.ts
+++ b/src/internal/observable/dom/fetch.ts
@@ -1,4 +1,5 @@
 import { Observable } from '../../Observable';
+import { Subscription } from '../../Subscription';
 
 /**
  * Uses [the Fetch API](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API) to
@@ -54,10 +55,17 @@ export function fromFetch(input: string | Request, init?: RequestInit): Observab
   return new Observable<Response>(subscriber => {
     const controller = new AbortController();
     const signal = controller.signal;
-    let outerSignalHandler: () => void;
     let abortable = true;
     let unsubscribed = false;
 
+    const subscription = new Subscription();
+    subscription.add(() => {
+      unsubscribed = true;
+      if (abortable) {
+        controller.abort();
+      }
+    });
+
     let perSubscriberInit: RequestInit;
     if (init) {
       // If a signal is provided, just have it teardown. It's a cancellation token, basically.
@@ -65,12 +73,14 @@ export function fromFetch(input: string | Request, init?: RequestInit): Observab
         if (init.signal.aborted) {
           controller.abort();
         } else {
-          outerSignalHandler = () => {
+          const outerSignal = init.signal;
+          const outerSignalHandler = () => {
             if (!signal.aborted) {
               controller.abort();
             }
           };
-          init.signal.addEventListener('abort', outerSignalHandler);
+          outerSignal.addEventListener('abort', outerSignalHandler);
+          subscription.add(() => outerSignal.removeEventListener('abort', outerSignalHandler));
         }
       }
       // init cannot be mutated or reassigned as it's closed over by the
@@ -92,11 +102,6 @@ export function fromFetch(input: string | Request, init?: RequestInit): Observab
       }
     });
 
-    return () => {
-      unsubscribed = true;
-      if (abortable) {
-        controller.abort();
-      }
-    };
+    return subscription;
   });
 }