PR feedback

Author: mgurgel

injected/integration-test/duckplayer-native.spec.js

@@ -170,3 +170,13 @@ test.describe('Duck Player Native custom error view', () => {
         await duckPlayer.didShowUnknownError();
     });
 });
+
+test.describe('Reporting exceptions', () => {
+    test('initial setup error', async ({ page }, workerInfo) => {
+        const duckPlayer = DuckPlayerNative.create(page, workerInfo);
+        await duckPlayer.withRemoteConfig();
+        await duckPlayer.messagingError();
+        await duckPlayer.gotoBlankPage();
+        await duckPlayer.didSendMessagingException();
+    });
+});

injected/integration-test/page-objects/duckplayer-native.js

@@ -11,12 +11,16 @@ import { ResultsCollector } from './results-collector.js';
 // eslint-disable-next-line @typescript-eslint/no-unused-vars
 const configFiles = /** @type {const} */ (['native.json']);
 
-const defaultInitialSetup = {
+const featureName = 'duckPlayerNative';
+
+/** @type {PageType} */
+const DEFAULT_PAGE_TYPE = 'YOUTUBE';
+
+/** @type {import('../../src/features/duck-player-native.js').InitialSettings} */
+const DEFAULT_INITIAL_SETUP = {
     locale: 'en',
 };
 
-const featureName = 'duckPlayerNative';
-
 export class DuckPlayerNative {
     /** @type {Partial<Record<PageType, string>>} */
     pages = {
@@ -36,7 +40,7 @@ export class DuckPlayerNative {
         this.isMobile = platform.name === 'android' || platform.name === 'ios';
         this.collector = new ResultsCollector(page, build, platform);
         this.collector.withMockResponse({
-            initialSetup: defaultInitialSetup,
+            initialSetup: DEFAULT_INITIAL_SETUP,
             onCurrentTimestamp: {},
         });
         this.collector.withUserPreferences({
@@ -86,13 +90,22 @@ export class DuckPlayerNative {
     }
 
     /**
-     * @param {PageType} pageType
+     * Goes to the default page (YOUTUBE) without overriding initialSetup
+     * Useful for testing messaging errors
+     **/
+    async gotoBlankPage() {
+        await this.gotoPage();
+    }
+
+    /**
+     * @param {PageType} [pageType]
      * @param {object} [params]
      * @param {PlayerPageVariants} [params.variant]
      * @param {string} [params.videoID]
      */
     async gotoPage(pageType, params = {}) {
-        await this.withPageType(pageType);
+        if (pageType) await this.withPageType(pageType);
+        const page = this.pages[pageType || DEFAULT_PAGE_TYPE];
 
         const defaultVariant = this.isMobile ? 'mobile' : 'default';
         const { variant = defaultVariant, videoID = '123' } = params;
@@ -101,8 +114,6 @@ export class DuckPlayerNative {
             ['variant', variant],
         ]);
 
-        const page = this.pages[pageType];
-
         await this.page.goto(page + '?' + urlParams.toString());
     }
 
@@ -122,7 +133,7 @@ export class DuckPlayerNative {
      * @return {Promise<void>}
      */
     async withPageType(pageType) {
-        const initialSetup = this.collector.mockResponses?.initialSetup || defaultInitialSetup;
+        const initialSetup = this.collector.mockResponses?.initialSetup || DEFAULT_INITIAL_SETUP;
 
         await this.collector.updateMockResponse({
             initialSetup: { pageType, ...initialSetup },
@@ -134,7 +145,7 @@ export class DuckPlayerNative {
      * @return {Promise<void>}
      */
     async withPlaybackPaused(playbackPaused = true) {
-        const initialSetup = this.collector.mockResponses.initialSetup || defaultInitialSetup;
+        const initialSetup = this.collector.mockResponses.initialSetup || DEFAULT_INITIAL_SETUP;
 
         await this.collector.updateMockResponse({
             initialSetup: { playbackPaused, ...initialSetup },
@@ -206,6 +217,34 @@ export class DuckPlayerNative {
         await this.simulateSubscriptionMessage('onUrlChanged', { pageType });
     }
 
+    /**
+     * Simulates a messaging error by passing an empty initialSetup object
+     */
+    async messagingError() {
+        await this.build.switch({
+            android: async () => {
+                await this.collector.updateMockResponse({
+                    initialSetup: {},
+                });
+            },
+            apple: async () => {
+                await this.collector.updateMockResponse({
+                    initialSetup: null,
+                });
+            },
+            'apple-isolated': async () => {
+                await this.collector.updateMockResponse({
+                    initialSetup: null,
+                });
+            },
+            windows: async () => {
+                await this.collector.updateMockResponse({
+                    initialSetup: '',
+                });
+            },
+        });
+    }
+
     /* Messaging assertions */
 
     async didSendInitialHandshake() {
@@ -236,6 +275,42 @@ export class DuckPlayerNative {
         ]);
     }
 
+    /**
+     * @param {string} kind
+     * @param {string} message
+     */
+    async didSendException(kind, message, context = 'contentScopeScripts') {
+        const messages = await this.collector.waitForMessage('reportMetric');
+        expect(messages).toMatchObject([
+            {
+                payload: {
+                    context,
+                    featureName: 'duckPlayerNative',
+                    method: 'reportMetric',
+                    params: { metricName: 'exception', params: { kind, message } },
+                },
+            },
+        ]);
+    }
+
+    async didSendMessagingException() {
+        await this.build.switch({
+            android: async () => {
+                // Android produces a TypeError due to how its messaging lib is wired up
+                await this.didSendException('TypeError', "Cannot read properties of undefined (reading 'privatePlayerMode')");
+            },
+            apple: async () => {
+                await this.didSendException('MessagingError', 'an unknown error occurred');
+            },
+            'apple-isolated': async () => {
+                await this.didSendException('MessagingError', 'an unknown error occurred', 'contentScopeScriptsIsolated');
+            },
+            windows: async () => {
+                await this.didSendException('MessagingError', 'unknown error');
+            },
+        });
+    }
+
     /* Thumbnail Overlay assertions */
 
     async didShowOverlay() {

injected/src/features/duck-player-native.js

@@ -4,6 +4,7 @@ import { DuckPlayerNativeMessages } from './duckplayer-native/messages.js';
 import { setupDuckPlayerForNoCookie, setupDuckPlayerForSerp, setupDuckPlayerForYouTube } from './duckplayer-native/sub-feature.js';
 import { Environment } from './duckplayer/environment.js';
 import { Logger } from './duckplayer/util.js';
+import { MetricsReporter, EXCEPTION_KIND_INITIAL_SETUP_ERROR } from '../../../special-pages/shared/metrics-reporter.js';
 
 /**
  * @import {DuckPlayerNativeSubFeature} from './duckplayer-native/sub-feature.js'
@@ -14,8 +15,8 @@ import { Logger } from './duckplayer/util.js';
 /**
  * @typedef InitialSettings - The initial payload used to communicate render-blocking information
  * @property {string} locale - UI locale
- * @property {UrlChangeSettings['pageType']} pageType - The type of the current page
- * @property {boolean} playbackPaused - Should video start playing or paused
+ * @property {UrlChangeSettings['pageType']} [pageType] - The type of the current page
+ * @property {boolean} [playbackPaused] - Should video start playing or paused
  */
 
 /**
@@ -29,30 +30,46 @@ export class DuckPlayerNativeFeature extends ContentFeature {
     /** @type {TranslationFn} */
     t;
 
-    async init(args) {
+    init(args) {
         /**
          * This feature never operates in a frame
          */
         if (isBeingFramed()) return;
 
-        const selectors = this.getFeatureSetting('selectors');
-        if (!selectors) {
-            console.warn('No selectors found. Check remote config. Feature will not be initialized.');
-            return;
-        }
-
         const locale = args?.locale || args?.language || 'en';
         const env = new Environment({
             debug: this.isDebug,
             injectName: import.meta.injectName,
             platform: this.platform,
             locale,
         });
+        const metrics = new MetricsReporter(this.messaging);
+
+        try {
+            const selectors = this.getFeatureSetting('selectors');
+            if (!selectors) {
+                console.warn('No selectors found. Check remote config. Feature will not be initialized.');
+                return;
+            }
 
-        // Translation function to be used by view components
-        this.t = (key) => env.strings('native.json')[key];
+            // Translation function to be used by view components
+            this.t = (key) => env.strings('native.json')[key];
+
+            const messages = new DuckPlayerNativeMessages(this.messaging, env);
+            this.initDuckPlayerNative(messages, selectors, env)
+                // Using then instead of await because this is the public interface of the parent, which doesn't explicitly wait for promises to be resolved.
+                // eslint-disable-next-line promise/prefer-await-to-then
+                .catch((e) => {
+                    console.error(e);
+                    metrics.reportExceptionWithError(e);
+                });
+        } catch (e) {
+            console.error(e);
+            metrics.reportExceptionWithError(e);
+        }
+    }
 
-        const messages = new DuckPlayerNativeMessages(this.messaging, env);
+    async initDuckPlayerNative(messages, selectors, env) {
         messages.subscribeToURLChange(({ pageType }) => {
             const playbackPaused = false; // This can be added to the event data in the future if needed
             this.urlDidChange(pageType, selectors, playbackPaused, env, messages);
@@ -64,7 +81,14 @@ export class DuckPlayerNativeFeature extends ContentFeature {
         try {
             initialSetup = await messages.initialSetup();
         } catch (e) {
-            console.warn('Failed to get initial setup', e);
+            console.warn(e);
+            return;
+        }
+
+        if (!initialSetup) {
+            const message = 'InitialSetup data is missing';
+            console.warn(message);
+            messages.metrics.reportException({ message, kind: EXCEPTION_KIND_INITIAL_SETUP_ERROR });
             return;
         }
 
@@ -118,7 +142,7 @@ export class DuckPlayerNativeFeature extends ContentFeature {
             if (document.readyState === 'loading') {
                 const loadHandler = () => {
                     logger.log('Running deferred load handlers');
-                    nextPage.onLoad();
+                    nextPage?.onLoad();
                     messages.notifyScriptIsReady();
                 };
                 document.addEventListener('DOMContentLoaded', loadHandler, { once: true });

injected/src/features/duck-player.js

@@ -61,7 +61,7 @@ import { MetricsReporter } from '../../../special-pages/shared/metrics-reporter.
  * @internal
  */
 export default class DuckPlayerFeature extends ContentFeature {
-    async init(args) {
+    init(args) {
         /**
          * This feature never operates in a frame
          */
@@ -93,6 +93,7 @@ export default class DuckPlayerFeature extends ContentFeature {
         if (!this.messaging) {
             throw new Error('cannot operate duck player without a messaging backend');
         }
+        const metrics = new MetricsReporter(this.messaging);
 
         try {
             const locale = args?.locale || args?.language || 'en';
@@ -105,12 +106,18 @@ export default class DuckPlayerFeature extends ContentFeature {
             const comms = new DuckPlayerOverlayMessages(this.messaging, env);
 
             if (overlaysEnabled) {
-                await initOverlays(overlaySettings.youtube, env, comms);
+                initOverlays(overlaySettings.youtube, env, comms)
+                    // Using then instead of await because this is the public interface of the parent, which doesn't explicitly wait for promises to be resolved.
+                    // eslint-disable-next-line promise/prefer-await-to-then
+                    .catch((e) => {
+                        console.error(e);
+                        metrics.reportExceptionWithError(e);
+                    });
             } else if (serpProxyEnabled) {
                 comms.serpProxy();
             }
         } catch (e) {
-            const metrics = new MetricsReporter(this.messaging);
+            console.error(e);
             metrics.reportExceptionWithError(e);
         }
     }

injected/src/features/duckplayer-native/messages.js

@@ -1,4 +1,5 @@
 import * as constants from './constants.js';
+import { MetricsReporter, EXCEPTION_KIND_MESSAGING_ERROR } from '../../../../special-pages/shared/metrics-reporter.js';
 
 /** @import {YouTubeError} from './error-detection.js' */
 /** @import {Environment} from '../duckplayer/environment.js' */
@@ -42,13 +43,20 @@ export class DuckPlayerNativeMessages {
          */
         this.messaging = messaging;
         this.environment = environment;
+        this.metrics = new MetricsReporter(messaging);
     }
 
     /**
      * @returns {Promise<import('../duck-player-native.js').InitialSettings>}
      */
-    initialSetup() {
-        return this.messaging.request(constants.MSG_NAME_INITIAL_SETUP);
+    async initialSetup() {
+        try {
+            console.log('initialSetup');
+            return await this.messaging.request(constants.MSG_NAME_INITIAL_SETUP);
+        } catch (e) {
+            this.metrics.reportException({ message: e?.message, kind: EXCEPTION_KIND_MESSAGING_ERROR });
+            throw e;
+        }
     }
 
     /**

special-pages/shared/metrics-reporter.js

@@ -157,6 +157,7 @@ export class MetricsReporter {
      * ```
      */
     reportException(params) {
+        console.log('reportException', params);
         const metric = this._createExceptionMetric(params);
         this._sendMetric(metric);
     }