Return to Attach After Error

If the player sees an error and we are attached to a media element, we
should return to the attach state because we assume that the media
element is still usable after an error.

If we keep throwing away the media element, it means that after each
error, an app would need to re-attach to the media element. This will
break backwards-compatibility.

This CL restores backwards-compatibility by resetting the walker to be
at the attached-state if it has a media element.

Issue #816
Issue #997
Close #1843

Change-Id: Idaa70d9fcc01cd9af06ff8967812a4051f8c6e53

lib/player.js

@@ -278,18 +278,19 @@ shaka.Player = function(mediaElement, dependencyInjector) {
         shaka.log.warning('Error Stack:', error.stack);
       }
 
-      // TODO(vaage): We don't always need to return to detached. If we have a
-      //              video element we may want to return to attached. We should
-      //              have this so that we return to |attachNode| when
-      //              |has.mediaElement != null|. We will need to have proper
-      //              testing before we make that change.
-
-      // If there is an error, reset us back to the detached state so that
-      // we will have a clean slate to restart from.
+      // Regardless of what state we were in, if there is an error, we unload.
+      // This ensures that any initialized system will be torn-down and we will
+      // go back to a safe foundation. We assume that the media element is
+      // always safe to use after an error.
       await this.onUnload_(has, this.createEmptyPayload_());
-      await this.onDetach_(has, this.createEmptyPayload_());
 
-      return this.detachNode_;
+      // There are only two nodes that come before we start loading content,
+      // attach and detach. If we have a media element, it means we were
+      // attached to the element, and we can safely return to the attach state
+      // (we assume that the video element is always re-usable). We favor
+      // returning to the attach node since it means that the app won't need to
+      // re-attach if it saw an error.
+      return has.mediaElement ? this.attachNode_ : this.detachNode_;
     },
     onIdle: (node) => {
       this.dispatchEvent(new shaka.util.FakeEvent(

test/player_integration.js

@@ -1059,6 +1059,49 @@ describe('Player Load Path', () => {
     });
   });
 
+  describe('error handling', () => {
+    beforeEach(() => {
+      createPlayer(/* attachedTo= */ null);
+    });
+
+    it('returns to attach after load error', async () => {
+      // The easiest way we can inject an error is to fail fetching the
+      // manifest. To do this, we force the network request by throwing an error
+      // in a request filter. The specific error does not matter.
+      const networkingEngine = player.getNetworkingEngine();
+      expect(networkingEngine).toBeTruthy();
+      networkingEngine.registerRequestFilter(() => {
+        throw new shaka.util.Error(
+            shaka.util.Error.Severity.CRITICAL,
+            shaka.util.Error.Category.NETWORK,
+            shaka.util.Error.Code.REQUEST_FILTER_ERROR);
+      });
+
+      /** @type {jasmine.Spy} */
+      const idleSpy = jasmine.createSpy('idle state');
+      player.addEventListener('onstateidle', shaka.test.Util.spyFunc(idleSpy));
+
+      // Make the two requests one-after-another so that we don't have any idle
+      // time between them.
+      const attachRequest = player.attach(video);
+      const loadRequest = player.load('test:sintel');
+
+      await attachRequest;
+      await rejected(loadRequest);
+
+      // Wait a couple interrupter cycles to allow the player to enter idle
+      // state.
+      const event = await new Promise((resolve) => {
+        idleSpy.and.callFake(resolve);
+      });
+
+      // Since attached and loaded in the same interrupter cycle, there won't be
+      // any idle time until we finish failing to load. We expect to idle in
+      // attach.
+      expect(event.state).toBe('attach');
+    });
+  });
+
   /**
    * Wait for |p| to be rejected. If |p| is not rejected, this will fail the
    * test;