fix(video360): prevent teardown crash when closing 360 video preview (#1692)

Co-authored-by: zhirongwang <zhirongwang@box.com>

Author: zhirongwang

src/lib/viewers/box3d/video360/Video360Viewer.js

@@ -22,7 +22,7 @@ class Video360Viewer extends DashViewer {
     renderer;
 
     /** @property {Video360Controls} - Instance of the Video360Controls */
-    controls;
+    video360Controls;
 
     /** @property {Box3D.Texture2DAsset} - Asset for the skybox texture */
     textureAsset;
@@ -65,6 +65,14 @@ class Video360Viewer extends DashViewer {
 
     /** @inheritdoc */
     destroy() {
+        // Tear down 360 controls before super.destroy(), which destroys this.controls
+        // (DashViewer's React ControlsRoot when createControls() has not run yet).
+        if (this.video360Controls) {
+            this.destroyControls();
+            this.video360Controls = null;
+            this.controls = null;
+        }
+
         super.destroy();
 
         if (this.skybox) {
@@ -82,11 +90,6 @@ class Video360Viewer extends DashViewer {
             this.videoAsset = null;
         }
 
-        if (this.controls) {
-            this.destroyControls();
-            this.controls = null;
-        }
-
         if (this.renderer) {
             // Remove event listeners from box3d, if any on it
             this.renderer.getBox3D().off('mouseDown', this.onCanvasMouseDown);
@@ -137,8 +140,10 @@ class Video360Viewer extends DashViewer {
      * @return {void}
      */
     createControls() {
-        this.controls = new Video360Controls(this.mediaContainerEl);
-        this.controls.on(EVENT_TOGGLE_VR, this.handleToggleVr);
+        this.video360Controls = new Video360Controls(this.mediaContainerEl);
+        // Overwrite this.controls so DashViewer.renderUI() skips React controls.
+        this.controls = this.video360Controls;
+        this.video360Controls.on(EVENT_TOGGLE_VR, this.handleToggleVr);
 
         // Add listeners to hide and show controls
         if (!this.renderer || !this.renderer.getBox3D()) {
@@ -163,8 +168,12 @@ class Video360Viewer extends DashViewer {
      * @return {void}
      */
     destroyControls() {
-        this.controls.removeListener(EVENT_TOGGLE_VR, this.handleToggleVr);
-        this.controls.destroy();
+        if (!this.video360Controls) {
+            return;
+        }
+
+        this.video360Controls.removeListener(EVENT_TOGGLE_VR, this.handleToggleVr);
+        this.video360Controls.destroy();
 
         // Remove listeners to hide and show controls
         if (!this.renderer || !this.renderer.getBox3D()) {
@@ -250,7 +259,9 @@ class Video360Viewer extends DashViewer {
      * @return {void}
      */
     handleShowVrButton() {
-        this.controls.showVrButton();
+        if (this.video360Controls) {
+            this.video360Controls.showVrButton();
+        }
     }
 
     /**

src/lib/viewers/box3d/video360/__tests__/Video360Viewer-test.js

@@ -118,15 +118,28 @@ describe('lib/viewers/box3d/video360/Video360Viewer', () => {
             expect(videoAsset.destroy).toBeCalled();
         });
 
-        test('should invoke .destroyControls() if it exists', () => {
+        test('should invoke .destroyControls() if video360Controls exists', () => {
             const destroyStub = jest.spyOn(viewer, 'destroyControls').mockImplementation();
 
-            viewer.controls = {};
+            viewer.video360Controls = {};
             viewer.destroy();
 
             expect(destroyStub).toBeCalled();
         });
 
+        test('should not invoke .destroyControls() when only DashViewer React controls exist', () => {
+            const destroyStub = jest.spyOn(viewer, 'destroyControls').mockImplementation();
+
+            viewer.controls = {
+                destroy: jest.fn(),
+                render: jest.fn(),
+            };
+            viewer.destroy();
+
+            expect(destroyStub).not.toBeCalled();
+            expect(viewer.controls.destroy).toBeCalled();
+        });
+
         describe('if renderer exists', () => {
             let rendererMock;
             let b3dMock;
@@ -282,12 +295,14 @@ describe('lib/viewers/box3d/video360/Video360Viewer', () => {
         });
 
         afterEach(() => {
+            viewer.video360Controls = null;
             viewer.controls = null;
         });
 
         test('should create and store an instance of Video360Controls', () => {
             viewer.createControls();
-            expect(viewer.controls).toBeInstanceOf(Video360Controls);
+            expect(viewer.video360Controls).toBeInstanceOf(Video360Controls);
+            expect(viewer.controls).toBe(viewer.video360Controls);
         });
 
         test('should attach event handler for vr toggle by invoking .controls.on() with EVENT_TOGGLE_VR and .handleToggleVr()', () => {
@@ -319,7 +334,7 @@ describe('lib/viewers/box3d/video360/Video360Viewer', () => {
                 removeListener: jest.fn(),
                 destroy: jest.fn(),
             };
-            viewer.controls = controls;
+            viewer.video360Controls = controls;
             viewer.destroyControls();
             canvas = {
                 removeEventListener: jest.fn(),
@@ -336,14 +351,15 @@ describe('lib/viewers/box3d/video360/Video360Viewer', () => {
         });
 
         afterEach(() => {
+            viewer.video360Controls = null;
             viewer.controls = null;
         });
 
-        test('should remove event handler for EVENT_TOGGLE_VR by invoking .controls.removeListener() with EVENT_TOGGLE_VR and .handleToggleVr()', () => {
+        test('should remove event handler for EVENT_TOGGLE_VR by invoking .video360Controls.removeListener() with EVENT_TOGGLE_VR and .handleToggleVr()', () => {
             expect(controls.removeListener).toBeCalledWith(EVENT_TOGGLE_VR, viewer.handleToggleVr);
         });
 
-        test('should invoke .controls.destroy()', () => {
+        test('should invoke .video360Controls.destroy()', () => {
             expect(controls.destroy).toBeCalled();
         });
 
@@ -552,14 +568,14 @@ describe('lib/viewers/box3d/video360/Video360Viewer', () => {
     });
 
     describe('handleShowVrButton()', () => {
-        test('should invoke .controls.showVrButton()', () => {
-            viewer.controls = {
+        test('should invoke .video360Controls.showVrButton()', () => {
+            viewer.video360Controls = {
                 showVrButton: jest.fn(),
             };
             viewer.handleShowVrButton();
-            expect(viewer.controls.showVrButton).toBeCalled();
+            expect(viewer.video360Controls.showVrButton).toBeCalled();
 
-            viewer.controls = null;
+            viewer.video360Controls = null;
         });
     });