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.

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

Use browser extension Port objects instead of hacky comlink string channel #10302

Merged
merged 13 commits into from
May 4, 2020

Conversation

felixfbecker
Copy link
Contributor

@felixfbecker felixfbecker commented Apr 30, 2020

Comlink relies on creating MessagePorts for each proxied object and transferring those with postMessage(). But browser extension Port objects don't support transferring MessagePorts (or anything really).

Because of this, we used a hacky adapter that channeled every MessagePort through a single Port using string IDs (and serializing everything to a string, although that's not required).

This string message adapter was a hacky addition to comlink v3 that "should have never made it to master" according to its creator, and is missing in v4. There is an experimental one in v4, but it is broken, and actually broke our browser extension (I thought I had tested this, but I must have made a mistake, maybe I had a prod browser extension running? 🤷‍♂️ Really wish we had tests for this in CI).

Because I couldn't get it fixed, I instead rethought our approach and worked on a solution that doesn't use a string wrapper. While Ports don't support transferables, it is possible to open multiple connections. How it works with this PR:

  • We capture all incoming Port connections and map them by their name.
  • Whenever we see a MessagePort in a message to be sent, we open a Port to the other side with a UUID name.
  • We include in the message sent over the Port the UUID and path at which the MessagePort appeared.
  • When we receive a message with UUID port references we take (or wait for) a connection with the UUID.
  • We hook that Port up to a MessagePort, and splice that MessagePort back into the message at the path it appeared.

@lguychard I hope I am not forgetting a reason why we didn't do this in the first place (besides saving effort) 😅

Also would like to source the swarm intelligence to figure out where we need to add ensure resources are cleaned up and not leaked. Although I'm relatively sure we were leaking before - we're only using the new v4 [releaseProxy]() as of this PR.

Tested:

  • Chrome extension
  • Firefox extension
  • Web app
  • Native integration: Bitbucket server

@felixfbecker felixfbecker added bug An error, flaw or fault that produces an incorrect or unexpected result, or behavior. browser-extension debt Technical debt. team/web labels Apr 30, 2020
@felixfbecker felixfbecker requested a review from a team April 30, 2020 16:50
@codecov
Copy link

codecov bot commented May 1, 2020

Codecov Report

Merging #10302 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #10302   +/-   ##
=======================================
  Coverage   44.38%   44.38%           
=======================================
  Files        1410     1410           
  Lines       77432    77432           
  Branches     6938     6938           
=======================================
  Hits        34372    34372           
  Misses      39839    39839           
  Partials     3221     3221           
Flag Coverage Δ
#go 47.33% <0.00%> (ø)
#typescript 35.32% <0.00%> (ø)
#unit 44.38% <0.00%> (ø)

@felixfbecker felixfbecker force-pushed the comlink-no-string-channel branch 2 times, most recently from 401bcc3 to a0e8ede Compare May 2, 2020 15:52
Copy link
Contributor

@lguychard lguychard left a comment

Choose a reason for hiding this comment

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

Code looks good. Also tested locally, and works like a charm 🙂

I hope I am not forgetting a reason why we didn't do this in the first place (besides saving effort)

We didn't go down this route at the time because the string message adapter in comlink v3 didn't require it, so there was no reason to spend the effort 🙂

browser/src/platform/ports.ts Outdated Show resolved Hide resolved
browser/src/platform/ports.ts Outdated Show resolved Hide resolved
browser/src/platform/ports.ts Outdated Show resolved Hide resolved
shared/src/api/client/api/common.ts Outdated Show resolved Hide resolved
shared/src/api/client/api/common.ts Show resolved Hide resolved
shared/src/api/client/api/languageFeatures.ts Outdated Show resolved Hide resolved
Web Team :: Current iteration automation moved this from Needs review to Reviewer approved May 4, 2020
@felixfbecker felixfbecker added this to the 3.16 milestone May 4, 2020
@lguychard lguychard mentioned this pull request May 4, 2020
41 tasks
Copy link
Contributor

@lguychard lguychard left a comment

Choose a reason for hiding this comment

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

Look good!

@felixfbecker
Copy link
Contributor Author

🎉

@felixfbecker felixfbecker merged commit 3cedeb5 into master May 4, 2020
Web Team :: Current iteration automation moved this from Reviewer approved to Done May 4, 2020
@felixfbecker felixfbecker deleted the comlink-no-string-channel branch May 4, 2020 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser-extension bug An error, flaw or fault that produces an incorrect or unexpected result, or behavior. debt Technical debt.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants