fix(brave): not updating tab to the new redirect url in some cases. (#…

…1285) * fix(brave): 🐛 Wait for the tab to exist before redirecting it. Signed-off-by: Nishant Arora <[email protected]> * feat(brave): 🧪 Add reload test Signed-off-by: Nishant Arora <[email protected]> * fix(test): 🔧 Ticking clock based on shared common value. Signed-off-by: Nishant Arora <[email protected]> --------- Signed-off-by: Nishant Arora <[email protected]>
ipfs · Sep 21, 2023 · 4097e2d · 4097e2d
1 parent dcc10f0
commit 4097e2d
Show file tree

Hide file tree

Showing 2 changed files with 56 additions and 5 deletions.
diff --git a/add-on/src/lib/redirect-handler/blockOrObserve.ts b/add-on/src/lib/redirect-handler/blockOrObserve.ts
@@ -13,9 +13,10 @@ const log = debug('ipfs-companion:redirect-handler:blockOrObserve')
 log.error = debug('ipfs-companion:redirect-handler:blockOrObserve:error')
 
 export const DEFAULT_NAMESPACES = new Set(['ipfs', 'ipns'])
-export const GLOBAL_STATE_OPTION_CHANGE = 'GLOBAL_STATE_OPTION_CHANGE'
 export const DELETE_RULE_REQUEST = 'DELETE_RULE_REQUEST'
 export const DELETE_RULE_REQUEST_SUCCESS = 'DELETE_RULE_REQUEST_SUCCESS'
+export const GLOBAL_STATE_OPTION_CHANGE = 'GLOBAL_STATE_OPTION_CHANGE'
+export const MAX_RETRIES_TO_UPDATE_TAB = 5
 
 // We need to match the rest of the URL, so we can use a wildcard.
 export const RULE_REGEX_ENDING = '((?:[^\\.]|$).*)$'
@@ -25,9 +26,12 @@ interface regexFilterMap {
   regexSubstitution: string
 }
 
-interface redirectHandlerInput {
+interface redirectPair {
   originUrl: string
   redirectUrl: string
+}
+
+interface redirectHandlerInput extends redirectPair {
   getPort: (state: CompanionState) => string
 }
 
@@ -362,6 +366,29 @@ export function addRuleToDynamicRuleSetGenerator (
     }
   })
 
+  /**
+   * Update the tab that matches the origin URL to the redirect URL.
+   *
+   * @param param0 redirectPair
+   * @param numTry number of times we have tried to update the tab.
+   * @returns
+   */
+  async function updateTabToNewUrl ({ originUrl, redirectUrl }: redirectPair, numTry: number = 1): Promise<void> {
+    // Don't get stuck in a loop.
+    if (numTry > MAX_RETRIES_TO_UPDATE_TAB) {
+      return
+    }
+
+    const tabs = await browser.tabs.query({ url: `${originUrl}*` })
+    if (tabs.length === 0) {
+      // wait for the tab to be created or a redirect to happen. Check every 100ms.
+      await new Promise(resolve => setTimeout(resolve, 100))
+      await updateTabToNewUrl({ originUrl, redirectUrl }, numTry + 1)
+    } else {
+      await Promise.all(tabs.map(async tab => await browser.tabs.update(tab.id, { url: redirectUrl })))
+    }
+  }
+
   // returning a closure to avoid passing `getState` as an argument to `addRuleToDynamicRuleSet`.
   return async function ({ originUrl, redirectUrl }: redirectHandlerInput): Promise<void> {
     // update the rules so that the next request is handled correctly.
@@ -376,8 +403,8 @@ export function addRuleToDynamicRuleSetGenerator (
     }
 
     // first update all the matching tabs to apply the new rule.
-    const tabs = await browser.tabs.query({ url: `${originUrl}*` })
-    await Promise.all(tabs.map(async tab => await browser.tabs.update(tab.id, { url: redirectUrl })))
+    // don't await this as this can happen in parallel.
+    void updateTabToNewUrl({ originUrl, redirectUrl })
 
     // Then update the rule set for future, we need to construct the regex filter and substitution.
     const { regexSubstitution, regexFilter } = constructRegexFilter({ originUrl, redirectUrl })

diff --git a/test/functional/lib/redirect-handler/blockOrObserve.test.ts b/test/functional/lib/redirect-handler/blockOrObserve.test.ts
@@ -2,6 +2,7 @@ import { expect } from 'chai';
 import { before, describe, it } from 'mocha';
 import sinon from 'sinon';
 import browserMock from 'sinon-chrome';
+import { MAX_RETRIES_TO_UPDATE_TAB } from './../../../../add-on/src/lib/redirect-handler/blockOrObserve';
 
 import { optionDefaults } from '../../../../add-on/src/lib/options.js';
 import { RULE_REGEX_ENDING, addRuleToDynamicRuleSetGenerator, cleanupRules, isLocalHost } from '../../../../add-on/src/lib/redirect-handler/blockOrObserve';
@@ -116,7 +117,6 @@ describe('lib/redirect-handler/blockOrObserve', () => {
     beforeEach(async () => {
       sinonSandbox.restore()
       browserMock.tabs.query.resetHistory()
-      browserMock.tabs.reload.resetHistory()
       browserMock.tabs.update.resetHistory()
       browserMock.declarativeNetRequest = sinonSandbox.spy(new DeclarativeNetRequestMock())
       // this cleans up the rules from the previous test stored in memory.
@@ -184,6 +184,30 @@ describe('lib/redirect-handler/blockOrObserve', () => {
       })
     })
 
+    it('Should update tab from originUrl to redirectUrl for the first time', async () => {
+      await addRuleToDynamicRuleSet({
+        originUrl: 'https://ipfs.io/ipns/en.wikipedia-on-ipfs.org',
+        redirectUrl: 'http://localhost:8080/ipns/en.wikipedia-on-ipfs.org'
+      })
+      expect(browserMock.tabs.query.called).to.be.true
+      expect(browserMock.tabs.update.called).to.be.true
+      expect(browserMock.tabs.update.lastCall.args).to.deep.equal([40, { url: 'http://localhost:8080/ipns/en.wikipedia-on-ipfs.org' }])
+    })
+
+    it('Should wait for the tab to exist before updating tab from originUrl to redirectUrl for the first time', async () => {
+      const clock = sinon.useFakeTimers();
+      browserMock.tabs.query.onCall(0).resolves([])
+      browserMock.tabs.query.onCall(1).resolves([{ id: 40 }])
+      await addRuleToDynamicRuleSet({
+        originUrl: 'https://ipfs.io/ipns/en.wikipedia-on-ipfs.org',
+        redirectUrl: 'http://localhost:8080/ipns/en.wikipedia-on-ipfs.org'
+      })
+      await clock.tickAsync(100 * MAX_RETRIES_TO_UPDATE_TAB)
+      expect(browserMock.tabs.query.callCount).to.be.equal(2)
+      expect(browserMock.tabs.update.called).to.be.true
+      expect(browserMock.tabs.update.lastCall.args).to.deep.equal([40, { url: 'http://localhost:8080/ipns/en.wikipedia-on-ipfs.org' }])
+    })
+
     it('Should add redirect rules for local gateway', async () => {
       await addRuleToDynamicRuleSet({
         originUrl: 'https://ipfs.io/ipns/en.wikipedia-on-ipfs.org',