From 95394b9489faf9635bbfab6f3edfb4b363f4c23a Mon Sep 17 00:00:00 2001
From: Pete Miller <miller.pete@gmail.com>
Date: Wed, 21 Feb 2018 12:41:10 -0800
Subject: [PATCH] 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               |  4 ++--
 app/common/state/historyState.js            | 13 +++++++++++++
 app/renderer/components/frame/frame.js      | 11 ++++++++++-
 test/unit/app/common/lib/historyUtilTest.js |  8 +++++++-
 4 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/app/common/lib/historyUtil.js b/app/common/lib/historyUtil.js
index 995cf35250d..dfcf5597a7b 100644
--- a/app/common/lib/historyUtil.js
+++ b/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({
diff --git a/app/common/state/historyState.js b/app/common/state/historyState.js
index 4dd8cf41401..130d378f3fc 100644
--- a/app/common/state/historyState.js
+++ b/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)
diff --git a/app/renderer/components/frame/frame.js b/app/renderer/components/frame/frame.js
index 603a717171f..b83170c5f2f 100644
--- a/app/renderer/components/frame/frame.js
+++ b/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)
       }
 
diff --git a/test/unit/app/common/lib/historyUtilTest.js b/test/unit/app/common/lib/historyUtilTest.js
index 646b9c90d24..6e550a6a39e 100644
--- a/test/unit/app/common/lib/historyUtilTest.js
+++ b/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 () {