Page MenuHomePhabricator

[Cashtab] Stop using port object to prevent port disconnect errors
ClosedPublic

Authored by bytesofman on Dec 22 2023, 21:21.

Details

Summary

T3370

Looking into this more. Really we don't have any need to use the port object to maintain long lived connections. Cashtab extension requests are one-off msg passing. Chrome has method to support this, so use it instead.

Note: The port passing is preserved for address sharing as, in testing, it was nontrivial to remove this extension-side. Also, have not seen the port disconnect error for the address passing feature, so lower priority to patch.

Test Plan

npm run extension
Navigate to components.cashtab.com
Click some of the CashtabBadge components and confirm extension pops up
Confirm Address Sharing works with accepting and requesting the address from the extension
Confirm you don't see 'port disconnect' error after lots of clicks

Diff Detail

Repository
rABC Bitcoin ABC
Branch
extension-patch-fiat
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 26066
Build 51706: Build Diffcashtab-tests
Build 51705: arc lint + arc unit

Event Timeline

bytesofman updated this revision to Diff 43775.

back out unrelated changes

Fabien added inline comments.
cashtab/extension/src/service_worker.js
49 ↗(On Diff #43776)

Do you still need that ? It looks duplicated

cashtab/extension/src/service_worker.js
49 ↗(On Diff #43776)

good catch.

1 - yes should be deleted
2 - accidentally not deleting this is why the address sharing was still working with the legacy port method

fully deprecating port object as it is only needed for special long lived connections. Cashtab extension use cases only requires single event msg passing.

deprecate port object and port listener

cashtab/src/components/AppModes/Extension.js
108 ↗(On Diff #43792)

this was issue holding up port object deprecation. Did not need tabId to be a number with old API.

emack requested changes to this revision.Dec 29 2023, 00:14
emack added a subscriber: emack.

Tested all ok in chrome however I feel like the usage of chrome APIs in this diff is the start of permanently locking the extension into the chrome ecosystem and any future plans to port this over to firefox, brave...etc will require significant refactor. Double checking here that this is the intention?

On an unrelated note, wasn't there a recent diff that stopped creating new extension windows for each interaction with a button component? Not for solving in this diff but it's still occurring here.

image.png (580×579 px, 62 KB)

This revision now requires changes to proceed.Dec 29 2023, 00:14

Tested all ok in chrome however I feel like the usage of chrome APIs in this diff is the start of permanently locking the extension into the chrome ecosystem and any future plans to port this over to firefox, brave...etc will require significant refactor. Double checking here that this is the intention?

The use of extensionizer to run chrome APIs means we could (in theory) still extend to other browsers supported by extensionizer -- is the chrome param used directly anywhere?

There would still be more work to do + testing to make sure everything works in another browser, but we aren't really adding more chrome APIs here, really we are getting rid of one.

will check on the window killing situation.

On an unrelated note, wasn't there a recent diff that stopped creating new extension windows for each interaction with a button component? Not for solving in this diff but it's still occurring here.

What steps are you following to create this / what browser? Do you also see this in master or just this PR?

I'm not able to repeat clicking on various CashtabBadge / GetAddress at components.cashtab.com

Tested all ok in chrome however I feel like the usage of chrome APIs in this diff is the start of permanently locking the extension into the chrome ecosystem and any future plans to port this over to firefox, brave...etc will require significant refactor. Double checking here that this is the intention?

The use of extensionizer to run chrome APIs means we could (in theory) still extend to other browsers supported by extensionizer -- is the chrome param used directly anywhere?

There would still be more work to do + testing to make sure everything works in another browser, but we aren't really adding more chrome APIs here, really we are getting rid of one.

will check on the window killing situation.

ok that's fine, haven't seen a chrome param used elsewhere. We can deal with the window killing issue outside of this diff.

This revision is now accepted and ready to land.Dec 29 2023, 22:10