diff --git a/packages/analytics-browser/e2e/cookies-is-enabled.spec.ts b/packages/analytics-browser/e2e/cookies-is-enabled.spec.ts new file mode 100644 index 0000000000..f90b96417c --- /dev/null +++ b/packages/analytics-browser/e2e/cookies-is-enabled.spec.ts @@ -0,0 +1,26 @@ +import { test, expect } from '@playwright/test'; + +const RUNS = 10; + +test.describe('CookieStorage', () => { + // Regression Test to cover re-entrancy issues fixed by https://github.com/amplitude/Amplitude-TypeScript/pull/1539 + test('.isEnabled works when called multiple times concurrently', async ({ page }) => { + const consoleErrors: string[] = []; + page.on('console', (msg) => { + if (msg.type() === 'error') { + consoleErrors.push(msg.text()); + } + }); + + const pageErrors: string[] = []; + page.on('pageerror', (err) => { + pageErrors.push(err.message); + }); + + for (let i = 0; i < RUNS; i++) { + await page.goto('/cookies/is-enabled.html'); + expect(pageErrors, `Expected no page errors, but got: ${pageErrors.join('; ')}`).toHaveLength(0); + expect(consoleErrors, `Expected no console.error calls, but got: ${consoleErrors.join('; ')}`).toHaveLength(0); + } + }); +}); diff --git a/packages/analytics-browser/src/config.ts b/packages/analytics-browser/src/config.ts index 690a635a10..dbb3fb1bcd 100644 --- a/packages/analytics-browser/src/config.ts +++ b/packages/analytics-browser/src/config.ts @@ -31,6 +31,7 @@ import { isDomainEqual, CookieStorageConfig, decodeCookieValue, + getGlobalScope, } from '@amplitude/analytics-core'; import { LocalStorage } from './storage/local-storage'; @@ -295,7 +296,9 @@ export const useBrowserConfig = async ( const identityStorage = options.identityStorage || DEFAULT_IDENTITY_STORAGE; const cookieOptions = { domain: - identityStorage !== DEFAULT_IDENTITY_STORAGE ? '' : options.cookieOptions?.domain ?? (await getTopLevelDomain()), + identityStorage !== DEFAULT_IDENTITY_STORAGE + ? '' + : options.cookieOptions?.domain ?? (await getTopLevelDomain(undefined, diagnosticsClient)), expiration: 365, sameSite: 'Lax' as const, secure: false, @@ -468,9 +471,51 @@ export const createTransport = (transport?: TransportTypeOrOptions) => { return new FetchTransport(headers); }; -export const getTopLevelDomain = async (url?: string) => { +export const getTopLevelDomain = async (url?: string, diagnosticsClient?: IDiagnosticsClient) => { if ( - !(await new CookieStorage().isEnabled()) || + !(await new CookieStorage(undefined, { diagnosticsClient }).isEnabled()) || + (!url && (typeof location === 'undefined' || !location.hostname)) + ) { + return ''; + } + + const global = getGlobalScope(); + if (!global?.navigator?.locks) { + return legacyGetTopLevelDomain(url, diagnosticsClient); + } + + return await global.navigator.locks.request('com/amplitude/get-top-level-domain', async () => { + const host = url ?? location.hostname; + const parts = host.split('.'); + const levels = []; + const storageKey = 'AMP_TLDTEST'; + + for (let i = parts.length - 2; i >= 0; --i) { + levels.push(parts.slice(i).join('.')); + } + for (let i = 0; i < levels.length; i++) { + const domain = levels[i]; + const options = { domain: '.' + domain }; + const storage = new CookieStorage(options); + try { + await storage.set(storageKey, 1); + const value = await storage.get(storageKey); + if (value) { + console.log('TLD -- exiting with domain:', '.' + domain); + return '.' + domain; + } + } finally { + await storage.remove(storageKey); + } + } + + return ''; + }); +}; + +const legacyGetTopLevelDomain = async (url?: string, diagnosticsClient?: IDiagnosticsClient) => { + if ( + !(await new CookieStorage(undefined, { diagnosticsClient }).isEnabled()) || (!url && (typeof location === 'undefined' || !location.hostname)) ) { return ''; diff --git a/packages/analytics-browser/test/config.test.ts b/packages/analytics-browser/test/config.test.ts index 04ea13efd2..8a8f951367 100644 --- a/packages/analytics-browser/test/config.test.ts +++ b/packages/analytics-browser/test/config.test.ts @@ -393,6 +393,7 @@ describe('config', () => { ...testCookieStorage, options: {}, config: {}, + legacyIsEnabled: jest.fn().mockResolvedValueOnce(Promise.resolve(false)), }); const domain = await Config.getTopLevelDomain(); expect(domain).toBe(''); @@ -421,11 +422,13 @@ describe('config', () => { ...testCookieStorage, options: {}, config: {}, + legacyIsEnabled: jest.fn().mockResolvedValueOnce(Promise.resolve(true)), }) .mockReturnValue({ ...actualCookieStorage, options: {}, config: {}, + legacyIsEnabled: jest.fn().mockResolvedValueOnce(Promise.resolve(true)), }); expect(await Config.getTopLevelDomain('www.legislation.gov.uk')).toBe('.legislation.gov.uk'); }); @@ -461,6 +464,21 @@ describe('config', () => { configurable: true, }); }); + + test('should record a diagnostics event when no access to cookies', async () => { + const mockDiagnosticsClient = { + recordEvent: jest.fn(), + setTag: jest.fn(), + increment: jest.fn(), + recordHistogram: jest.fn(), + _flush: jest.fn(), + _setSampleRate: jest.fn(), + }; + await Config.getTopLevelDomain('www.amplitude.com', mockDiagnosticsClient); + expect(mockDiagnosticsClient.recordEvent).toHaveBeenCalledWith('cookies.tld.failure', { + reason: 'Could not determine TLD for host www.amplitude.com', + }); + }); }); describe('fetchRemoteConfig', () => { diff --git a/packages/analytics-browser/test/cookie-migration/index.test.ts b/packages/analytics-browser/test/cookie-migration/index.test.ts index 324cd226f5..c1e40e0162 100644 --- a/packages/analytics-browser/test/cookie-migration/index.test.ts +++ b/packages/analytics-browser/test/cookie-migration/index.test.ts @@ -1,7 +1,6 @@ -import { Storage, UserSession } from '@amplitude/analytics-core'; +import { Storage, UserSession, MemoryStorage, CookieStorage, getOldCookieName } from '@amplitude/analytics-core'; import { decode, parseLegacyCookies, parseTime } from '../../src/cookie-migration'; import * as LocalStorageModule from '../../src/storage/local-storage'; -import { MemoryStorage, CookieStorage, getOldCookieName } from '@amplitude/analytics-core'; describe('cookie-migration', () => { const API_KEY = 'asdfasdf'; diff --git a/packages/analytics-core/src/storage/cookie.ts b/packages/analytics-core/src/storage/cookie.ts index 526da6a2f8..4d36db1355 100644 --- a/packages/analytics-core/src/storage/cookie.ts +++ b/packages/analytics-core/src/storage/cookie.ts @@ -35,6 +35,59 @@ export class CookieStorage implements Storage { if (!globalScope || !globalScope.document) { return false; } + const { navigator } = globalScope; + if (!navigator?.locks) { + return await this.legacyIsEnabled(); + } + + return await navigator.locks.request('com/amplitude/cookie-storage-is-enabled', async () => { + const testValue = String(Date.now()); + const testCookieOptions = { ...this.options }; + const testStorage = new CookieStorage(testCookieOptions); + const testKey = 'AMP_TEST'; + try { + await testStorage.set(testKey, testValue); + const value = await testStorage.get(testKey); + + /* istanbul ignore next */ + if (value !== testValue && this.config.diagnosticsClient) { + this.config.diagnosticsClient?.recordEvent('cookies.isEnabled.failure', { + reason: 'Test Value mismatch', + testKey, + testValue, + }); + } + console.log('isEnabled -- exiting with result:', value === testValue); + return value === testValue; + } catch (e){ + /* istanbul ignore next */ + if (this.config.diagnosticsClient) { + const errMessage = e instanceof Error ? e.message : String(e); + this.config.diagnosticsClient?.recordEvent('cookies.isEnabled.failure', { + reason: 'Cookie getter/setter failed', + testKey, + testValue, + error: errMessage, + }); + } + return false; + } finally { + await testStorage.remove(testKey); + } + }); + } + + /** + * Detects if cookies are enabled by setting a test cookie and checking if it is set. + * Legacy version that doesn't use navigator.locks + * @returns boolean + */ + async legacyIsEnabled(): Promise { + const globalScope = getGlobalScope(); + /* istanbul ignore if */ + if (!globalScope || !globalScope.document) { + return false; + } const testValue = String(Date.now()); const testCookieOptions = { @@ -46,9 +99,26 @@ export class CookieStorage implements Storage { try { await testStorage.set(testKey, testValue); const value = await testStorage.get(testKey); + /* istanbul ignore next */ + if (value !== testValue && this.config.diagnosticsClient) { + this.config.diagnosticsClient?.recordEvent('cookies.isEnabled.failure', { + reason: 'Test Value mismatch', + testKey, + testValue, + }); + } return value === testValue; - } catch { + } catch (e) { /* istanbul ignore next */ + if (this.config.diagnosticsClient) { + const errMessage = e instanceof Error ? e.message : String(e); + this.config.diagnosticsClient?.recordEvent('cookies.isEnabled.failure', { + reason: 'Cookie getter/setter failed', + testKey, + testValue, + error: errMessage, + }); + } return false; } finally { await testStorage.remove(testKey); diff --git a/packages/analytics-core/test/storage/cookies.test.ts b/packages/analytics-core/test/storage/cookies.test.ts index 2f8d28f642..d50cbb5e19 100644 --- a/packages/analytics-core/test/storage/cookies.test.ts +++ b/packages/analytics-core/test/storage/cookies.test.ts @@ -9,22 +9,119 @@ import * as GlobalScopeModule from '../../src/global-scope'; describe('cookies', () => { describe('isEnabled', () => { - test('regression test re-entrancy issue', async () => { - const c1 = new CookieStorage(); - const c2 = new CookieStorage(); - // calling isEnabled one after the other should not cause a re-entrancy issue - const promises = [c1.isEnabled(), c2.isEnabled()]; - const res = await Promise.all(promises); - expect(res).toEqual([true, true]); + describe('concurrent calls', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + afterEach(() => { + jest.useRealTimers(); + }); + + test('regression test re-entrancy issue', async () => { + const c1 = new CookieStorage(); + const c2 = new CookieStorage(); + // calling isEnabled one after the other should not cause a re-entrancy issue + const p1 = c1.isEnabled(); + jest.advanceTimersByTime(10); + const p2 = c2.isEnabled(); + await Promise.all([p1, p2]); + expect(await p1).toBe(true); + expect(await p2).toBe(true); + }); }); + test('should return true', async () => { const cookies = new CookieStorage(); expect(await cookies.isEnabled()).toBe(true); }); - test('should return false when document is not available', async () => { - jest.spyOn(GlobalScopeModule, 'getGlobalScope').mockReturnValueOnce({} as typeof globalThis); - const cookies = new CookieStorage(); - expect(await cookies.isEnabled()).toBe(false); + + describe('when document is not available', () => { + let getGlobalScopeSpy: jest.SpyInstance; + beforeEach(() => { + getGlobalScopeSpy = jest.spyOn(GlobalScopeModule, 'getGlobalScope').mockReturnValue({} as typeof globalThis); + }); + afterEach(() => { + getGlobalScopeSpy.mockRestore(); + }); + test('should return false', async () => { + const cookies = new CookieStorage(); + expect(await cookies.isEnabled()).toBe(false); + }); + }); + + describe('when document.cookie throws an error', () => { + let getGlobalScopeSpy: jest.SpyInstance; + beforeEach(() => { + getGlobalScopeSpy = jest.spyOn(GlobalScopeModule, 'getGlobalScope').mockReturnValue({} as typeof globalThis); + getGlobalScopeSpy.mockImplementation(() => { + return { + document: { + cookie: { + get() { + throw new Error('getter error'); + }, + set() { + throw new Error('setter error'); + }, + }, + }, + }; + }); + }); + afterEach(() => { + getGlobalScopeSpy.mockRestore(); + }); + + test('should return false', async () => { + const mockDiagnosticsClient = { + recordEvent: jest.fn(), + increment: jest.fn(), + recordHistogram: jest.fn(), + setTag: jest.fn(), + _flush: jest.fn(), + _setSampleRate: jest.fn(), + }; + const cookies = new CookieStorage(undefined, { diagnosticsClient: mockDiagnosticsClient as any }); + expect(await cookies.isEnabled()).toBe(false); + expect(mockDiagnosticsClient.recordEvent).toHaveBeenCalledTimes(1); + }); + }); + + describe('when document.cookie returns wrong test value', () => { + let getGlobalScopeSpy: jest.SpyInstance; + beforeEach(() => { + getGlobalScopeSpy = jest.spyOn(GlobalScopeModule, 'getGlobalScope').mockReturnValue({} as typeof globalThis); + getGlobalScopeSpy.mockImplementation(() => { + return { + document: { + cookie: { + get() { + return 'wrong value'; + }, + set() { + return 'wrong value'; + }, + }, + }, + }; + }); + }); + afterEach(() => { + getGlobalScopeSpy.mockRestore(); + }); + test('should return false', async () => { + const mockDiagnosticsClient = { + recordEvent: jest.fn(), + increment: jest.fn(), + recordHistogram: jest.fn(), + setTag: jest.fn(), + _flush: jest.fn(), + _setSampleRate: jest.fn(), + }; + const cookies = new CookieStorage(undefined, { diagnosticsClient: mockDiagnosticsClient as any }); + expect(await cookies.isEnabled()).toBe(false); + expect(mockDiagnosticsClient.recordEvent).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/packages/analytics-react-native/test/config.test.ts b/packages/analytics-react-native/test/config.test.ts index c476e4e604..116db8ba50 100644 --- a/packages/analytics-react-native/test/config.test.ts +++ b/packages/analytics-react-native/test/config.test.ts @@ -250,6 +250,7 @@ describe('config', () => { set: async () => undefined, remove: async () => undefined, reset: async () => undefined, + legacyIsEnabled: async () => false, }); const localStorageConstructor = jest.spyOn(LocalStorageModule, 'LocalStorage').mockReturnValueOnce({ isEnabled: async () => false, @@ -315,6 +316,7 @@ describe('config', () => { ...testCookieStorage, options: {}, config: {}, + legacyIsEnabled: async () => false, }); const domain = await Config.getTopLevelDomain(); expect(domain).toBe(''); @@ -343,11 +345,13 @@ describe('config', () => { ...testCookieStorage, options: {}, config: {}, + legacyIsEnabled: async () => false, }) .mockReturnValue({ ...actualCookieStorage, options: {}, config: {}, + legacyIsEnabled: async () => false, }); expect(await Config.getTopLevelDomain('www.legislation.gov.uk')).toBe('.legislation.gov.uk'); }); diff --git a/test-server/autocapture/element-interactions.html b/test-server/autocapture/element-interactions.html index b910425dad..acef18d8c0 100644 --- a/test-server/autocapture/element-interactions.html +++ b/test-server/autocapture/element-interactions.html @@ -284,6 +284,31 @@

Content Changing Button

} ); + amplitude.createInstance('test-instance-4').init( + import.meta.env.VITE_AMPLITUDE_API_KEY, + import.meta.env.VITE_AMPLITUDE_USER_ID || 'amplitude-typescript test user', + ); + amplitude.createInstance('test-instance').init( + import.meta.env.VITE_AMPLITUDE_API_KEY, + import.meta.env.VITE_AMPLITUDE_USER_ID || 'amplitude-typescript test user', + ); + amplitude.createInstance('test-instance-3').init( + import.meta.env.VITE_AMPLITUDE_API_KEY, + import.meta.env.VITE_AMPLITUDE_USER_ID || 'amplitude-typescript test user', + ); + amplitude.createInstance('test-instance-0').init( + import.meta.env.VITE_AMPLITUDE_API_KEY, + import.meta.env.VITE_AMPLITUDE_USER_ID || 'amplitude-typescript test user', + ); + amplitude.createInstance('test-instance-1').init( + import.meta.env.VITE_AMPLITUDE_API_KEY, + import.meta.env.VITE_AMPLITUDE_USER_ID || 'amplitude-typescript test user', + ); + amplitude.createInstance('test-instance-2').init( + import.meta.env.VITE_AMPLITUDE_API_KEY, + import.meta.env.VITE_AMPLITUDE_USER_ID || 'amplitude-typescript test user', + ); + // Add click handler for the first clickable div document.getElementById('dynamic-div').addEventListener('click', () => { setTimeout(() => { diff --git a/test-server/cookies/is-enabled.html b/test-server/cookies/is-enabled.html index 3a7f80fd49..9910d1925a 100644 --- a/test-server/cookies/is-enabled.html +++ b/test-server/cookies/is-enabled.html @@ -16,32 +16,31 @@ } async function run() { - const c1 = new CookieStorage(); - const c2 = new CookieStorage(); - - const promises = [ - c1.isEnabled(), - c2.isEnabled(), - c1.isEnabled(), - c2.isEnabled(), - ]; - const res = await Promise.all(promises); - - const allTrue = res.every((r) => r === true); - log('Parallel calls: ' + JSON.stringify(res)); - log('All true: ' + allTrue); - - const a = await c1.isEnabled(); - const b = await c1.isEnabled(); - const c = await c2.isEnabled(); - log('Sequential c1.isEnabled(): ' + a + ', ' + b); - log('Sequential c2.isEnabled(): ' + c); - log('All true: ' + (a && b && c)); + const promises = []; + + // call isEnabled many times concurrently to stress test the re-entrancy issue + // that was fixed by fixed by https://github.com/amplitude/Amplitude-TypeScript/pull/1539 + const RUNS = 100; + for (let i = 0; i < RUNS; i++) { + const c = new CookieStorage(); + promises.push(c.isEnabled()); + } + + const results = await Promise.all(promises); + + log(`All ${RUNS} calls to CookieStorage.prototype.isEnabled returned true: ${results.every((result) => result === true)}`); + + // Assert that all of the concurrent calls returned true + for (const result of results) { + if (!result) { + const errorMessage = `isEnabled returned 'false'. Test failed.`; + console.error(errorMessage); + throw new Error(errorMessage); + } + } } - run().catch((e) => { - log('Error: ' + e.message); - }); + run();