feat(mv3): Faster Redirects During The First Page Load in main_frame (#…

…1239) * feat(mv3): ⚡ Faster redirects for the first time. Signed-off-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com> * fix: removing only from the tests Signed-off-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com> * test(mv3): Adding removing rule example. Signed-off-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com> * test(mv3): 🧪 Added test regarding removal of rules Signed-off-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com> --------- Signed-off-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
ipfs · Jul 20, 2023 · 9f63fd8 · 9f63fd8
1 parent 4395b2b
commit 9f63fd8
Show file tree

Hide file tree

Showing 2 changed files with 129 additions and 45 deletions.
diff --git a/add-on/src/lib/redirect-handler/blockOrObserve.ts b/add-on/src/lib/redirect-handler/blockOrObserve.ts
@@ -317,6 +317,7 @@ export function addRuleToDynamicRuleSetGenerator (
   getState: () => CompanionState): (input: redirectHandlerInput) => Promise<void> {
   // 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.
     const state = getState()
     const redirectIsOrigin = originUrl === redirectUrl
     const redirectIsLocal = isLocalHost(originUrl) && isLocalHost(redirectUrl)
@@ -327,7 +328,11 @@ export function addRuleToDynamicRuleSetGenerator (
       return
     }
 
-    // We need to construct the regex filter and substitution.
+    // first update the tab 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 })))
+
+    // Then update the rule set for future, we need to construct the regex filter and substitution.
     const { regexSubstitution, regexFilter } = constructRegexFilter({ originUrl, redirectUrl })
 
     const savedRule = savedRegexFilters.get(regexFilter)
@@ -347,10 +352,6 @@ export function addRuleToDynamicRuleSetGenerator (
           removeRuleIds
         }
       )
-
-      // refresh the tab to apply the new rule.
-      const tabs = await browser.tabs.query({ url: `${originUrl}*` })
-      await Promise.all(tabs.map(async tab => await browser.tabs.reload(tab.id)))
     }
 
     setupListeners(async (): Promise<void> => await reconcileRulesAndRemoveOld(getState()))

diff --git a/test/functional/lib/redirect-handler/blockOrObserve.test.ts b/test/functional/lib/redirect-handler/blockOrObserve.test.ts
@@ -1,3 +1,4 @@
+import { reset } from './../../../../node_modules/ansi-colors/types/index.d';
 import {expect} from 'chai'
 import {before, describe, it} from 'mocha'
 import sinon from 'sinon'
@@ -30,9 +31,58 @@ const dynamicRulesConditions = (regexFilter) => ({
   ]
 })
 
+const TEST_TAB_ID = 1234
+const LAST_GROUP_REGEX = '((?:[^\\.]|$).*)$'
+
+/**
+ * Ensures that the tab is redirected to the given url on the first request.
+ *
+ * @param url
+ */
+function ensureTabRedirected (url): void {
+  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([TEST_TAB_ID, { url }])
+}
+
+/**
+ * Ensures that the declarativeNetRequest API is called with the expected rule.
+ * @param expectedCondition
+ * @param regexSubstitution
+ */
+function ensureDeclrativeNetRequetRuleIsAdded ({
+  addRuleLength = 1,
+  callIndex = 0,
+  expectedCondition,
+  regexSubstitution,
+  removedRulesIds = [],
+}: {
+  addRuleLength?: number
+  callIndex?: number
+  expectedCondition: string
+  regexSubstitution: string
+  removedRulesIds?: number[]
+}): void {
+  if (callIndex < 0) {
+    callIndex = browserMock.declarativeNetRequest.updateDynamicRules.getCalls().length + callIndex
+  }
+  expect(browserMock.declarativeNetRequest.updateDynamicRules.called).to.be.true
+  const [{ addRules, removeRuleIds }] = browserMock.declarativeNetRequest.updateDynamicRules.getCall(callIndex).args
+  expect(removeRuleIds).to.deep.equal(removedRulesIds)
+  if (addRuleLength > 0) {
+    expect(addRules).to.have.lengthOf(addRuleLength)
+    const [{ id, priority, action, condition }] = addRules
+    expect(id).to.be.a('number')
+    expect(priority).to.equal(1)
+    expect(action).to.deep.equal({ type: 'redirect', redirect: { regexSubstitution } })
+    expect(condition).to.deep.equal(dynamicRulesConditions(expectedCondition))
+  }
+}
+
 describe('lib/redirect-handler/blockOrObserve', () => {
   before(function () {
     browserMock.runtime.id = 'testid'
+    browserMock.tabs.query.resolves([{id: TEST_TAB_ID}])
   })
 
   describe('isLocalHost', () => {
@@ -68,6 +118,7 @@ describe('lib/redirect-handler/blockOrObserve', () => {
       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.
       await cleanupRules(true)
@@ -83,6 +134,7 @@ describe('lib/redirect-handler/blockOrObserve', () => {
       // when redirectUrl is different from originUrl, but both are localhost.
       await addRuleToDynamicRuleSet({ originUrl: 'http://localhost:9001/foo', redirectUrl: 'http://localhost:9001/bar' })
       expect(browserMock.declarativeNetRequest.updateDynamicRules.called).to.be.false
+      expect (browserMock.tabs.query.called).to.be.false
     })
 
     it('Should allow pages to be recovered', async () => {
@@ -91,73 +143,104 @@ describe('lib/redirect-handler/blockOrObserve', () => {
         originUrl: 'http://localhost:8080',
         redirectUrl: 'chrome-extension://some-path/dist/recover/recovery.html'
       })
-      expect(browserMock.declarativeNetRequest.updateDynamicRules.called).to.be.true
+      ensureTabRedirected('chrome-extension://some-path/dist/recover/recovery.html')
+      ensureDeclrativeNetRequetRuleIsAdded({
+        expectedCondition: `^https?\\:\\/\\/localhost\\:8080${LAST_GROUP_REGEX}`,
+        regexSubstitution: 'chrome-extension://some-path/dist/recover/recovery.html\\1',
+      })
     })
 
     it('Should add redirect rules for local gateway', 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.declarativeNetRequest.updateDynamicRules.called).to.be.true
-      const [{ addRules, removeRuleIds }] = browserMock.declarativeNetRequest.updateDynamicRules.firstCall.args
-      expect(removeRuleIds).to.deep.equal([])
-      expect(addRules).to.have.lengthOf(1)
-      const [{ id, priority, action, condition }] = addRules
-      expect(id).to.be.a('number')
-      expect(priority).to.equal(1)
-      expect(action).to.deep.equal({ type: 'redirect', redirect: { "regexSubstitution": "http://localhost:8080\\1" } })
-      expect(condition).to.deep.equal(dynamicRulesConditions('^https?\\:\\/\\/ipfs\\.io((?:[^\\.]|$).*)$'))
+      ensureTabRedirected('http://localhost:8080/ipns/en.wikipedia-on-ipfs.org')
+      ensureDeclrativeNetRequetRuleIsAdded({
+        expectedCondition: `^https?\\:\\/\\/ipfs\\.io${LAST_GROUP_REGEX}`,
+        regexSubstitution: 'http://localhost:8080\\1'
+      })
     })
 
     it('Should add redirect for local gateway where originUrl is similar to redirectUrl', async () => {
       await addRuleToDynamicRuleSet({
         originUrl: 'https://docs.ipfs.tech',
         redirectUrl: 'http://localhost:8080/ipns/docs.ipfs.tech'
       })
-      expect(browserMock.declarativeNetRequest.updateDynamicRules.called).to.be.true
-      const [{ addRules, removeRuleIds }] = browserMock.declarativeNetRequest.updateDynamicRules.firstCall.args
-      expect(removeRuleIds).to.deep.equal([])
-      expect(addRules).to.have.lengthOf(1)
-      const [{ id, priority, action, condition }] = addRules
-      expect(id).to.be.a('number')
-      expect(priority).to.equal(1)
-      expect(action).to.deep.equal({
-        type: 'redirect', redirect: {
-          "regexSubstitution": "http://localhost:8080/ipns/docs.ipfs.tech\\1"
-        }
-      })
-      expect(condition).to.deep.equal(dynamicRulesConditions('^https?\\:\\/\\/docs\\.ipfs\\.tech((?:[^\\.]|$).*)$'))
+      ensureTabRedirected('http://localhost:8080/ipns/docs.ipfs.tech')
+      ensureDeclrativeNetRequetRuleIsAdded({
+        expectedCondition: `^https?\\:\\/\\/docs\\.ipfs\\.tech${LAST_GROUP_REGEX}`,
+        regexSubstitution: 'http://localhost:8080/ipns/docs.ipfs.tech\\1'
+      })
     })
 
     it('Should add redirect for local gateway where originUrl is similar to redirectUrl and is not https', async () => {
       await addRuleToDynamicRuleSet({
         originUrl: 'http://docs.ipfs.tech',
         redirectUrl: 'http://localhost:8080/ipns/docs.ipfs.tech'
       })
-      expect(browserMock.declarativeNetRequest.updateDynamicRules.called).to.be.true
-      const [{ addRules, removeRuleIds }] = browserMock.declarativeNetRequest.updateDynamicRules.firstCall.args
-      expect(removeRuleIds).to.deep.equal([])
-      expect(addRules).to.have.lengthOf(1)
-      const [{ id, priority, action, condition }] = addRules
-      expect(id).to.be.a('number')
-      expect(priority).to.equal(1)
-      expect(action).to.deep.equal({
-        type: 'redirect', redirect: {
-          "regexSubstitution": "http://localhost:8080/ipns/docs.ipfs.tech\\1"
-        }
-      })
-      expect(condition).to.deep.equal(dynamicRulesConditions('^https?\\:\\/\\/docs\\.ipfs\\.tech((?:[^\\.]|$).*)$'))
+      ensureTabRedirected('http://localhost:8080/ipns/docs.ipfs.tech')
+      ensureDeclrativeNetRequetRuleIsAdded({
+        expectedCondition: `^https?\\:\\/\\/docs\\.ipfs\\.tech${LAST_GROUP_REGEX}`,
+        regexSubstitution: 'http://localhost:8080/ipns/docs.ipfs.tech\\1'
+      })
     })
 
-    it('Should refresh the tab when redirect URL is added', async () => {
-      browserMock.tabs.query.resolves([{id: 1234}])
+    it('Should remove the old rule when redirect changes for local gateway', async () => {
+      await addRuleToDynamicRuleSet({
+        originUrl: 'http://docs.ipfs.tech',
+        redirectUrl: 'http://localhost:8080/ipns/docs.ipfs.tech'
+      })
+      ensureTabRedirected('http://localhost:8080/ipns/docs.ipfs.tech')
+      ensureDeclrativeNetRequetRuleIsAdded({
+        expectedCondition: `^https?\\:\\/\\/docs\\.ipfs\\.tech${LAST_GROUP_REGEX}`,
+        regexSubstitution: 'http://localhost:8080/ipns/docs.ipfs.tech\\1'
+      })
+      const [{ addRules }] = browserMock.declarativeNetRequest.updateDynamicRules.firstCall.args
+
+      await browserMock.declarativeNetRequest.updateDynamicRules.resetHistory()
+      // assuming the localhost changed or the redirectURL changed.
+      await addRuleToDynamicRuleSet({
+        originUrl: 'http://docs.ipfs.tech',
+        redirectUrl: 'http://localhost:8081/ipns/docs.ipfs.tech'
+      })
+      ensureTabRedirected('http://localhost:8081/ipns/docs.ipfs.tech')
+      ensureDeclrativeNetRequetRuleIsAdded({
+        expectedCondition: `^https?\\:\\/\\/docs\\.ipfs\\.tech${LAST_GROUP_REGEX}`,
+        regexSubstitution: 'http://localhost:8081/ipns/docs.ipfs.tech\\1',
+        removedRulesIds: [addRules[0].id]
+      })
+    })
+
+    it('Should remove the old rules when companion is no longer in active state', async () => {
+      // first redirect
+      const getRuleIdsAddedSoFar = () => browserMock.declarativeNetRequest.updateDynamicRules.getCalls().map(({args}) => args[0]?.addRules.map(rule => rule.id).flat()).flat()
+
+      await addRuleToDynamicRuleSet({
+        originUrl: 'http://docs.ipfs.tech',
+        redirectUrl: 'http://localhost:8080/ipns/docs.ipfs.tech'
+      })
+
+      await addRuleToDynamicRuleSet({
+        originUrl: 'http://awesome.ipfs.io',
+        redirectUrl: 'http://localhost:8080/ipns/awesome.ipfs.io'
+      })
+
+      state.active = false
       await addRuleToDynamicRuleSet({
         originUrl: 'https://ipfs.io/ipns/en.wikipedia-on-ipfs.org',
         redirectUrl: 'http://localhost:8080/ipns/en.wikipedia-on-ipfs.org'
       })
-      sinon.assert.calledWith(browserMock.tabs.query, { url: 'https://ipfs.io/ipns/en.wikipedia-on-ipfs.org*' })
-      sinon.assert.calledWith(browserMock.tabs.reload, 1234)
+
+      ensureTabRedirected('http://localhost:8080/ipns/en.wikipedia-on-ipfs.org')
+      ensureDeclrativeNetRequetRuleIsAdded({
+        addRuleLength: 0,
+        callIndex: -1,
+        expectedCondition: `^https?\\:\\/\\/ipfs\\.io${LAST_GROUP_REGEX}`,
+        regexSubstitution: 'http://localhost:8080\\1',
+        removedRulesIds: getRuleIdsAddedSoFar()
+      })
+
     })
   })
 })