Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename provider streams, notify provider of stream failures #10006

Merged
merged 1 commit into from
Dec 8, 2020
Merged

Rename provider streams, notify provider of stream failures #10006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 44 additions & 48 deletions app/scripts/contentscript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,13 @@ const inpageContent = fs.readFileSync(
const inpageSuffix = `//# sourceURL=${extension.runtime.getURL('inpage.js')}\n`
const inpageBundle = inpageContent + inpageSuffix

// Eventually this streaming injection could be replaced with:
// https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.exportFunction
//
// But for now that is only Firefox
// If we create a FireFox-only code path using that API,
// MetaMask will be much faster loading and performant on Firefox.
const CONTENT_SCRIPT = 'metamask-contentscript'
const INPAGE = 'metamask-inpage'
const PROVIDER = 'metamask-provider'

if (shouldInjectProvider()) {
injectScript(inpageBundle)
start()
setupStreams()
}

/**
Expand All @@ -46,15 +43,6 @@ function injectScript(content) {
}
}

/**
* Sets up the stream communication and submits site metadata
*
*/
async function start() {
await setupStreams()
await domIsReady()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we no longer need to know that the dom is ready?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
* Sets up two-way communication streams between the
* browser extension and local per-page browser context.
Expand All @@ -63,10 +51,10 @@ async function start() {
async function setupStreams() {
// the transport-specific streams for communication between inpage and background
const pageStream = new LocalMessageDuplexStream({
name: 'contentscript',
target: 'inpage',
name: CONTENT_SCRIPT,
target: INPAGE,
})
const extensionPort = extension.runtime.connect({ name: 'contentscript' })
const extensionPort = extension.runtime.connect({ name: CONTENT_SCRIPT })
const extensionStream = new PortStream(extensionPort)

// create and connect channel muxers
Expand All @@ -79,25 +67,26 @@ async function setupStreams() {
pump(pageMux, pageStream, pageMux, (err) =>
logStreamDisconnectWarning('MetaMask Inpage Multiplex', err),
)
pump(extensionMux, extensionStream, extensionMux, (err) =>
logStreamDisconnectWarning('MetaMask Background Multiplex', err),
)
pump(extensionMux, extensionStream, extensionMux, (err) => {
logStreamDisconnectWarning('MetaMask Background Multiplex', err)
notifyInpageOfStreamFailure()
})

// forward communication across inpage-background for these channels only
forwardTrafficBetweenMuxers('provider', pageMux, extensionMux)
forwardTrafficBetweenMuxes(PROVIDER, pageMux, extensionMux)

// connect "phishing" channel to warning system
const phishingStream = extensionMux.createStream('phishing')
phishingStream.once('data', redirectToPhishingWarning)
}

function forwardTrafficBetweenMuxers(channelName, muxA, muxB) {
function forwardTrafficBetweenMuxes(channelName, muxA, muxB) {
const channelA = muxA.createStream(channelName)
const channelB = muxB.createStream(channelName)
pump(channelA, channelB, channelA, (err) =>
logStreamDisconnectWarning(
pump(channelA, channelB, channelA, (error) =>
console.debug(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Was this change made simply to clean up the log output (Not prefixing it with the connection lost message?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah; I just decided that these were cluttering the user's console with warnings for little benefit, and so downgraded them to debugs.

`MetaMask muxed traffic for channel "${channelName}" failed.`,
err,
error,
),
)
}
Expand All @@ -106,14 +95,35 @@ function forwardTrafficBetweenMuxers(channelName, muxA, muxB) {
* Error handler for page to extension stream disconnections
*
* @param {string} remoteLabel - Remote stream name
* @param {Error} err - Stream connection error
* @param {Error} error - Stream connection error
*/
function logStreamDisconnectWarning(remoteLabel, err) {
let warningMsg = `MetamaskContentscript - lost connection to ${remoteLabel}`
if (err) {
warningMsg += `\n${err.stack}`
}
console.warn(warningMsg)
function logStreamDisconnectWarning(remoteLabel, error) {
console.debug(
`MetaMask Contentscript: Lost connection to "${remoteLabel}".`,
error,
)
}

/**
* This function must ONLY be called in pump destruction/close callbacks.
* Notifies the inpage context that streams have failed, via window.postMessage.
* Relies on obj-multiplex and post-message-stream implementation details.
*/
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,
)
Comment on lines +112 to +126
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
Expand Down Expand Up @@ -220,17 +230,3 @@ function redirectToPhishingWarning() {
href: window.location.href,
})}`
}

/**
* Returns a promise that resolves when the DOM is loaded (does not wait for images to load)
*/
async function domIsReady() {
// already loaded
if (['interactive', 'complete'].includes(document.readyState)) {
return undefined
}
// wait for load
return new Promise((resolve) =>
window.addEventListener('DOMContentLoaded', resolve, { once: true }),
)
}
4 changes: 2 additions & 2 deletions app/scripts/inpage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ log.setDefaultLevel(process.env.METAMASK_DEBUG ? 'debug' : 'warn')

// setup background connection
const metamaskStream = new LocalMessageDuplexStream({
name: 'inpage',
target: 'contentscript',
name: 'metamask-inpage',
target: 'metamask-contentscript',
})

initializeProvider({
Expand Down
2 changes: 1 addition & 1 deletion app/scripts/metamask-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1813,7 +1813,7 @@ export default class MetamaskController extends EventEmitter {
const mux = setupMultiplex(connectionStream)

// messages between inpage and background
this.setupProviderConnection(mux.createStream('provider'), sender)
this.setupProviderConnection(mux.createStream('metamask-provider'), sender)
}

/**
Expand Down
4 changes: 2 additions & 2 deletions test/unit/app/controllers/metamask-controller-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,7 +1022,7 @@ describe('MetaMaskController', function () {
}
streamTest.write(
{
name: 'provider',
name: 'metamask-provider',
data: message,
},
null,
Expand Down Expand Up @@ -1061,7 +1061,7 @@ describe('MetaMaskController', function () {
}
streamTest.write(
{
name: 'provider',
name: 'metamask-provider',
data: message,
},
null,
Expand Down