Rename provider streams, notify provider of stream failures#10006
Rename provider streams, notify provider of stream failures#10006rekmarks merged 1 commit intodelete-public-config-storefrom
Conversation
ee064fd to
79ba597
Compare
e321e91 to
a8c452d
Compare
79ba597 to
a26c84c
Compare
a8c452d to
f3700ca
Compare
Builds ready [f3700ca]
Page Load Metrics (355 ± 33 ms)
|
| function notifyInpageOfStreamFailure() { | ||
| window.postMessage( | ||
| { | ||
| target: INPAGE, // the post-message-stream "target" | ||
| data: { | ||
| // this object gets passed to obj-multiplex | ||
| name: PROVIDER, // the obj-multiplex channel name | ||
| data: { | ||
| jsonrpc: '2.0', | ||
| method: 'METAMASK_STREAM_FAILURE', | ||
| }, | ||
| }, | ||
| }, | ||
| window.location.origin, | ||
| ) |
There was a problem hiding this comment.
We use postMessage directly here because, by the time we want to send this message, the streams will have been torn down because they are all pump:ed together, and pump tears them all down if even a single stream fails.
a26c84c to
81618d7
Compare
f3700ca to
57ce6bb
Compare
Builds ready [57ce6bb]
Page Load Metrics (364 ± 40 ms)
|
brad-decker
left a comment
There was a problem hiding this comment.
Couple questions, but appears to be working and LGTM
| pump(channelA, channelB, channelA, (err) => | ||
| logStreamDisconnectWarning( | ||
| pump(channelA, channelB, channelA, (error) => | ||
| console.debug( |
There was a problem hiding this comment.
Q: Was this change made simply to clean up the log output (Not prefixing it with the connection lost message?)
There was a problem hiding this comment.
Yeah; I just decided that these were cluttering the user's console with warnings for little benefit, and so downgraded them to debugs.
| */ | ||
| async function start() { | ||
| await setupStreams() | ||
| await domIsReady() |
There was a problem hiding this comment.
Why do we no longer need to know that the dom is ready?
There was a problem hiding this comment.
I think we needed it in the past because we pulled metadata in this script instead of in the inpage provider. Now, we were just calling start, which first set up the streams, and then waited for the DOM to be ready to do, nothing.
35b48d6 to
62e6602
Compare
57ce6bb to
5801a4e
Compare
This PR renames the streams we use to connect to the
inpage-provider, and ensures that the provider is notified when the streams connecting the content script to the MetaMask background fails.Currently, the provider streams have very generic names. Although it is unlikely that this would cause problems in production, we could encounter naming collisions with other extensions that use our stream packages in production. In any event, it doesn't cost us anything to rename the streams. IIRC, the last time I suggested we do this, @Gudahtt mentioned that it could be breaking for e.g. sites that bring our provider to get around the Firefox CSP issue where injection fails, but we are shipping so many breaking changes that those folks will have to upgrade anyway.
Now, for the second change, if the extension background process is restarted, all pages will have to be reloaded in order for the provider to establish its connection to the new background process. Historically, three warnings about MetaMask stream failures have appeared in the page console when this happens. In these cases, the provider should emit the
disconnectevent. However, this never happened in practice, because it was the content script streams that failed, and the provider was never notified of this. We improve upon this state of affairs by informing the provider that the connection to the background is lost, so that it can emit thedisconnectevent, and the user can reload the page.Corresponding
inpage-providerPR: https://github.com/MetaMask/inpage-provider/pull/120