From 6910648ec6cef6920e7b363fbf5122e2f1c3e852 Mon Sep 17 00:00:00 2001
From: Joey Parrish <joeyparrish@google.com>
Date: Fri, 26 Jan 2018 14:27:15 -0800
Subject: [PATCH] Add one more test for AbortableOperation

This is a regression test for an implementation bug we caught before
it was released.  It took a little longer to get the regression test
working, so this was deferred to a later commit.

Issue #829

Change-Id: I272ebe0e6fa2f0ac462c3a3d236250242f7d7192
---
 lib/util/abortable_operation.js       | 11 ++++---
 test/util/abortable_operation_unit.js | 42 +++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/lib/util/abortable_operation.js b/lib/util/abortable_operation.js
index fd489554c4..04cd421947 100644
--- a/lib/util/abortable_operation.js
+++ b/lib/util/abortable_operation.js
@@ -61,7 +61,7 @@ shaka.util.AbortableOperation = class {
   static failed(error) {
     return new shaka.util.AbortableOperation(
         Promise.reject(error),
-        Promise.resolve);
+        () => Promise.resolve());
   }
 
   /**
@@ -74,7 +74,7 @@ shaka.util.AbortableOperation = class {
             shaka.util.Error.Severity.CRITICAL,
             shaka.util.Error.Category.PLAYER,
             shaka.util.Error.Code.OPERATION_ABORTED)),
-        Promise.resolve);
+        () => Promise.resolve());
   }
 
   /**
@@ -86,7 +86,7 @@ shaka.util.AbortableOperation = class {
   static completed(value) {
     return new shaka.util.AbortableOperation(
         Promise.resolve(value),
-        Promise.resolve);
+        () => Promise.resolve());
   }
 
   /**
@@ -226,6 +226,9 @@ shaka.util.AbortableOperation = class {
         // callback, and the new promise should be tied to the promise
         // from the callback's operation.
         newPromise.resolve(ret.promise);
+        // This used to say "return ret.abort;", but it caused subtle issues by
+        // unbinding part of the abort chain.  There is now a test to ensure
+        // that we don't call abort with the wrong "this".
         return () => ret.abort();
       } else {
         // This is a Promise or a plain value, and this step cannot be aborted.
@@ -238,7 +241,7 @@ shaka.util.AbortableOperation = class {
       // The callback threw an exception or error.  Reject the new Promise and
       // resolve any future abort call right away.
       newPromise.reject(exception);
-      return Promise.resolve;
+      return () => Promise.resolve();
     }
   }
 };
diff --git a/test/util/abortable_operation_unit.js b/test/util/abortable_operation_unit.js
index e58a9fae3c..d157776673 100644
--- a/test/util/abortable_operation_unit.js
+++ b/test/util/abortable_operation_unit.js
@@ -487,5 +487,47 @@ describe('AbortableOperation', function() {
         shaka.test.Util.expectToEqualError(e, error);
       }).finally(done);
     });
+
+    it('ensures abort is called with the correct "this"', function(done) {
+      // During testing and development, an early version of chain() would
+      // sometimes unbind an abort method from an earlier stage of the chain.
+      // Make sure this doesn't happen.
+      var innerOperation;
+      var p = new shaka.util.PublicPromise();
+      var abortCalled = false;
+
+      /**
+       * @this {shaka.util.AbortableOperation}
+       * @return {!Promise}
+       *
+       * NOTE: This is a subtle thing, but this must be an ES5 anonymous
+       * function for the test to work.  ES6 arrow functions would always be
+       * called with the "this" of the test itself, regardless of what the
+       * library is doing.
+       */
+      var abort = function() {
+        expect(this).toBe(innerOperation);
+        abortCalled = true;
+        return Promise.resolve();
+      };
+
+      // Since the issue was with the calling of operation.abort, rather than
+      // the onAbort_ callback, we make an operation-like thing instead of using
+      // the AbortableOperation constructor.
+      innerOperation = { promise: p, abort: abort };
+
+      // The second stage of the chain returns innerOperation.  A brief moment
+      // later, the outer chain is aborted.
+      var operation = shaka.util.AbortableOperation.completed(100).chain(() => {
+        shaka.test.Util.delay(0.1).then(() => {
+          operation.abort();
+          p.resolve();
+        });
+        return innerOperation;
+      }).finally(() => {
+        expect(abortCalled).toBe(true);
+        done();
+      });
+    });
   });  // describe('chain')
 });  // describe('AbortableOperation')