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 () {