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

Perspective + CodeMirror 6 clash after update from 2.10.0 to 2.10.1 #2704

Closed
mhkeller opened this issue Aug 6, 2024 · 9 comments
Closed

Comments

@mhkeller
Copy link

mhkeller commented Aug 6, 2024

Bug Report

The original issue is locked and I can't post this reproduction there so I'm making a new issue.

Steps to Reproduce:

Here's a reproduction of #2635

https://github.com/mhkeller/perspective-codemirror-bug

If you comment out codemirror, perspective loads fine

Expected Result:

The two load together fine.

Actual Result:

If you load code mirror 6 with perspective 2.10.1, you get a RunTimeError: unreachable error
error

Environment:

See reproduction

@texodus
Copy link
Member

texodus commented Aug 6, 2024

Thanks a bunch for the repro @mhkeller!

The issue is due to a bug in wasm-bindgen@0.2.87 used by perspective-viewer@2.10.1. Upgrading to perspective-viewer@3.0.0-rc.1 (which already uses a fixed wasm-bindgen@0.2.91) or downgrading to perspective-viewer@2.10.0 are your only options. I've opened a PR against your sample repo that uses perspective@3.0.0-rc.1 from jsdelivr as an example.

Running your example in debug mode shows the trigger as an assertion failure deep in the memory allocation code - it does not seem to be strictly related to CodeMirror per se, more that this specific version of CodeMirror's CSS rules just happen to apply to perspective-viewer also, and in parsing the string rules themselves to discover Theme values, this path happens to trip a rare allocation failure.

@mhkeller
Copy link
Author

mhkeller commented Aug 6, 2024

Awesome thanks for looking at it so quickly. I imagined that with a bump in the major version this would likely be resolved. Do you happen to know which CSS rules are clashing? Could be good for users who have that version of codemirror to try to put something in to mitigate any display or functionality issues.

@texodus
Copy link
Member

texodus commented Aug 6, 2024

It's not exactly a CSS clash - it is a rare string allocation issue that just happens to be triggered by CodeMirror's CSS. It can be mitigated by doing basically any trivial text modification of this file (for example, Prospective.co just happens to use a CSS bundler). You may similarly be able to configure parcel to bundle CSS differently.

These aren't sane solutions though - they are just randomizing the input so a rare/random memory bug is avoided.

Uncaught (in promise) RuntimeError: unreachable
    at perspective.wasm.dlmalloc::dlmalloc::Dlmalloc<A>::validate_size::ha0b54ee3fb3ae665 (perspective.wasm-0120b59e:0x260c98)
    at perspective.wasm.__rdl_dealloc (perspective.wasm-0120b59e:0x2ca532)
    at perspective.wasm.__rust_dealloc (perspective.wasm-0120b59e:0x2d504c)
    at perspective.wasm.<alloc::raw_vec::RawVec<T,A> as core::ops::drop::Drop>::drop::h50b2562ef7d70c97 (perspective.wasm-0120b59e:0x2cc2bd)
    at perspective.wasm.perspective::presentation::get_theme_names::hb47b50f067804733 (perspective.wasm-0120b59e:0x749a7)

@mhkeller
Copy link
Author

mhkeller commented Aug 6, 2024

Got it. Well, on the bright side, the CSS isn’t clashing. I think I’ll keep 2.10.0 for now and wait for 3.0. Any sense of when that will be ready?

@texodus
Copy link
Member

texodus commented Aug 6, 2024

Soon, but 3.0.0-rc.1 is already available and will the JavaScript libraries will be identical to those released in 3.0.0. The only things we'll change between now and 3.0.0 are related to the packaging process for JupyterLab.

@mhkeller
Copy link
Author

mhkeller commented Aug 6, 2024

Ah great. Sounds like a good option then.

@texodus
Copy link
Member

texodus commented Aug 23, 2024

3.0.0 is now released, closing

@texodus texodus closed this as completed Aug 23, 2024
@mhkeller
Copy link
Author

Congrats on the release! I'll try to get it set up with that version. I assume these docs are outdated right? I did a quick test when we were talking three weeks ago and ran into an issue where I have to update to esm.

@texodus
Copy link
Member

texodus commented Aug 23, 2024

The docs are up-to-date and they talk about ESM in the CDN and bundler sections.

There are holes and blindspots befitting a major refactor - let us know if you think something needs to be more clear and we'll update the language.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants