Improve error for Keyguard iframe with unexpected src #397
Conversation
There was a problem hiding this comment.
I am unsure if this will change the scenario where this error is thrown.
It will provide better feedback for future debugging either way, so good to go on my end.
Having briefly looked at the code here I think it might help, if IFrameRequestBehaviour._iframe would become a promise. All requests can then call IFrameRequestBehaviour.createIFrame first thing if it does not exist already (or assign and potentially return the promise in IFrameRequestBehaviour.createIFrame as well). Then await the stored Promise afterwards once it is needed?
Or is there a good reason why the order is this way?
There was a problem hiding this comment.
Making _iframe a Promise is generally a good idea to avoid the risk of creating a second iframe while the other one is still in the process of being created / not assigned yet.
However, I don't think making it a Promise would affect the error thrown here, as it is specifically only thrown when the iframe already exists.
sisou
force-pushed
the
soeren/client-unexpected-iframe-src
branch
from
4675b15
to
289745c
Compare
|
I have now reworked both the iframe instance and the RpcClient used in the IframeRequestBehavior to be singleton promises. For the endpoint error, now only the endpoint passed as the function argument is stored and checked against new requests (not the src of the iframe). Please re-review. |
There was a problem hiding this comment.
It's still possible to get different iframes by creating more than one IFrameRquestBehvior. If we really want to enforce having only a single iframe, _iframeEndpoint, _iframePromise and _clientPromise might be static.
And as we only want to allow a single endpoint anyways, what's the reason to not pass that only once in the constructor instead of in every request?
| } | ||
| this._iframeEndpoint = endpoint; | ||
|
|
||
| this._iframePromise = this._iframePromise || new Promise((resolve, reject) => { | ||
| const $iframe = document.createElement('iframe'); | ||
| $iframe.name = 'NimiqKeyguardIFrame'; | ||
| $iframe.style.display = 'none'; | ||
| document.body.appendChild($iframe); | ||
| $iframe.src = `${endpoint}${IFrameRequestBehavior.IFRAME_PATH_SUFFIX}`; | ||
| $iframe.onload = () => resolve($iframe); | ||
| $iframe.onerror = reject; |
There was a problem hiding this comment.
In reject case clear the _iframePromise to avoid that all subsequent createIframe calls always fail.
| const iframe = await this.createIFrame(endpoint); | ||
| if (!iframe.contentWindow) { | ||
| throw new Error(`IFrame contentWindow is ${typeof iframe.contentWindow}`); | ||
| } | ||
|
|
||
| const origin = RequestBehavior.getAllowedOrigin(endpoint); | ||
| const client = new PostMessageRpcClient(iframe.contentWindow, origin); | ||
| await client.init(); | ||
|
|
||
| resolve(client); |
There was a problem hiding this comment.
Wrap in try ... catch and reject in error case.
Or easier: replace the Promise construction by a self invoking async function (that automatically returns a promise).
On error also clear the _clientPromise.
| $iframe.onload = () => resolve($iframe); | ||
| $iframe.onerror = reject; |
There was a problem hiding this comment.
[Optional]
Might also want to clean up onload and onerror when not needed anymore but makes code more verbose and shouldn't have a real impact here.
https://sentry.io/organizations/nimiq/issues/1584763523/
Error:
Keyguard iframe is already opened with another endpointSometimes it happens that the
WalletInfoCollectorin the Hub fails to derive a batch of addresses, due to the KeyguardClient failing to forward the request to the iframe due to an iframe src error.This PR is based on the assumption, that the src is never the wrong one, but instead is not readable by the Hub context. Thus this PR adds a check for the
_iframe.srcbeing falsy (null|undefined|'') and skips that condition. For genuine missmatches, the error is made more explicit and includes the actual and expected src.The root cause of the issue might also be another one entirely, based on the process of the WalletInfoCollector. I have not investigated this further, but it might be the case that the Collector triggers a second batch of address derivations before the iframe had a chance to load, thus the src being empty (and thus not matching the expected src).