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.

Sign up for GitHub

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

Jump to bottom

bug(web): undefined reading 'deleteLeft' causing crash in Android app #12494

Open
darcywong00 opened this issue Oct 2, 2024 · 8 comments · May be fixed by #12864
Open

bug(web): undefined reading 'deleteLeft' causing crash in Android app #12494

darcywong00 opened this issue Oct 2, 2024 · 8 comments · May be fixed by #12864
Assignees
@jahorton
Labels
bug web/
Milestone
A18S19

Comments

@darcywong00
Copy link
Contributor

darcywong00 commented Oct 2, 2024 •
edited
Loading

@dinakaranr recorded and reported this issue using the current Keyman for Android 18.0 alpha
(I can repro on 18.0.117-alpha)

Steps to repro:

  1. In the Keyman app with the default sil_euro_latin keyboard
  2. Longpress g --> g̃
  3. Longpress h --> ḥ
  4. Longpress j --> ĵ
  5. Flick down on c --> '
  6. Flick down on v --> Javascript error

The chrome dev tools gives the following error

@keymanapp/keyman/we…es/src/common.ts:40 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'deleteLeft')
    at buildMergedTransform (@@keymanapp/keyman/web/src/engine/predictive-text/templates/src/common.ts:40:43)
    at ContextTracker.attemptMatchContext (@keymanapp/keyman/web/src/engine/predictive-text/templates/src/common.ts:444:57)
    at _ContextTracker.analyzeState (@keymanapp/keyman/web/src/engine/predictive-text/worker-thread/src/main/correction/context-tracker.…:673:37)
    at @keymanapp/keyman/we…t-helpers.ts:175:48
    at Generator.next (<anonymous>)
    at blob:null/1b8791be-3…-49f3ec17d480:49:63
    at new Promise (<anonymous>)
    at __async (blob:null/1b8791be-3…-49f3ec17d480:33:12)
    at correctAndEnumerate (@keymanapp/keyman/we…ct-helpers.ts:103:4)
    at _ModelCompositor.<anonymous> (@keymanapp/keyman/we…ompositor.ts:122:56)
@darcywong00
Copy link
Contributor Author

darcywong00 commented Oct 2, 2024 •
edited
Loading

His video was too large to attach to GH so I will split it...

Uploading output_video.mp4…

@ermshiperete
Copy link
Contributor

ermshiperete commented Oct 2, 2024 •
edited
Loading

To get the error it is sufficient to do steps 5 and 6. Same thing happens with KeymanWeb test page.

Note that you'll have to switch keyboards to get the error again after it happened.

@mcdurdin mcdurdin added this to the A18S19 milestone Nov 4, 2024
@mcdurdin mcdurdin assigned ermshiperete and darcywong00 and unassigned jahorton Nov 18, 2024
@darcywong00
Copy link
Contributor Author

Since this is web, I'll unassign myself on this. @ermshiperete can get to it A18S19

@darcywong00 darcywong00 removed their assignment Nov 26, 2024
@ermshiperete ermshiperete moved this to Todo in Keyman Dec 6, 2024
@ermshiperete
Copy link
Contributor

I'm no longer able to reproduce this. @dinakaranr Can you please re-test and see if this works now for you as well?

@ermshiperete ermshiperete moved this from Todo to In Progress in Keyman Jan 6, 2025
ermshiperete added a commit that referenced this issue Jan 6, 2025
@ermshiperete
fix(web): don't crash if second transform is null
c6f8ab6 
Fixes: #12494
@ermshiperete ermshiperete mentioned this issue Jan 6, 2025
Closed
@ermshiperete
Copy link
Contributor

#12860 prevents the crash in the code which otherwise could still happen even though I wasn't able to reproduce it.

@dinakaranr
Copy link

I tested this issue with the attached "18.0.162-alpha" build on the Andoird 14 and am sharing my observations here. I reproduced this crash using the "sil_euro_latin" keyboard.
Open the keyman app in the Android mobile app. Added a sentence. added letters this order g --> g̃, h --> ḥ, j --> ĵ, c --> ', v --> "
Here, Crash appeared it
Please take a look at the attached video file(Android14_Keyman18.0.162Alpha.mp4). Thank you.
https://github.com/user-attachments/assets/a4067af7-8582-4b67-bc40-e6f88b1b2671

@jahorton
Copy link
Contributor

jahorton commented Jan 8, 2025 •
edited
Loading

Here's the source of the null:

keyman/web/src/engine/predictive-text/worker-thread/src/main/correction/context-tracker.ts

Lines 426 to 432 in d702553

const transformDistIndex = i - (lastMatch + 1);
const tokenDistribution = transformSequenceDistribution.map((entry) => {
return {
sample: entry.sample[transformDistIndex],
p: entry.p
};
});

This block is within a loop:

keyman/web/src/engine/predictive-text/worker-thread/src/main/correction/context-tracker.ts

Lines 416 to 418 in d702553

// Now to update the end of the context window.
for(let i = lastMatch+1; i < editPath.length; i++) {
const isLastToken = i == editPath.length - 1;

It's iterating over three items when following the repro at the top, with edit path [insert, insert, substitution], but the transformSequenceDistribution here has only one entry. I'd say that this is where the bug is. Among other things, that should run [substitution, insert, insert]

Note: this is a fun consequence of '-character handling and default word-breaking. g̃ḥĵ' is a single token when pre-caret, but once the " is present, it gets split off into three tokens: [g̃ḥĵ, ', "].

@ermshiperete
Copy link
Contributor

No wonder I'm not able to reproduce if I use the wrong test file. Since this is predictive-text related it has to be tested on a page that shows predictions 🤦 . For example "Prediction - robust testing".

@ermshiperete ermshiperete assigned jahorton and unassigned ermshiperete Jan 8, 2025
jahorton pushed a commit that referenced this issue Jan 9, 2025
fix(web): better bad-context match handling due to tokenization shifts
064d753 
Fixes: #12494.
@jahorton jahorton linked a pull request Jan 9, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jahorton jahorton

Labels
bug web/
Projects
Keyman
Status: In Progress
Milestone
A18S19
5 participants
@ermshiperete @mcdurdin @darcywong00 @dinakaranr @jahorton