Update the uninstall URL each time the ATB setting is updated (#2156)

When a user uninstalls the extension, an survey is displayed which
aims to find out what the user's reason was. Some parameters are
included with that request (e.g. the browser name and version) to
help us diagnose issues. The ATB setting is included as well, so it's
important that we update the uninstall URL each time the ATB setting
is changed. So far, changes to the ATB setting were only reflected
after the next time the updateUninstallURL event fired, which led to a
delay of up to 10 minutes.

shared/js/background/atb.js

@@ -47,7 +47,6 @@ const ATB = (() => {
             if (!atbSetting) {
                 atbSetting = ATB_ERROR_COHORT
                 settings.updateSetting('atb', ATB_ERROR_COHORT)
-                ATB.setOrUpdateATBdnrRule(ATB_ERROR_COHORT)
                 errorParam = '&e=1'
             }
 
@@ -63,10 +62,8 @@ const ATB = (() => {
 
                 if (res.data.updateVersion) {
                     settings.updateSetting('atb', res.data.updateVersion)
-                    ATB.setOrUpdateATBdnrRule(res.data.updateVersion)
                 } else if (atbSetting === ATB_ERROR_COHORT) {
                     settings.updateSetting('atb', res.data.version)
-                    ATB.setOrUpdateATBdnrRule(res.data.version)
                 }
             })
         },
@@ -105,7 +102,6 @@ const ATB = (() => {
             const url = ddgAtbURL + randomValue + '&browser=' + parseUserAgentString().browser
             return load.JSONfromExternalFile(url).then((res) => {
                 settings.updateSetting('atb', res.data.version)
-                ATB.setOrUpdateATBdnrRule(res.data.version)
             }, () => {
                 console.log('couldn\'t reach atb.js for initial server call, trying again')
                 numTries += 1
@@ -189,7 +185,6 @@ const ATB = (() => {
 
                     if (atb) {
                         settings.updateSetting('atb', atb)
-                        ATB.setOrUpdateATBdnrRule(atb)
                     }
 
                     ATB.finalizeATB(params)
@@ -280,9 +275,23 @@ const ATB = (() => {
     }
 })()
 
-settings.ready().then(async () => {
+settings.ready().then(() => {
+    const updateUninstallURL =
+        async () => { browserWrapper.setUninstallURL(await ATB.getSurveyURL()) }
+
     // set initial uninstall url
-    browserWrapper.setUninstallURL(await ATB.getSurveyURL())
+    updateUninstallURL()
+
+    // Ensure the uninstall URL and the ATB declarativeNetRequest rules are also
+    // kept up to date as the ATB values are updated.
+    settings.onSettingUpdate.addEventListener(
+        'atb', event => {
+            const atb = event instanceof CustomEvent ? event.detail : undefined
+            updateUninstallURL()
+            ATB.setOrUpdateATBdnrRule(atb)
+        }
+    )
+    settings.onSettingUpdate.addEventListener('set_atb', updateUninstallURL)
 })
 
 export default ATB

shared/js/background/events.js

@@ -519,8 +519,6 @@ browserWrapper.createAlarm('updateHTTPSLists', {
 browserWrapper.createAlarm('updateLists', {
     periodInMinutes: tdsStorage.updatePeriodInMinutes
 })
-// update uninstall URL every 10 minutes
-browserWrapper.createAlarm('updateUninstallURL', { periodInMinutes: 10 })
 // remove expired HTTPS service entries
 browserWrapper.createAlarm('clearExpiredHTTPSServiceCache', { periodInMinutes: 60 })
 // Rotate the user agent spoofed
@@ -538,8 +536,6 @@ browser.alarms.onAlarm.addListener(async alarmEvent => {
         } catch (e) {
             console.log(e)
         }
-    } else if (alarmEvent.name === 'updateUninstallURL') {
-        browser.runtime.setUninstallURL(await ATB.getSurveyURL())
     } else if (alarmEvent.name === 'updateLists') {
         await settings.ready()
 

shared/js/background/settings.js

@@ -1,6 +1,11 @@
 const defaultSettings = require('../../data/defaultSettings')
 const browserWrapper = require('./wrapper')
 
+const onSettingUpdate = new EventTarget()
+
+// @ts-expect-error Workaround for the tests in unit-tests/node/reference-tests.
+if (typeof CustomEvent === 'undefined') globalThis.CustomEvent = Event
+
 /**
  * Settings whose defaults can by managed by the system administrator
  */
@@ -97,7 +102,7 @@ function buildSettingsFromDefaults () {
 }
 
 function syncSettingTolocalStorage () {
-    browserWrapper.syncToStorage({ settings })
+    return browserWrapper.syncToStorage({ settings })
 }
 
 function getSetting (name) {
@@ -123,7 +128,9 @@ function updateSetting (name, value) {
     }
 
     settings[name] = value
-    syncSettingTolocalStorage()
+    syncSettingTolocalStorage().then(() => {
+        onSettingUpdate.dispatchEvent(new CustomEvent(name, { detail: value }))
+    })
 }
 
 function removeSetting (name) {
@@ -148,5 +155,6 @@ module.exports = {
     updateSetting,
     removeSetting,
     logSettings,
-    ready
+    ready,
+    onSettingUpdate
 }

shared/js/background/storage/tds.js

@@ -30,6 +30,7 @@ class TDSStorage {
 
         this.isInstalling = false
 
+        // TODO: Use an EventTarget instead.
         this._onUpdatedListeners = new Map()
 
         this._onReadyResolvers = new Map()

shared/js/background/wrapper.js

@@ -30,7 +30,7 @@ export function getManifestVersion () {
 }
 
 export function syncToStorage (data) {
-    browser.storage.local.set(data)
+    return browser.storage.local.set(data)
 }
 
 // @ts-ignore

unit-test/background/atb.js

@@ -287,3 +287,63 @@ describe('complex install workflow cases', () => {
             })
     })
 })
+
+describe('atb.getUninstallURL()', () => {
+    it('should update the uninstall URL correctly as the ATB values are updated', async () => {
+        settingHelper.stub({ atb: null })
+
+        let uninstallUrl = null
+        const setUninstallURLSpy = spyOn(browser.runtime, 'setUninstallURL').and.callFake(url => {
+            uninstallUrl = url
+        })
+
+        const checkUninstallUrl = (expectedAtb, expectedSetAtb) => {
+            const { origin, pathname, searchParams } = new URL(uninstallUrl)
+
+            expect(origin).toEqual('https://duckduckgo.com')
+            expect(pathname).toEqual('/atb.js')
+            expect(searchParams.get('uninstall')).toEqual('1')
+            expect(searchParams.get('action')).toEqual('survey')
+            expect(searchParams.get('atb')).toEqual(expectedAtb)
+            expect(searchParams.get('set_atb')).toEqual(expectedSetAtb)
+            expect(searchParams.get('browser')).toEqual('Chrome')
+            expect(searchParams.get('bv')).toEqual('108')
+            expect(searchParams.get('v')).toEqual('1234.56')
+        }
+
+        // Initial state.
+        expect(settings.getSetting('atb')).toEqual(null)
+        expect(setUninstallURLSpy).not.toHaveBeenCalled()
+        expect(uninstallUrl).toEqual(null)
+
+        // Before the atb is set.
+        uninstallUrl = await atb.getSurveyURL()
+        checkUninstallUrl(null, null)
+
+        // After the atb is set.
+        stubLoadJSON({ returnedAtb: 'v119-8' })
+        await atb.setInitialVersions()
+        await new Promise(resolve => setTimeout(resolve, 0))
+        expect(settings.getSetting('atb')).toEqual('v119-8')
+        expect(setUninstallURLSpy).toHaveBeenCalledTimes(1)
+        checkUninstallUrl('v119-8', null)
+
+        // After set_atb is set.
+        await atb.updateSetAtb()
+        await new Promise(resolve => setTimeout(resolve, 0))
+        expect(setUninstallURLSpy).toHaveBeenCalledTimes(2)
+        checkUninstallUrl('v119-8', 'v119-8')
+
+        // After atb is updated.
+        settings.updateSetting('atb', 'v119-8a')
+        await new Promise(resolve => setTimeout(resolve, 0))
+        expect(setUninstallURLSpy).toHaveBeenCalledTimes(3)
+        checkUninstallUrl('v119-8a', 'v119-8')
+
+        // After set_atb is updated.
+        settings.updateSetting('set_atb', 'v119-8b')
+        await new Promise(resolve => setTimeout(resolve, 0))
+        expect(setUninstallURLSpy).toHaveBeenCalledTimes(4)
+        checkUninstallUrl('v119-8a', 'v119-8b')
+    })
+})

unit-test/helpers/settings.js

@@ -13,7 +13,12 @@ const stub = (startingVals) => {
         .and.callFake(key => settingObj[key])
 
     const update = spyOn(origSettings, 'updateSetting')
-        .and.callFake((key, val) => { settingObj[key] = val })
+        .and.callFake((key, val) => {
+            settingObj[key] = val
+            origSettings.onSettingUpdate.dispatchEvent(
+                new CustomEvent(key, { detail: val })
+            )
+        })
 
     const remove = spyOn(origSettings, 'removeSetting')
         .and.callFake(key => { delete settingObj[key] })