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

Fix tracks hanging in safari and polyfill for bigwig tracks #3256

Merged
merged 2 commits into from Oct 10, 2022

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented Oct 8, 2022

I found that safari/webkitgtk, tracks can hang in #3245

This seems to be a safari/webkitgtk only bug

I found an odd workaround where if I console.log the value returned by makeWorkerHandle (which is a "new Worker" https://developer.mozilla.org/en-US/docs/Web/API/Worker/Worker instance) then it removes the issue with tracks hanging. This seems odd, perhaps indicative of a race condition, but I did not see issue after doing this change.

Fixes #3245

Also found two instances where tracks (pileup and snpcoverage) were using view.staticBlocks before view.initialized was completed, which throws a hard error, so that was also fixed. These errors were not seen in other browsers but i did see it thrown in safari

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Oct 8, 2022
@cmdcolin cmdcolin added bug Something isn't working and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Oct 8, 2022
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Oct 8, 2022

just for some extra detail while debugging, the main thing seen was that e.g. in WebWorkerRpc, if a track was hanging (which can be reproduced pretty reliably in safari), it would console.log in WebWorkerRpc (on the client side) but would never log in rpc.worker.ts (on the server side)

It was not just slow or race-condition-y in this sense, it would just never any code in the rpc.worker.ts. But after this change, it reliably runs code and initializes rpc.worker.ts

@cmdcolin cmdcolin force-pushed the fix_safari_hang branch 2 times, most recently from 57d56b8 to 4ada1b0 Compare October 8, 2022 19:52
@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Oct 8, 2022

just as a general observation, side scrolling is much choppier in webkitgtk than it is in firefox or chrome on my computer.

@codecov
Copy link

codecov bot commented Oct 8, 2022

Codecov Report

Merging #3256 (8d945c6) into main (5eaa86a) will decrease coverage by 0.02%.
The diff coverage is 64.00%.

❗ Current head 8d945c6 differs from pull request most recent head 3001909. Consider uploading reports for the commit 3001909 to get more accurate results

@@            Coverage Diff             @@
##             main    #3256      +/-   ##
==========================================
- Coverage   59.41%   59.39%   -0.03%     
==========================================
  Files         674      674              
  Lines       28782    28783       +1     
  Branches     7008     7014       +6     
==========================================
- Hits        17101    17095       -6     
- Misses      11406    11413       +7     
  Partials      275      275              
Impacted Files Coverage Δ
packages/core/rpc/WebWorkerRpcDriver.ts 0.00% <0.00%> (ø)
...ments/src/LinearSNPCoverageDisplay/models/model.ts 75.00% <54.54%> (+4.03%) ⬆️
packages/core/ui/FatalErrorDialog.tsx 84.61% <100.00%> (ø)
...lugins/alignments/src/LinearPileupDisplay/model.ts 66.94% <100.00%> (ø)
...ns/variants/src/VcfTabixAdapter/VcfTabixAdapter.ts 92.30% <100.00%> (ø)
...svg/src/SvgFeatureRenderer/components/Segments.tsx 85.71% <0.00%> (-7.15%) ⬇️
...src/SvgFeatureRenderer/components/FeatureLabel.tsx 77.50% <0.00%> (-5.00%) ⬇️
...gins/svg/src/SvgFeatureRenderer/components/util.ts 91.83% <0.00%> (-4.09%) ⬇️
...FeatureRenderer/components/ProcessedTranscript.tsx 87.32% <0.00%> (-1.41%) ⬇️
plugins/alignments/src/BamAdapter/BamAdapter.ts 72.07% <0.00%> (-0.91%) ⬇️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin
Copy link
Collaborator Author

can confirm fix on a real safari on mac. also found another safari issue with usage of BigInt which was used in bbi-js 2.0.0 upgrade, but this fails in safari 14 which is not that ancient (https://en.wikipedia.org/wiki/Safari_version_history#Safari_14) so I added a polyfill to help

@cmdcolin
Copy link
Collaborator Author

this fix is pretty weird. i think it reliably fixes though, so might merge for now

if we want to revisit we can perhaps dig into it and report a small reproducible issue to webkit. also could perhaps look into whether it's related to our librpc/look into removing librpc from our codebase in favor of a smaller library

@cmdcolin cmdcolin merged commit 6221f6a into main Oct 10, 2022
@cmdcolin cmdcolin deleted the fix_safari_hang branch October 10, 2022 16:02
@cmdcolin cmdcolin changed the title Fix tracks hanging in safari Fix tracks hanging in safari and polyfill BigInt for bigwig tracks for safari Oct 10, 2022
@cmdcolin cmdcolin changed the title Fix tracks hanging in safari and polyfill BigInt for bigwig tracks for safari Fix tracks hanging in safari and polyfill for bigwig tracks Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some tracks can get stuck in loading state (seen in safari/webkit)
1 participant