polkadot-js / extension

Simple browser extension for managing Polkadot and Substrate network accounts in a browser. Allows the signing of extrinsics using these accounts. Also provides a simple interface for compliant extensions for dapps.
Apache License 2.0
977 stars 415 forks source link

fix: service worker port #1437

Closed ryanleecode closed 3 months ago

F-OBrien commented 3 months ago

@ryanleecode do you have something I can refer to where it stays a port can be renamed if the service worker crashes or have you experienced this while testing?

If the service worker terminates a port.onDisconnect event should be emitted which will set the port to undefined. The only time the current code sets a name for the port stored in messaging.ts or content.ts is after successfully calling ensurePortConnection.

TarikGul commented 3 months ago

This seems more than reasonable - and I think "may" help resolve the issue we were discussing about. Im going to pull down the branch and do a full test before I can be too sure.

I still want to find out how I can get a step by step repro.

TarikGul commented 3 months ago

@ryanleecode do you have something I can refer to where it stays a port can be renamed if the service worker crashes or have you experienced this while testing?

If the service worker terminates a port.onDisconnect event should be emitted which will set the port to undefined. The only time the current code sets a name for the port stored in messaging.ts or content.ts is after successfully calling ensurePortConnection.

I actually experienced this issue last night while doing full runs before the release. I am having a hard time doing a full step by step repro but I was able to get this to happen twice.

Here are some pic's:

Screenshot 2024-08-06 at 10 56 31 PM Screenshot 2024-08-06 at 10 56 04 PM

cc: @F-OBrien

ryanleecode commented 3 months ago

@ryanleecode do you have something I can refer to where it stays a port can be renamed if the service worker crashes or have you experienced this while testing?

If the service worker terminates a port.onDisconnect event should be emitted which will set the port to undefined. The only time the current code sets a name for the port stored in messaging.ts or content.ts is after successfully calling ensurePortConnection.

we don't know its just a guess rn to solve the above screenshot

F-OBrien commented 3 months ago

@TarikGul that is a strange error. The port name is actually correct and not something bogus in the errors you attached so I'm not sure why it is erroring at this check.

assert([PORT_CONTENT, PORT_EXTENSION].includes(port.name), `Unknown connection from ${port.name}`);
const PORT_PREFIX = `${EXTENSION_PREFIX || 'unknown'}-${process.env['PORT_PREFIX'] || 'unknown'}`;

export const PORT_CONTENT = `${PORT_PREFIX}-content`;
export const PORT_EXTENSION = `${PORT_PREFIX}-extension`;

where EXTENSION_PREFIX = 'polkadot{.js}' and 0x8eeee0f75a689b99 is the port prefix PORT_PREFIX: JSON.stringify(blake2AsHex(JSON.stringify(manifest), 64)) so the actual ports should be polkadot{.js}-0x8eeee0f75a689b99-extension or polkadot{.js}-0x8eeee0f75a689b99-content

TarikGul commented 3 months ago

where EXTENSION_PREFIX = 'polkadot{.js}' and 0x8eeee0f75a689b99 is the port prefix PORT_PREFIX: JSON.stringify(blake2AsHex(JSON.stringify(manifest), 64)) so the actual ports should be polkadot{.js}-0x8eeee0f75a689b99-extension or polkadot{.js}-0x8eeee0f75a689b99-content

Yea this is what I was able to dig up last night as well, which confused me even more. I would have thought it was a one off thing - but I was able to get this error twice.

ryanleecode commented 3 months ago

include [PORT_CONTENT, PORT_EXTENSION] as part of the logs to see if they differ

TarikGul commented 3 months ago
PORT_CONTENT:  polkadot{.js}-0x8ecd92f31039f04d-content
PORT_EXTENSION:  polkadot{.js}-0x8ecd92f31039f04d-extension

This is what I am getting on chrome which differs from what the error was showing

Let me check firefox and see if there was some kind of mishap

F-OBrien commented 3 months ago
PORT_CONTENT:  polkadot{.js}-0x8ecd92f31039f04d-content
PORT_EXTENSION:  polkadot{.js}-0x8ecd92f31039f04d-extension

This is what I am getting on chrome which differs from what the error was showing

Let me check firefox and see if there was some kind of mishap

I expect the hash could have changed as the version in the manifest file has changed? You could compare the two manifest files in the master-ff-build and master-chrome-build zips

edit: actually they should both have different hashes anyway as FF an Chrome have different manifest files

TarikGul commented 3 months ago

I figured it out!! WOW what a stupid issue.

Okay @F-OBrien @ryanleecode thank you both for helping narrow this down.

SO... I noticed from above the hash that chrome was disconnecting from was the firefox manifest hash. Now how did the firefox hash get mixed up in chrome? I load unpacked in chrome - tested there and then rebuilt the manifest for firefox doing yarn build:ff, and since chrome makes a reference to the manifest file that has now changed, it throws the errors along with ...loading....

Such an odd error, sorry if any time was wasted for either of you! Going to release the extension now as everything looks good to go :)

polkadot-js-bot commented 3 months ago

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.