siteCache Enhancement

fix #11161 1. Support clearing method triggered by clearing browserHistory 2. Fix invalid favicon only entry generation 3. Migration of invalid favicon entry and trailing slash siteCache also fix start-brk user-data-dir Auditors: @bsclifton, @ayumi Test Plan: Covered by unit test
brave · Sep 27, 2017 · 5753efb · 5753efb
1 parent c9ed9e7
commit 5753efb
Show file tree

Hide file tree

Showing 9 changed files with 80 additions and 3 deletions.
diff --git a/app/browser/reducers/sitesReducer.js b/app/browser/reducers/sitesReducer.js
@@ -49,6 +49,7 @@ const sitesReducer = (state, action, immutableAction) => {
         const clearData = defaults ? defaults.merge(temp) : temp
         if (clearData.get('browserHistory')) {
           state = state.set('sites', siteUtil.clearHistory(state.get('sites')))
+          state = siteCache.clearLocationSiteKeysCache(state)
           filtering.clearHistory()
         }
         break

diff --git a/app/common/state/siteCache.js b/app/common/state/siteCache.js
@@ -97,3 +97,8 @@ const removeLocationSiteKey = (state, location, siteKey) => {
   }
 }
 module.exports.removeLocationSiteKey = removeLocationSiteKey
+
+const clearLocationSiteKeysCache = (state) => {
+  return state.set('locationSiteKeysCache', new Immutable.Map())
+}
+module.exports.clearLocationSiteKeysCache = clearLocationSiteKeysCache
diff --git a/app/sessionStore.js b/app/sessionStore.js
@@ -33,6 +33,7 @@ const Channel = require('./channel')
 const {isList, isMap, isImmutable, makeImmutable, deleteImmutablePaths} = require('./common/state/immutableUtil')
 const tabState = require('./common/state/tabState')
 const windowState = require('./common/state/windowState')
+const siteCache = require('./common/state/siteCache')
 
 const platformUtil = require('./common/lib/platformUtil')
 const getSetting = require('../js/settings').getSetting
@@ -559,6 +560,11 @@ module.exports.runPostMigrations = (immutableData) => {
         const site = oldSites.get(key)
         const newKey = siteUtil.getSiteKey(site)
         if (!newKey) {
+          immutableData = immutableData.deleteIn(['sites', key])
+          const location = siteUtil.getLocationFromSiteKey(key)
+          if (location) {
+            immutableData = siteCache.removeLocationSiteKey(immutableData, location, key)
+          }
           continue
         }
         immutableData = immutableData.setIn(['sites', newKey], site)

diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js
@@ -657,7 +657,9 @@ module.exports.updateSiteFavicon = function (state, location, favicon) {
     return state
   }
   siteKeys.forEach((siteKey) => {
-    state = state.setIn(['sites', siteKey, 'favicon'], favicon)
+    if (state.getIn(['sites', siteKey])) {
+      state = state.setIn(['sites', siteKey, 'favicon'], favicon)
+    }
   })
   return state
 }

diff --git a/package.json b/package.json
@@ -29,7 +29,7 @@
     "start-log": "node ./tools/start.js --user-data-dir=brave-development --enable-logging=stderr --v=1 --enable-extension-activity-logging --enable-sandbox-logging --enable-dcheck",
     "start": "node ./tools/start.js --user-data-dir=brave-development --enable-logging --v=0 --enable-extension-activity-logging --enable-sandbox-logging --enable-dcheck",
     "start2": "node ./tools/start.js --user-data-dir=brave-development-2 --enable-logging --v=0 --enable-extension-activity-logging --enable-sandbox-logging --enable-dcheck",
-    "start-brk": "node ./tools/start.js --debug-brk=5858 -enable-logging --v=0 --enable-dcheck",
+    "start-brk": "node ./tools/start.js --debug-brk=5858 -enable-logging --v=0 --enable-dcheck --user-data-dir=brave-development",
     "test": "cross-env NODE_ENV=test mocha \"test/**/*Test.js\"",
     "testsuite": "node ./tools/test.js",
     "unittest": "cross-env NODE_ENV=test mocha \"test/unit/**/*Test.js\"",

diff --git a/test/unit/app/browser/reducers/sitesReducerTest.js b/test/unit/app/browser/reducers/sitesReducerTest.js
@@ -25,12 +25,16 @@ describe('sitesReducerTest', function () {
     this.fakeFiltering = {
       clearHistory: () => {}
     }
+    this.fakeSiteCache = {
+      clearLocationSiteKeysCache: () => {}
+    }
     mockery.enable({
       warnOnReplace: false,
       warnOnUnregistered: false,
       useCleanCache: true
     })
     mockery.registerMock('../../filtering', this.fakeFiltering)
+    mockery.registerMock('../../common/state/siteCache', this.fakeSiteCache)
     sitesReducer = require('../../../../../app/browser/reducers/sitesReducer')
   })
 
@@ -44,16 +48,22 @@ describe('sitesReducerTest', function () {
       }
       const newState = initState.setIn(['clearBrowsingDataDefaults', 'browserHistory'], true)
       this.clearHistory = sinon.stub(this.fakeFiltering, 'clearHistory')
+      this.clearLocationSiteKeysCache = sinon.stub(this.fakeSiteCache, 'clearLocationSiteKeysCache')
       this.state = sitesReducer(newState, this.action, makeImmutable(this.action))
     })
 
     after(function () {
+      this.clearLocationSiteKeysCache.restore()
       this.clearHistory.restore()
     })
 
     it('calls `filtering.clearHistory`', function () {
       assert.ok(this.clearHistory.calledOnce)
     })
+
+    it('calls `siteCache.clearLocationSiteKeysCache`', function () {
+      assert.ok(this.clearLocationSiteKeysCache.calledOnce)
+    })
   })
 
   describe('APP_ADD_SITE', function () {

diff --git a/test/unit/app/common/state/siteCacheTest.js b/test/unit/app/common/state/siteCacheTest.js
@@ -140,4 +140,13 @@ describe('siteCache', function () {
       assert.deepEqual(cachedKeys, undefined)
     })
   })
+
+  describe('clearLocationSiteKeysCache', function () {
+    it('clear all cached siteKeys', function () {
+      let state = siteCache.loadLocationSiteKeysCache(baseState)
+      const expectedCache = {}
+      state = siteCache.clearLocationSiteKeysCache(state)
+      assert.deepEqual(state.get('locationSiteKeysCache').toJS(), expectedCache)
+    })
+  })
 })
diff --git a/test/unit/app/sessionStoreTest.js b/test/unit/app/sessionStoreTest.js
@@ -76,6 +76,9 @@ describe('sessionStore unit tests', function () {
     },
     getSiteKey: (siteDetail) => {
       return siteUtil.getSiteKey(siteDetail)
+    },
+    getLocationFromSiteKey: (siteKey) => {
+      return siteUtil.getLocationFromSiteKey(siteKey)
     }
   }
   const fakeLocale = {
@@ -1096,8 +1099,38 @@ describe('sessionStore unit tests', function () {
             }
           }
         })
+        const expectedResult = {
+          sites: {}
+        }
         const result = sessionStore.runPostMigrations(data)
-        assert.deepEqual(result.toJS(), data.toJS())
+        assert.deepEqual(result.toJS(), expectedResult)
+      })
+    })
+    describe('locationSiteKeysCache trailing slash migration', function () {
+      it('triggered by invalid site entry', function () {
+        const data = Immutable.fromJS({
+          sites: {
+            'https://brave.com/|0|0': {
+              favicon: 'https://brave.com/bat.ico'
+            }
+          },
+          locationSiteKeysCache: {
+            'https://brave.com/': [
+              'https://brave.com/|0|0',
+              'https://brave.com|0|0'
+            ]
+          }
+        })
+        const expectedResult = {
+          sites: {},
+          locationSiteKeysCache: {
+            'https://brave.com/': [
+              'https://brave.com|0|0'
+            ]
+          }
+        }
+        const result = sessionStore.runPostMigrations(data)
+        assert.deepEqual(result.toJS(), expectedResult)
       })
     })
   })

diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js
@@ -1119,6 +1119,17 @@ describe('siteUtil', function () {
       const expectedState = siteUtil.addSite(stateWithInvalidEntries, updatedSiteDetail, siteTags.BOOKMARK)
       assert.deepEqual(processedState.get('sites').toJS(), expectedState.get('sites').toJS())
     })
+    it('returns the object unchanged if the entry does not exist but found in cache', function () {
+      const stateWithNoEntries = Immutable.fromJS({
+        sites: {},
+        locationSiteKeysCache: {
+          'https://brave.com': [testUrl1 + '/|0|0', testUrl1 + '|0|0']
+        }
+      })
+      const processedState = siteUtil.updateSiteFavicon(stateWithNoEntries, testUrl1, 'https://brave.com/favicon.ico')
+      const expectedState = stateWithNoEntries
+      assert.deepEqual(processedState.get('sites').toJS(), expectedState.get('sites').toJS())
+    })
   })
 
   describe('getDetailFromFrame', function () {