Skip to content

Commit

Permalink
Avoid race condtion that could prevent contextual account switching (#…
Browse files Browse the repository at this point in the history
…7623)

There was a race condition in the logic responsible for switching the
selected account based upon the active tab. It was asynchronously
querying the active tab, then assuming it had been retrieved later.

The active tab info itself was already in the redux store in another
spot, one that is guaranteed to be set before the UI renders. The
race condition was avoided by deleting the duplicate state, and using
the other active tab state.
  • Loading branch information
Gudahtt authored Dec 3, 2019
1 parent 39a9664 commit c5b85aa
Show file tree
Hide file tree
Showing 5 changed files with 17 additions and 35 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ import { getOriginFromUrl } from '../../../helpers/utils/util'

const mapStateToProps = state => {
const addressConnectedToCurrentTab = getAddressConnectedToCurrentTab(state)
const { openMetaMaskTabs, currentActiveTab = {} } = state.appState
const { title, url, id } = currentActiveTab
const { openMetaMaskTabs } = state.appState
const { title, url, id } = state.activeTab

let tabToConnect

Expand Down
6 changes: 0 additions & 6 deletions ui/app/ducks/app/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ export default function reduceApp (state, action) {
requestAccountTabs: {},
openMetaMaskTabs: {},
currentWindowTab: {},
currentActiveTab: {},
}, state.appState)

switch (action.type) {
Expand Down Expand Up @@ -782,11 +781,6 @@ export default function reduceApp (state, action) {
currentWindowTab: action.value,
})

case actions.SET_ACTIVE_TAB:
return extend(appState, {
currentActiveTab: action.value,
})

default:
return appState
}
Expand Down
17 changes: 13 additions & 4 deletions ui/app/pages/routes/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ import {

class Routes extends Component {
componentWillMount () {
const { currentCurrency, setCurrentCurrencyToUSD, getActiveTab } = this.props
const { currentCurrency, setCurrentCurrencyToUSD } = this.props

if (!currentCurrency) {
setCurrentCurrencyToUSD()
Expand All @@ -105,8 +105,13 @@ class Routes extends Component {
})
}
})
}

getActiveTab()
componentDidMount () {
const { addressConnectedToCurrentTab, showAccountDetail, selectedAddress } = this.props
if (addressConnectedToCurrentTab && addressConnectedToCurrentTab !== selectedAddress) {
showAccountDetail(addressConnectedToCurrentTab)
}
}

componentDidUpdate (prevProps) {
Expand Down Expand Up @@ -357,6 +362,7 @@ Routes.propTypes = {
textDirection: PropTypes.string,
network: PropTypes.string,
provider: PropTypes.object,
selectedAddress: PropTypes.string,
frequentRpcListDetail: PropTypes.array,
currentView: PropTypes.object,
sidebar: PropTypes.object,
Expand All @@ -373,11 +379,14 @@ Routes.propTypes = {
providerId: PropTypes.string,
hasPermissionsRequests: PropTypes.bool,
autoLogoutTimeLimit: PropTypes.number,
getActiveTab: PropTypes.func,
addressConnectedToCurrentTab: PropTypes.string,
showAccountDetail: PropTypes.func,
}

Routes.defaultProps = {
selectedAddress: undefined,
}

function mapStateToProps (state) {
const { appState } = state
const {
Expand All @@ -403,6 +412,7 @@ function mapStateToProps (state) {
submittedPendingTransactions: submittedPendingTransactionsSelector(state),
network: state.metamask.network,
provider: state.metamask.provider,
selectedAddress: state.metamask.selectedAddress,
frequentRpcListDetail: state.metamask.frequentRpcListDetail || [],
currentCurrency: state.metamask.currentCurrency,
isMouseUser: state.appState.isMouseUser,
Expand All @@ -420,7 +430,6 @@ function mapDispatchToProps (dispatch) {
setCurrentCurrencyToUSD: () => dispatch(actions.setCurrentCurrency('usd')),
setMouseUserState: (isMouseUser) => dispatch(actions.setMouseUserState(isMouseUser)),
setLastActiveTime: () => dispatch(actions.setLastActiveTime()),
getActiveTab: () => dispatch(actions.getActiveTab()),
showAccountDetail: address => dispatch(actions.showAccountDetail(address)),
}
}
Expand Down
4 changes: 2 additions & 2 deletions ui/app/selectors/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -560,8 +560,8 @@ function getRenderablePermissionsDomains (state) {
}

function getOriginOfCurrentTab (state) {
const { appState: { currentActiveTab = {} } } = state
return currentActiveTab.url && getOriginFromUrl(currentActiveTab.url)
const { activeTab } = state
return activeTab && activeTab.url && getOriginFromUrl(activeTab.url)
}

function getLastConnectedInfo (state) {
Expand Down
21 changes: 0 additions & 21 deletions ui/app/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -402,9 +402,6 @@ var actions = {
getCurrentWindowTab,
SET_REQUEST_ACCOUNT_TABS: 'SET_REQUEST_ACCOUNT_TABS',
SET_CURRENT_WINDOW_TAB: 'SET_CURRENT_WINDOW_TAB',
setActiveTab,
SET_ACTIVE_TAB: 'SET_ACTIVE_TAB',
getActiveTab,
getOpenMetamaskTabsIds,
SET_OPEN_METAMASK_TAB_IDS: 'SET_OPEN_METAMASK_TAB_IDS',
}
Expand Down Expand Up @@ -3074,21 +3071,3 @@ function getCurrentWindowTab () {
dispatch(setCurrentWindowTab(currentWindowTab))
}
}

function setActiveTab (activeTab) {
return {
type: actions.SET_ACTIVE_TAB,
value: activeTab,
}
}

function getActiveTab () {
return (dispatch) => {
return global.platform.queryTabs()
.then(tabs => {
const activeTab = tabs.find(tab => tab.active)
dispatch(setActiveTab(activeTab))
return activeTab
})
}
}

0 comments on commit c5b85aa

Please sign in to comment.