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

Initialization failure on Firefox #31

Closed
Devmacs opened this issue May 5, 2020 · 12 comments · Fixed by #35
Closed

Initialization failure on Firefox #31

Devmacs opened this issue May 5, 2020 · 12 comments · Fixed by #35
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@Devmacs
Copy link
Contributor

Devmacs commented May 5, 2020

Firefox 75, using the provided en-US dictionary. Works fine in Chrome.

When initializing, we get this error :

RangeError: "attempting to construct out-of-bounds TypedArray on ArrayBuffer"

This happens in the writeToBuffer call in index.js

I seem to get better results if I set the initial WebAssembly memory to 1000 pages instead of 1, but it still crashes. With devtools open, it seems to crash less.

@justinwilaby
Copy link
Owner

Could be related to a detached ArrayBuffer. It seems the rules are different for FF. I'll investigate.

@justinwilaby justinwilaby added the bug Something isn't working label May 6, 2020
@justinwilaby justinwilaby self-assigned this May 6, 2020
@Devmacs
Copy link
Contributor Author

Devmacs commented May 6, 2020

Another hint, if I halve the size of the dictionary, then it seems to crash less often.

@justinwilaby
Copy link
Owner

Conditionally creating a new Uint8Array here is an optimization that drastically speeds up loading the dictionary. If the function body is replaced with:

this.writeBuffer = new Uint8Array(memory.buffer, 0, memory.buffer.byteLength);

I imagine the problem will go away but the write speed will be atrocious.

@justinwilaby
Copy link
Owner

related to #30

@Devmacs
Copy link
Contributor Author

Devmacs commented May 6, 2020

Could it somehow be conditionally set for Firefox? It may be acceptable to have worse performance on Firefox as a trade-off for not crashing.

@Devmacs
Copy link
Contributor Author

Devmacs commented May 7, 2020

I tried your fix locally but still saw the errors in Firefox. Did I miss anything? Was there something else to change?

@justinwilaby
Copy link
Owner

I did some digging and was able to move past the RangeError only to run into a index out of bounds error inside the wasm. This is specific to FireFox and occurs within (*BUFFER).extend_from_slice(slice::from_raw_parts(ptr, length));.

The extend_from_slice function fails in FF but the exact same code works in Chrome. I'm not sure how to deal with this yet since it's a critical function and there are few (if any) substitutes.

@Devmacs
Copy link
Contributor Author

Devmacs commented May 7, 2020

Can't it be swapped out for extend? It seems that extend is already specialized for slices and that extend_from_slice could become deprecated :

See bjorn3's comment in :
rust-lang/rust#52774

@justinwilaby
Copy link
Owner

Tried that already of course. Same result.

@justinwilaby
Copy link
Owner

Looks like this is a FF issue and has to do with transmute. I can repro this issue simply by calling (*BUFFER).len()

This will be tricky since the implication is that statics are not available in FF. The reason I've chosen to use them is because parsing the dictionary is quite slow on the JS side and it doesn't make sense to create a new instance of SymSpell on the wasm side each time a spell check occurs.

I'm leaving this open and asking for help. I'm sure theres a fix but it's eluding me atm.

@justinwilaby justinwilaby added the help wanted Extra attention is needed label May 7, 2020
@Devmacs
Copy link
Contributor Author

Devmacs commented May 7, 2020

I know it's not a compile-time error, but I'll leave this here on the odd chance that it helps :

https://users.rust-lang.org/t/static-allocation-for-webassembly-with-refcell/14420/3

@Devmacs
Copy link
Contributor Author

Devmacs commented May 8, 2020

Does transmute really need to be used here? There might be an alternative : https://doc.rust-lang.org/nomicon/transmutes.html

If it does, it seems that it should probably be qualified as it is here as to reduce UB :
src/sym_spell/suggested_item.rs:037

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants