Merge pull request #13240 from brave/fix/log-state-null-key-paths

Handle failure in creating a state diff by logging (and crash-reporting) the full path of any keys that are null

app/common/state/immutableUtil.js

@@ -45,6 +45,22 @@ const api = {
       return defaultValue
     }
     return api.isImmutable(obj) ? obj.toJS() : obj
+  },
+
+  findNullKeyPaths (state, pathToState = []) {
+    let nullKeys = [ ]
+    if (!Immutable.Map.isMap(state) && !Immutable.List.isList(state)) {
+      return nullKeys
+    }
+    for (const key of state.keySeq()) {
+      const keyPath = [...pathToState, key]
+      if (key === null) {
+        nullKeys.push(keyPath)
+      }
+      // recursive, to find deep keys
+      nullKeys.push(...api.findNullKeyPaths(state.get(key), keyPath))
+    }
+    return nullKeys
   }
 }
 

js/stores/appStore.js

@@ -36,7 +36,7 @@ const urlUtil = require('../lib/urlutil')
 const buildConfig = require('../constants/buildConfig')
 
 // state helpers
-const {makeImmutable} = require('../../app/common/state/immutableUtil')
+const {makeImmutable, findNullKeyPaths} = require('../../app/common/state/immutableUtil')
 const basicAuthState = require('../../app/common/state/basicAuthState')
 const extensionState = require('../../app/common/state/extensionState')
 const aboutNewTabState = require('../../app/common/state/aboutNewTabState')
@@ -80,8 +80,27 @@ class AppStore extends EventEmitter {
 
   emitChanges () {
     if (this.lastEmittedState && this.lastEmittedState !== appState) {
-      const d = diff(this.lastEmittedState, appState)
-      if (!d.isEmpty()) {
+      let d
+      try {
+        d = diff(this.lastEmittedState, appState)
+      } catch (e) {
+        console.error('Error getting a diff from latest state.')
+        // one possible reason immutablediff can throw an error
+        // is due to null keys, so let's log any that we find
+        const nullKeyPaths = findNullKeyPaths(appState)
+        const error = (typeof e === 'object')
+          ? e
+          : (typeof e === 'string')
+            ? new Error(e)
+            : new Error()
+        for (let keyPath of nullKeyPaths) {
+          keyPath = keyPath.map(key => key === null ? 'null' : key)
+          const message = ` State path had null entry! Path was: [${keyPath.join(', ')}].`
+          error.message += message
+        }
+        throw error
+      }
+      if (d && !d.isEmpty()) {
         BrowserWindow.getAllWindows().forEach((wnd) => {
           if (wnd.webContents && !wnd.webContents.isDestroyed()) {
             wnd.webContents.send(messages.APP_STATE_CHANGE, { stateDiff: d.toJS() })

test/unit/app/common/state/immutableUtilTest.js

@@ -148,4 +148,32 @@ describe('immutableUtil unit test', function () {
       assert.deepEqual(immutableUtil.deleteImmutablePaths(data, [['a', 'a1'], 'c']).toJS(), {a: {a2: 8}, d: 5})
     })
   })
+  describe('findNullKeyPaths', function () {
+    it('finds maps which have a null key', function () {
+      const data = Immutable.fromJS({
+        normal: {
+          key1: 'value'
+        },
+        bad: { },
+        anotherBad: {
+          deeper: { }
+        },
+        null: {
+          badParent: 'value4'
+        },
+        nullValue: null
+      })
+      .setIn(['bad', null], 'value2')
+      .setIn(['anotherBad', 'deeper', null], 'value3')
+      .set(null, Immutable.fromJS({badParent: 'value4'}))
+
+      const expectedNullPaths = [
+        ['bad', null],
+        ['anotherBad', 'deeper', null],
+        [null]
+      ]
+      const actualNullPaths = immutableUtil.findNullKeyPaths(data)
+      assert.deepEqual(actualNullPaths, expectedNullPaths)
+    })
+  })
 })