keymanapp / keymanweb.com

https://keymanweb.com/ source
2 stars 2 forks source link

bug: TypeError: Cannot read properties of null (reading 'KI') #96

Closed mcdurdin closed 9 months ago

mcdurdin commented 9 months ago

URL: https://keymanweb.com/?version=17.0.228#hy,Keyboard_armenian

instrument.ts:129 TypeError: Cannot read properties of null (reading 'KI')
    at t.getKeyboardForStub (stubAndKeyboardCache.ts:56:34)
    at t.getKeyboard (keymanEngine.ts:443:55)
    at changeKeyboard (kmwlive.cd4118c148650fcffddd04accefefda1.js:420:28)
    at kmwlive.cd4118c148650fcffddd04accefefda1.js:263:81
    at i.callEvent (legacyEventEmitter.ts:142:16)
    at t.<anonymous> (keymanEngine.ts:139:28)
    at W.emit (index.js:181:35)
    at t.<anonymous> (contextManagerBase.ts:277:12)
    at g (tslib.js:142:27)
    at Object.next (tslib.js:123:57)
    at o (tslib.js:113:62)
jahorton commented 9 months ago

Found the cause.

https://github.com/keymanapp/keymanweb.com/blob/544f9689975818a44ff750feb690a16676174605/cdn/dev/js/kmwlive.js#L263

The important part:

function(p){if(!pageLoading) changeKeyboard(p['internalName'],p['languageCode'],p);}

Note the implicit assumption: that p is not null.

The other contributor:

https://github.com/keymanapp/keyman/blob/8b02e79e043feee710bc235330dd51f857738cad/web/src/engine/main/src/contextManagerBase.ts#L272-L278

Note that there is no check against replacing the initially-empty (null) active keyboard with... another null. As a result, when loading keymanweb.com on a desktop form-factor device, which doesn't specify a default keyboard, the event is raised with a null object.

The error does not happen on mobile because we force a default selection on mobile. Keyboard stubs are ready by this point; mobile picks one, while desktop does not. That's the difference.

So, there are two ways this can be fixed:

  1. Prevent redundant null -> null keyboard-change events.
    • It's not like we could even be updating a keyboard going from null -> null - unlike with non-null change-to-same requests, which could be indicative of resource updates when embedded in mobile app webviews.
  2. Add a null guard to kmwlive.js's handler of the event.

Either should suffice.

mcdurdin commented 9 months ago

Either should suffice.

Good analysis. Let's do both of these -- defence in depth.

jahorton commented 9 months ago

Hold a second... it's not so much the null guard here as much as in the method it calls. Same idea, though.

https://github.com/keymanapp/keymanweb.com/blob/544f9689975818a44ff750feb690a16676174605/cdn/dev/js/kmwlive.js#L420

Looks like the actual problem arises when trying to get the keyboard object when there is no stub. The error does arise within KMW code. But yeah, fix in both spots - can do.