Fix: ensure the service worker is awake before every port message

[F-OBrien]

This builds on the approach taken in #1424 but instead of only doing it once when opening the popup it does it before any message over a port from either the extension UI or content script. This prevents attempting to send message over disconnected ports or while the service worker is idle or terminated and should hopefully close multiple open issues including #1432, #1420, #1417, #1416, #1415.

This PR does not add a heartbeat to the service worker so if there are transactions that may take a long time to return it is still possible the service worker may timeout and become idle however I've not seen instance of this from my testing as when connected to a dApp there is a ping message being sent every 5 seconds.

secondly I added a check filter out non http url when setting an active tab as the stripUrl function was always erroring for pages like browser tabs like chrome://newtab/ and removed a WINDOW console log which appeared to have been left in when debugging.

@TarikGul

[TarikGul]

Overall, really solid stuff. Thanks so much - I see just the failing tests - and once that is resolved this can go in!

[F-OBrien]

Overall, really solid stuff. Thanks so much - I see just the failing tests - and once that is resolved this can go in!

@TarikGul I tried to fix that test and I'm honestly struggling to figure out how to get it to work. It won't let me mock an implementation of ensurePortConnection to return a mockPort with jest.spyOn and I can't see any other easy way to get this to pass. Any help with the test is appreciated. I'm not going to have much access to a laptop again until next Tuesday

[TarikGul]

Overall, really solid stuff. Thanks so much - I see just the failing tests - and once that is resolved this can go in!

@TarikGul I tried to fix that test and I'm honestly struggling to figure out how to get it to work. It won't let me mock an implementation of ensurePortConnection to return a mockPort with jest.spyOn and I can't see any other easy way to get this to pass. Any help with the test is appreciated. I'm not going to have much access to a laptop again until next Tuesday

Would love to help out on it! I'll have a look :)

[TarikGul]

The error comes from https://github.com/polkadot-js/dev/blob/master/packages/dev-test/src/env/expect.ts#L92-L102 which is part of toHaveBeenCalledWith. I know this is obvious but I wanted to note it down.

I looked into to this for an hour or so, and will get back to it soon, but some things to note:

1. The extension-mocks package is a bit out of date. It uses sinon-chrome which is note compatible with v3 - we did some patching for now to make it compatible but there could also be an issue here as well in some way.

2. It seems as if there is an underlying issue in the test with how the listeners are being called or captured.

3. The PJS test wrapper is a bit challenging to work with since its very custom, but also has no documentation. I know parts of it, but let me see if I can find some functionality that may help us debug this.

[ryanleecode]

@F-OBrien the issue seems to be the test requires the background script to respond with wakeup but it seems like sinon isn't running the background script. So the port is never connected

[TarikGul]

@F-OBrien the issue seems to be the test requires the background script to respond with wakeup but it seems like sinon isn't running the background script. So the port is never connected

To add to this too sinon-chrome doesn't support V3 either which makes it a bit more difficult. But I guess we could monkey patch it ourselves

[F-OBrien]

@F-OBrien the issue seems to be the test requires the background script to respond with wakeup but it seems like sinon isn't running the background script. So the port is never connected

Yes, that's why I was trying to mock ensurePortConnection or wakeUpServiceWorker with jest.spyOn but they just error. If there is a way with Sinon chrome to mock the awake response message then that could would work too. I'd have thought the send message API should be the same for manifest V2 and V3 but I've not dug into Sinon chrome to see what it can do.

The test framework seems quite custom so I'm not exactly clear what the options are. I was thinking I could try use to use Sinon rather than jest to fake functions that use the chrome api it instead to see if we can get that to work. Ideally we'd like to be able to test as much of the code as possible without mocks.

[Juanma0x]

We have received a report from a user trying to add a Ledger account to the Polkadot extension. Will it be fixed after this PR is deployed?

[F-OBrien]

@TarikGul this took me way to long to figure out but I've finally got the test to pass. I needed to create a object to wrap wakeUpServiceWorker so I could use jest.spyOn and also import from the relative path instead of using the @polkadot/extension-base path.

[F-OBrien]

We have received a report from a user trying to add a Ledger account to the Polkadot extension. Will it be fixed after this PR is deployed?

@Juanma0x I believe it should

[TarikGul]

@TarikGul this took me way to long to figure out but I've finally got the test to pass. I needed to create a object to wrap wakeUpServiceWorker so I could use jest.spyOn and also import from the relative path instead of using the @polkadot/extension-base path.

Brilliant! You're amazing thanks so much for this - this was pretty critical, hats off to you! Once this goes in I will bump the PJS deps as all the upstream repos have been updated, then we can go ahead and release this and push it to the store.

[TarikGul]

lgtm 🚀

[polkadot-js-bot]

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.