1142432 - KeyboardHelper: kh_withSettings() might be ineffective in withApps()

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::System::Input Mgmt, defect)

Description

[John Lu [:mnjul][Please NI or I'll miss]]

The kh_withSettings() in withApps() inside KeyboardHelper.getLayouts() [1] is called synchronously after the big apps.reduce(). However, inside apps.reduce()'s callback function (which is also synchronous) we're already retrieving mozSettings-derived properties [2]: |KeyboardLayout.prototype.default| and |KeyboardLayout.prototype.enabled| getters [3] already assume |currentSettings| has meaningful values.

I traced the code flows of entries to KeyboardHelper from KeyboardManager; they include KH.init() and KH.watchLayouts(). The former will eventually populate |currentSettings|, and the latter will call KH.getLayouts() [*]. In the flows, there seems to be no guarantee that at [2] the settings have already been populated to |currentSettings| (which is the purpose of kh_withSettings()).

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/js/keyboard_helper.js#L709

[2] https://github.com/mozilla-b2g/gaia/blob/master/shared/js/keyboard_helper.js#L679-L685

[3] https://github.com/mozilla-b2g/gaia/blob/master/shared/js/keyboard_helper.js#L395-L415

[*] The entries are both in KeyboardManager.init(). KH.init() is immediately invoked as keyboard_helper.js is loaded/parsed/executed, KH.watchLayouts() is invoked when LazyLoader has done its job.

I'm not observing actual behavioral bugs from this (I just discovered this by studying the codes). I believe kh_withSettings() should be employed *before* calling KH.getApps(). Reason: aside from the scenario in the first paragraph, the following is also a possible race condition:

1. |currentSettings| not ready yet.
2. getApps() gets app list from InputAppList.
3. getApps() calls cleanupSettings().
4. cleanupSettings() does nothing, as |currentSettings| is empty.
5. getApps() finishes and getLayouts() continues to execute.
6. |currentSettings| is now populated by the callbacks set in kh_getSettings().
7. ...and oops, cleanupSettings() has missed its opportunity to do its job.

I do think if we are to fix this bug, we should convert the messy callbacks to Promises. But that's probably not easy, I gather.

Comment 1

[BMO Automation]

Firefox OS is not being worked on