From e9aa14de9d64bbe0f170678b80c6074046345e0d Mon Sep 17 00:00:00 2001 From: Leila Wang Date: Tue, 25 Feb 2020 16:09:09 +0000 Subject: [PATCH] fix(sdk): fix race condition when opening multiple ui popups --- .../ApiService/RegisterExtension/index.js | 14 --- .../src/background/utils/connection.js | 105 +++++++++++++++--- .../services/ConnectionService/index.js | 24 ++-- packages/extension/src/ui/App.jsx | 5 +- packages/extension/src/ui/pages/Register.jsx | 16 --- .../src/ui/pages/RegisterAddress.jsx | 16 --- packages/extension/src/ui/pages/Restore.jsx | 41 ++----- packages/extension/src/utils/Iframe.js | 8 +- 8 files changed, 117 insertions(+), 112 deletions(-) diff --git a/packages/extension/src/background/services/ApiService/RegisterExtension/index.js b/packages/extension/src/background/services/ApiService/RegisterExtension/index.js index 1f2d0a91d..92458d35d 100644 --- a/packages/extension/src/background/services/ApiService/RegisterExtension/index.js +++ b/packages/extension/src/background/services/ApiService/RegisterExtension/index.js @@ -3,30 +3,16 @@ import { } from '~/config/event'; import filterStream from '~/utils/filterStream'; import userPermissionQuery from '~/background/services/GraphQLService/Queries/userPermissionQuery'; -import AuthService from '~/background/services/AuthService'; import query from '../utils/query'; const registerExtensionUi = async (request, connection) => { const { requestId, - domain, - data: { - args, - ...rest - }, } = request; - const registeredDomain = await AuthService.getRegisteredDomain(domain); connection.UiActionSubject.next({ ...request, type: 'ui.register.extension', - data: { - ...rest, - args: { - ...args, - domainRegistered: !!registeredDomain, - }, - }, }); return filterStream( diff --git a/packages/extension/src/background/utils/connection.js b/packages/extension/src/background/utils/connection.js index cd4a213e4..e1e3d295f 100644 --- a/packages/extension/src/background/utils/connection.js +++ b/packages/extension/src/background/utils/connection.js @@ -30,6 +30,7 @@ import { import Iframe from '~/utils/Iframe'; import { warnLog, + errorLog, } from '~/utils/log'; import { permissionError, @@ -76,6 +77,8 @@ class Connection { containerId: this.containerId, }); + this.uiRequestQueue = []; + // send the messages to the client merge(this.clientAction$, this.clientResponse$).pipe( tap(({ webClientId, ...rest }) => { @@ -112,6 +115,8 @@ class Connection { }, } = action; + await this.ensureUiRequestOrder(requestId); + const loadingElem = document.getElementById('aztec-popup-placeholder'); loadingElem.style.display = 'block'; @@ -128,6 +133,8 @@ class Connection { webClientId, }); + // wait until the iframe is ready before opening the ui + // because creating iframe will block other processes and make the loading animation look aweful const frame = await this.uiFrame.ensureCreated(); this.openUi({ @@ -185,12 +192,7 @@ class Connection { webClientId, } = this.requests[requestId]); } - this.closeUi(); - this.ClientResponseSubject.next({ - type: uiCloseEvent, - requestId, - webClientId, - }); + this.closeUi(data); const { abort, @@ -267,11 +269,9 @@ class Connection { type: clientResponseEvent, }); - this.closeUi(); - - this.ClientResponseSubject.next({ - ...data, - type: uiCloseEvent, + const ids = this.uiRequestQueue.map(({ requestId }) => requestId); + ids.forEach((requestId) => { + this.closeUi({ requestId }, false); }); const { @@ -438,9 +438,7 @@ class Connection { requestId, webClientId, }) { - this.closeUi(); - this.ClientResponseSubject.next({ - type: uiCloseEvent, + this.closeUi({ requestId, webClientId, }); @@ -455,7 +453,31 @@ class Connection { this.removeRequest(requestId); } - closeUi = () => { + closeUi = ({ + requestId, + webClientId, + }, closeBackground = true) => { + if (!this.uiRequestQueue.length) { + return false; + } + const request = this.uiRequestQueue.shift(); + if (request.requestId !== requestId) { + errorLog('Request not run in order.'); + this.uiRequestQueue.unshift(request); + return false; + } + if (this.uiRequestQueue.length) { + const { + requestId: nextRequestId, + resolve, + } = this.uiRequestQueue.shift(); + this.uiRequestQueue.unshift({ + requestId: nextRequestId, + }); + resolve(); + return false; + } + const event = new CustomEvent('closeAztec'); window.dispatchEvent(event); @@ -465,13 +487,60 @@ class Connection { type: 'closed', }, }, '*'); + + if (closeBackground) { + // wait for the fade-out animation to finish before closing the backgroundFrame + setTimeout(() => { + if (!this.uiRequestQueue.length) { + this.ClientResponseSubject.next({ + type: uiCloseEvent, + requestId, + webClientId, + }); + } + }, 500); + } + + return true; } openUi = (detail) => { - const event = new CustomEvent('openAztec', { - detail, + if (!this.uiRequestQueue.length) { + return false; + } + if (this.uiRequestQueue[0].requestId !== detail.requestId) { + errorLog('Request not run in order.'); + return false; + } + if (this.uiRequestQueue[0].resolve) { + errorLog('UI open request not resolved.'); + return false; + } + if (this.uiRequestQueue.length === 1) { + const event = new CustomEvent('openAztec', { + detail, + }); + window.dispatchEvent(event); + return true; + } + + return false; + } + + async ensureUiRequestOrder(requestId) { + if (!this.uiRequestQueue.length) { + this.uiRequestQueue.push({ + requestId, + }); + return null; + } + + return new Promise((resolve) => { + this.uiRequestQueue.push({ + requestId, + resolve, + }); }); - window.dispatchEvent(event); } removeRequest(requestId) { diff --git a/packages/extension/src/client/services/ConnectionService/index.js b/packages/extension/src/client/services/ConnectionService/index.js index 23598b2fe..e4fb8a1ca 100644 --- a/packages/extension/src/client/services/ConnectionService/index.js +++ b/packages/extension/src/client/services/ConnectionService/index.js @@ -71,36 +71,36 @@ class ConnectionService { if (!this.port) return; let requestId = this.disconnectRequestId; + let callbackKey = `${requestId}-disconnect`; if (requestId) { await new Promise((resolve) => { - if (!this.callbackMapping[requestId]) { - this.callbackMapping[requestId] = { - [uiCloseEvent]: [], - }; + if (!this.callbackMapping[callbackKey]) { + this.callbackMapping[callbackKey] = []; } - this.callbackMapping[requestId][uiCloseEvent].push(resolve); + this.callbackMapping[callbackKey].push(resolve); }); return; } requestId = randomId(); this.disconnectRequestId = requestId; + callbackKey = `${requestId}-disconnect`; - const uiDisconnected = new Promise((resolve) => { - this.callbackMapping[requestId] = { - [uiCloseEvent]: [resolve], - }; - }); + backgroundFrame.close(); await this.postToBackground({ type: clientDisconnectEvent, requestId, }); - await uiDisconnected; - this.disconnectRequestId = null; this.setInitialVars(); + + if (this.callbackMapping[callbackKey]) { + this.callbackMapping[callbackKey].forEach((cb) => { + cb(); + }); + } } async openConnection(clientProfile) { diff --git a/packages/extension/src/ui/App.jsx b/packages/extension/src/ui/App.jsx index ef90376a8..4dce666cf 100644 --- a/packages/extension/src/ui/App.jsx +++ b/packages/extension/src/ui/App.jsx @@ -165,7 +165,10 @@ class App extends PureComponent { }, minDelayTime); } else { this.setState( - nextState, + { + ...nextState, + loading: true, + }, ensureMinPendingTime(() => this.goToPage(route), minDelayTime), ); } diff --git a/packages/extension/src/ui/pages/Register.jsx b/packages/extension/src/ui/pages/Register.jsx index d912be10e..55835f0ef 100644 --- a/packages/extension/src/ui/pages/Register.jsx +++ b/packages/extension/src/ui/pages/Register.jsx @@ -1,7 +1,5 @@ import React from 'react'; import PropTypes from 'prop-types'; -import ConnectionService from '~uiModules/services/ConnectionService'; -import returnAndClose from '~/ui/helpers/returnAndClose'; import getGSNConfig from '~/ui/helpers/getGSNConfig'; import StepsHandler from '~/ui/views/handlers/StepsHandler'; import RegisterContent from '~/ui/views/RegisterContent'; @@ -9,8 +7,6 @@ import registerSteps from '~/ui/steps/register'; const Register = ({ currentAccount, - domainRegistered, - goToPage, }) => { const fetchInitialData = async () => { const gsnConfig = await getGSNConfig(); @@ -30,21 +26,11 @@ const Register = ({ }; }; - const handleClose = async (data) => { - if (domainRegistered) { - returnAndClose(data); - } else { - goToPage('loading'); - ConnectionService.returnToClient(data); - } - }; - return ( ); }; @@ -53,8 +39,6 @@ Register.propTypes = { currentAccount: PropTypes.shape({ address: PropTypes.string.isRequired, }).isRequired, - domainRegistered: PropTypes.bool.isRequired, - goToPage: PropTypes.func.isRequired, }; export default Register; diff --git a/packages/extension/src/ui/pages/RegisterAddress.jsx b/packages/extension/src/ui/pages/RegisterAddress.jsx index 51bb57550..4ec5ac444 100644 --- a/packages/extension/src/ui/pages/RegisterAddress.jsx +++ b/packages/extension/src/ui/pages/RegisterAddress.jsx @@ -1,7 +1,5 @@ import React from 'react'; import PropTypes from 'prop-types'; -import ConnectionService from '~/ui/services/ConnectionService'; -import returnAndClose from '~/ui/helpers/returnAndClose'; import getGSNConfig from '~/ui/helpers/getGSNConfig'; import StepsHandler from '~/ui/views/handlers/StepsHandler'; import RegisterContent from '~/ui/views/RegisterContent'; @@ -10,8 +8,6 @@ import apis from '~uiModules/apis'; const RegisterAddress = ({ currentAccount, - domainRegistered, - goToPage, }) => { const fetchInitialData = async () => { const gsnConfig = await getGSNConfig(); @@ -37,21 +33,11 @@ const RegisterAddress = ({ }; }; - const handleClose = async (data) => { - if (domainRegistered) { - returnAndClose(data); - } else { - goToPage('loading'); - ConnectionService.returnToClient(data); - } - }; - return ( ); }; @@ -61,8 +47,6 @@ RegisterAddress.propTypes = { address: PropTypes.string.isRequired, linkedPublicKey: PropTypes.string.isRequired, }).isRequired, - domainRegistered: PropTypes.bool.isRequired, - goToPage: PropTypes.func.isRequired, }; export default RegisterAddress; diff --git a/packages/extension/src/ui/pages/Restore.jsx b/packages/extension/src/ui/pages/Restore.jsx index 3ae6cf6e7..af22dd627 100644 --- a/packages/extension/src/ui/pages/Restore.jsx +++ b/packages/extension/src/ui/pages/Restore.jsx @@ -1,8 +1,6 @@ import React from 'react'; import PropTypes from 'prop-types'; import apis from '~uiModules/apis'; -import ConnectionService from '~uiModules/services/ConnectionService'; -import returnAndClose from '~uiModules/helpers/returnAndClose'; import AnimatedTransaction from '~/ui/views/handlers/AnimatedTransaction'; import RestoreFromSeedPhrase from '~/ui/views/RestoreFromSeedPhrase'; import CreatePassword from '~/ui/views/CreatePassword'; @@ -82,31 +80,17 @@ const steps = [ const Restore = ({ initialStep, currentAccount, - domainRegistered, - goToPage, -}) => { - const handleClose = async (data) => { - if (domainRegistered) { - returnAndClose(data); - } else { - goToPage('loading'); - ConnectionService.returnToClient(data); - } - }; - - return ( - - ); -}; +}) => ( + +); Restore.propTypes = { initialStep: PropTypes.number, @@ -114,13 +98,10 @@ Restore.propTypes = { address: PropTypes.string.isRequired, linkedPublicKey: PropTypes.string, }).isRequired, - domainRegistered: PropTypes.bool, - goToPage: PropTypes.func.isRequired, }; Restore.defaultProps = { initialStep: 0, - domainRegistered: false, }; export default Restore; diff --git a/packages/extension/src/utils/Iframe.js b/packages/extension/src/utils/Iframe.js index 8d528a4b7..f9c77ffe1 100644 --- a/packages/extension/src/utils/Iframe.js +++ b/packages/extension/src/utils/Iframe.js @@ -121,11 +121,9 @@ export default class Iframe { } close() { - setTimeout(() => { - this.frame.style.display = 'none'; - this.frame.height = 0; - this.frame.width = 0; - }, 500); + this.frame.style.display = 'none'; + this.frame.height = 0; + this.frame.width = 0; } updateStyle({