bug(web): poor cookie default can cause engine-init error

[jahorton]

This comes out of https://community.software.sil.org/t/keman-keyboard-is-not-rendering-on-chorme-and-mozilla-browsers/9783/6.

When attempting to reproduce the user's error on their site, I saw a rather... paradoxical-looking bug occur. I'll copy down the relevant parts of their source below.

Starting at line 534:

<script>
    keyman.init({
        attachType: 'auto'
    });

    keyman.addKeyboards({
        name: 'SHIFT FOR EMOJIS',
        id: 'sil_arabic_phonetic',
        filename: '//casadoriente.com/cdn/shop/t/40/assets/sil_arabic_phonetic.js?v=90679577857814663911738067428',
        version: '1.0',
        language: [{
            name: 'English to Arabic',
            id: 'arabic',
        }]
    });

    keyman.addKeyboards({
        name: 'SHIFT FOR EMOJIS',
        id: 'basic_kbda2',
        filename: '//casadoriente.com/cdn/shop/t/40/assets/basic_kbda2.js?v=155236329126653460221738067436',
        version: '1.0',
        language: [{
            name: 'Arabic',
            id: 'basic',
        }]

    });
</script>

OK, that looks fine. Keyboard stubs can be registered before engine init, after all - especially with local information, and especially when the keyboard's .js file is locally hosted. The addKeyboards methods have built-in checks that will defer further processing until the engine has sufficiently initialized; they're safe to call in this manner.

Starting at line 1970 or so (Chrome prettified this):

<script type="text/javascript">
    function SetupDocument() {
        keyman.init({
            root: './',
            ui: 'toggle',
            resources: './'
        }).then(function() {
            // loadKeyboards(); // Load keyboards AFTER Keyman initializes
            keyman.setKeyboardForControl($('input.pplr_text.pplr_monogram.keymanweb-font')[0], 'sil_arabic_phonetic', 'arabic');
            keyman.setActiveKeyboard('sil_arabic_phonetic', 'arabic');
        }).catch(function(err) {
            console.error("Keyman initialization error:", err);
        });
    }
    $(document).ready(function() {
        window.addEventListener('load', SetupDocument);
    });
</script>

OK, that looks to be fine - sil_arabic_phonetic has a stub added way earlier in the line-534 script tag's code. They're automatically deferred, yeah, but that should be fulfilled before init returns , so we should be fine here... right?

The thing is... I did see one instance where a rather strange error occurred upon investigation. I couldn't get it to repro, but I saw an error with the following details:

• “No matching stub has been registered” was the error message.
• keyman.setKeyboardForControl (from the second block) appeared in the stack trace as the triggering call, within the later init’s then clause.

So... somehow sil_arabic_phonetic wasn't registered by line 1978? But... how? Did the later block's then somehow get called before the deferred stub-registering calls got completed? They should be synchronous after the deferment wears off!

[jahorton]

My initial suspicion was focused on the following block:

keyman/web/src/app/browser/src/keymanEngine.ts

Lines 177 to 182 in 263f918

	// Deferred keyboard loading + shortcutting if a different init call on the engine has
	// already fully resolved.
	if(this.config.deferForInitialization.isResolved) {
	// abort! Maybe throw an error, too.
	return Promise.resolve();
	}

When that Promise is resolved, registered-stub deferment ends. But, what if there are pending then() clauses based on that Promise still queued up for completion? That's something that's technically not guaranteed here.

It may be wiser to replace that with...

 if(this.config.deferForInitialization.isResolved) { 
   // abort!  Maybe throw an error, too. 
   return this.config.deferForInitialization.corePromise; 
 } 

Per MDN's documentation:

All handlers attached to the same promise object are always called in the order they were added.

Adding a new handler here will ensure that later, secondary init calls will wait for any remaining init-deferred, still-queued to resolve before continuing.

That said... we'd still be queuing up a new Promise resolution... and since the deferForInitialization Promise has resolved, those deferred calls should have already been resolved and thus on the microtask queue anyway. It really shouldn't make a difference how we wait in this section, so long as we await something.

---

So, next I turned my attention here:

keyman/web/src/app/browser/src/keymanEngine.ts

Lines 216 to 229 in 263f918

	// Automatically performs related handler setup & maintains references
	// needed for related cleanup / shutdown.
	this.pageIntegration = new PageIntegrationHandlers(window, this);
	this.config.finalizeInit();
	if(this.ui) {
	this.ui.initialize();
	this.legacyAPIEvents.callEvent('loaduserinterface', {});
	}
	this._initialized = 2;
	// Let any deferred, pre-init stubs complete registration
	await Promise.resolve();

Line 219: this.config.finalizeInit() - is what fulfills the deferForInitialization Promise. We don't actually await it here, though. That's done by line 229's await, though indirectly, using the conventional "fulfilled Promise resolutions all resolve in order" logic. Line 229's Promise.resolve() resolved later, so all deferred registrations should still be queued before it.

So... huh. Is there a subtle detail I'm missing here?

[jahorton]

With the user's site, it appears that there needs to be a clean cache in regard to the site. The first successful load (with Keyman Engine for Web activating) seems to generate this case; refreshing or revisiting the page afterward doesn't trigger the issue for me.

It does appear to reproduce consistently, but only on the first visit each time after I use the "Delete browsing data" function or similar. I did this in Chrome with the following options set:

• Cookies and other site data
• Cached images and files

What I see:

contextManager.ts:542 Error: No matching stub has been registered.
    at mt.prepareKeyboardForActivation (contextManagerBase.ts:312:15)
    at mt.<anonymous> (contextManagerBase.ts:237:37)
    at Generator.next (<anonymous>)
    at keymanweb.js:1:1395
    at new Promise (<anonymous>)
    at Z (keymanweb.js:1:1215)
    at mt.activateKeyboard (contextManagerBase.ts:227:116)
    at mt.<anonymous> (contextManager.ts:501:32)
    at Generator.next (<anonymous>)
    at keymanweb.js:1:1395
(anonymous) @ contextManager.ts:542
c @ keymanweb.js:1
Promise.then
l @ keymanweb.js:1
(anonymous) @ keymanweb.js:1
Z @ keymanweb.js:1
activateKeyboard @ contextManager.ts:488
restoreSavedKeyboard @ contextManager.ts:821
(anonymous) @ keymanEngine.ts:231
s @ keymanweb.js:1
Promise.then
l @ keymanweb.js:1
s @ keymanweb.js:1
Promise.then
l @ keymanweb.js:1
s @ keymanweb.js:1
Promise.then
l @ keymanweb.js:1
(anonymous) @ keymanweb.js:1
Z @ keymanweb.js:1
init @ keymanEngine.ts:154
(anonymous) @ (index):537

contextManagerBase.ts:312 Uncaught (in promise) Error: No matching stub has been registered.
    at mt.prepareKeyboardForActivation (contextManagerBase.ts:312:15)
    at mt.<anonymous> (contextManagerBase.ts:237:37)
    at Generator.next (<anonymous>)
    at keymanweb.js:1:1395
    at new Promise (<anonymous>)
    at Z (keymanweb.js:1:1215)
    at mt.activateKeyboard (contextManagerBase.ts:227:116)
    at mt.<anonymous> (contextManager.ts:501:32)
    at Generator.next (<anonymous>)
    at keymanweb.js:1:1395
prepareKeyboardForActivation	@	contextManagerBase.ts:312
(anonymous)	@	contextManagerBase.ts:237
(anonymous)	@	keymanweb.js:1
Z	@	keymanweb.js:1
activateKeyboard	@	contextManagerBase.ts:227
(anonymous)	@	contextManager.ts:501
(anonymous)	@	keymanweb.js:1
Z	@	keymanweb.js:1
activateKeyboard	@	contextManager.ts:488
restoreSavedKeyboard	@	contextManager.ts:821
(anonymous)	@	keymanEngine.ts:231
s	@	keymanweb.js:1
Promise.then		
l	@	keymanweb.js:1
s	@	keymanweb.js:1
Promise.then		
l	@	keymanweb.js:1
s	@	keymanweb.js:1
Promise.then		
l	@	keymanweb.js:1
(anonymous)	@	keymanweb.js:1
Z	@	keymanweb.js:1
init	@	keymanEngine.ts:154
(anonymous)	@	(index):537

Line 537 of the main page is the first keyman.init call, positioned before the page's keyman.addKeyboards() calls. (See the "starting at line 534" block above.)

[jahorton]

Using that, via breakpoint I am able to see two things:

1. The site-author's intended stubs have successfully been registered at the time of the error!
2. the engine is actually trying (and failing) to load... Keyboard_us?

The engine actually does attempt to load one of the page's intended keyboards first. I confirmed that by setting a breakpoint at the first line of ContextManagerBase.activateKeyboard. It's the second call to it that has Keyboard_us.

... and I think I see what's happening.

Within app/browser's engine init:

keyman/web/src/app/browser/src/keymanEngine.ts

Line 159 in 7ab6887

public async init(options: Required<BrowserInitOptionSpec>) {

keyman/web/src/app/browser/src/keymanEngine.ts

Lines 204 to 206 in 7ab6887

	// Capture the saved-keyboard string now, before we load any keyboards/stubs
	// or do anything that would mutate the value.
	const savedKeyboardStr = this.contextManager.getSavedKeyboardRaw();

Note that we aim to preserve whatever the user's last keyboard setting was upon repeat visits to the page.

keyman/web/src/app/browser/src/keymanEngine.ts

Lines 228 to 236 in 7ab6887

	// Let any deferred, pre-init stubs complete registration
	await Promise.resolve();
	// Attempt to restore the user's last-used keyboard from their previous session.
	//
	// Note: any cloud stubs will probably not be available yet.
	// If we tracked cloud requests and awaited a Promise.all on pending queries,
	// we could handle that too.
	this.contextManager.restoreSavedKeyboard(savedKeyboardStr);

At the end of init, we allow stubs to register (the first of which is generally auto-activated, depending on init options), after which we look to enforce their prior keyboard setting.

The issue: what happens when there is no prior keyboard setting?

keyman/web/src/app/browser/src/contextManager.ts

Lines 753 to 769 in 7ab6887

	/**
	* Gets the 'saved keyboard' cookie value for the last keyboard used in the
	* iser's previous session.
	**/
	getSavedKeyboardRaw(): string {
	const cookie = new CookieSerializer<KeyboardCookie>('KeymanWeb_Keyboard');
	var v = cookie.load(decodeURIComponent);
	if(typeof(v.current) != 'string') {
	return 'Keyboard_us:en';
	} else if(v.current == 'Keyboard_us:eng') {
	// 16.0 used the :eng variant!
	return 'Keyboard_us:en';
	} else {
	return v.current;
	}
	}

Upon first visit, v.current should be empty... yielding Keyboard_us:en. This page does not include a link for that keyboard, nor for its stub, and this triggers the error. Note: as a result, the init() call does not reach the point at which it resolves its Promise, meaning this can cause significant issue for site authors! The getSavedKeyboardRaw method was added in 17.

Contrast with the longer-standing getSavedKeyboard:

keyman/web/src/app/browser/src/contextManager.ts

Line 778 in 7ab6887

getSavedKeyboard(): string {

keyman/web/src/app/browser/src/contextManager.ts

Lines 800 to 807 in 7ab6887

	// Otherwise use the first keyboard stub
	if(stubs.length > 0) {
	return stubs[0]['KI']+':'+stubs[0]['KLC'];
	}
	// Or US English if no stubs loaded (should never happen)
	return 'Keyboard_us:en';
	}

The final default is likely what was being modeled by the error-triggering path, but now that it's known to have adverse effects, we should nix that.

[mcdurdin]

Good analysis.

Reminds me that we should be deprecating KMW's responsibility for storing state and shifting that responsibility to a completely separate module...