Prevent null key in historySites state

This was causing an error to be thrown for every subsequent action, and no more state updates to be sent to windows, resulting in multiple issues.
It was also causing an error dump to be generated upon every action, thus halting the browser at those times, including any audio/video (and creating stutters).

Fix #13079 (main test case with STR)
Fix #12333
Fix #12828
Fix #13157
Address #13087 but this is a broad issue - this change will fix a hard issue, whereas that refers to general slowness.

app/common/lib/historyUtil.js

@@ -160,8 +160,8 @@ const mergeSiteDetails = (oldDetail, newDetail) => {
 }
 
 const getDetailFromFrame = (frame) => {
-  if (frame == null) {
-    return Immutable.Map()
+  if (frame == null || !frame.has('location')) {
+    return null
   }
 
   return makeImmutable({

app/common/state/historyState.js

@@ -8,6 +8,7 @@ const {STATE_SITES} = require('../../../js/constants/stateConstants')
 const historyUtil = require('../lib/historyUtil')
 const urlUtil = require('../../../js/lib/urlutil')
 const {makeImmutable, isMap} = require('./immutableUtil')
+const shouldLogWarnings = process.env.NODE_ENV !== 'production'
 
 const validateState = function (state) {
   state = makeImmutable(state)
@@ -33,8 +34,20 @@ const historyState = {
   },
 
   addSite: (state, siteDetail) => {
+    if (!siteDetail) {
+      if (shouldLogWarnings) {
+        console.error('historyState:addSite siteDetail was null')
+      }
+      return state
+    }
     let sites = historyState.getSites(state)
     let siteKey = historyUtil.getKey(siteDetail)
+    if (!siteKey) {
+      if (shouldLogWarnings) {
+        console.log('historyState:addSite siteKey was null for siteDetail:', (siteDetail && siteDetail.toJS) ? siteDetail.toJS() : siteDetail)
+      }
+      return state
+    }
     siteDetail = makeImmutable(siteDetail)
 
     const oldSite = sites.get(siteKey)

app/renderer/components/frame/frame.js

@@ -620,8 +620,17 @@ class Frame extends React.Component {
         // calling with setTimeout is an ugly hack for a race condition
         // with setTitle. We either need to delay this call until the title is
         // or add a way to update it
+        // However, it's possible that the frame could be destroyed, or in a bad
+        // way by then, so make sure we do a null check.
         setTimeout(() => {
-          appActions.addHistorySite(historyUtil.getDetailFromFrame(this.frame))
+          const siteDetail = historyUtil.getDetailFromFrame(this.frame)
+          if (siteDetail) {
+            appActions.addHistorySite(siteDetail)
+          } else if (process.env.NODE_ENV !== 'production') {
+            // log, in case we decide we want these entries to go in to the history
+            // but do not send a null entry to history as it will be rejected
+            console.error('frame: siteDetail was null when calling addHistorySite')
+          }
         }, 250)
       }
 

test/unit/app/common/lib/historyUtilTest.js

@@ -297,7 +297,13 @@ describe('historyUtil unit tests', function () {
   describe('getDetailFromFrame', function () {
     it('null case', function () {
       const result = historyUtil.getDetailFromFrame()
-      assert.deepEqual(result, Immutable.Map())
+      assert.deepEqual(result, null)
+    })
+
+    it('no location case', function () {
+      const badFrame = Immutable.Map().set('partitionNumber', 0)
+      const result = historyUtil.getDetailFromFrame(badFrame)
+      assert.deepEqual(result, null)
     })
 
     it('returns details', function () {