1996113 - ShortcutUtils should be robust when a key element lacks a key attribute

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Stephen Thompson [:sthompson]]

In cases like bug 1994969 or bug 1994716 comment 11 we have a stack trace like:

Uncaught TypeError: can't access property "toUpperCase", keyAttribute is null
    getKeyString resource://gre/modules/ShortcutUtils.sys.mjs:143
    prettifyShortcut resource://gre/modules/ShortcutUtils.sys.mjs:48
    getText chrome://browser/content/browser.js:3298
    init chrome://browser/content/tabbrowser/tabs.js:187
    init chrome://browser/content/tabbrowser/tabbrowser.js:202
    onDOMContentLoaded chrome://browser/content/browser-init.js:156

ShortcutUtils.getModifierString is robust if there is no modifier string (returns ""). ShortcutUtils.getKeyString is robust if there is no keycode string. However, ShortcutUtils.getKeyString will throw if there is no key string specified.

I imagine every <key> element is supposed to have at least a "key" attribute, so this may be a degenerate case, but it seems worth patching. The result from ShortcutUtils.prettifyShortcut will not be useful if there was no "key" attribute specified, but at least it will not throw.

Comment 1

[Stephen Thompson [:sthompson]]

Attachment: Bug 1996113 - ShortcutUtils should not throw if key attribute is missing r=Gijs!

This patch ensures that ShortcutUtils.prettifyShortcut will return a string instead of throwing if the supplied <key> element does not have a "key" attribute.

If a <key> doesn't have the "key" attribute, there is probably a more fundamental problem. However, it would be better if ShortcutUtils didn't make things worse.

I included a console warning to leave some bread crumbs to help anyone who has to debug any scenarios with missing shortcut data.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/41aeb796a645
https://hg.mozilla.org/integration/autoland/rev/897cefe56cb7
ShortcutUtils should not throw if key attribute is missing r=Gijs

Comment 3

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/897cefe56cb7

Comment 4

[Stephen Thompson [:sthompson]]

Attachment: Bug 1996113 - ShortcutUtils should not throw if key attribute is missing

This patch ensures that ShortcutUtils.prettifyShortcut will return a string instead of throwing if the supplied <key> element does not have a "key" attribute.

If a <key> doesn't have the "key" attribute, there is probably a more fundamental problem. However, it would be better if ShortcutUtils didn't make things worse.

I included a console warning to leave some bread crumbs to help anyone who has to debug any scenarios with missing shortcut data.

Additionally, if a pretty shortcut couldn't be computed, the DynamicShortcutTooltip won't cache that fact. Hopefully, the next time it runs, the necessary data might be present.

Note that gNavigatorBundle.getFormattedString can still throw if a string is not present in the strings bundle, so there are still ways for DynamicShortcutTooltip.getText to throw.

Original Revision: https://phabricator.services.mozilla.com/D269862

Comment 5

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: Low/Medium. There were a small number of reports after the 144 release of Firefox windows failing to start up in a usable state. The stack traces showed an unhandled error during browser initialization, and this patch was designed to mitigate that problem without necessarily addressing the root cause. We'd like to get this into 145 based on the assumption that the root cause is related to Firefox updates in some way.
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: We don't understand the root cause of the problem, so we've performed manual testing by manually removing the tab-new-shortcut Fluent string from browserSets.ftl. When this string was missing, it could cause an unhandled error during Firefox startup. With this patch in place, Firefox should start up normally. There will be a warning emitted in the browser console and the new tab button's tooltip will be missing the text that describes the keyboard shortcut, but the browser will otherwise work correctly.
• Risk associated with taking this patch: medium
• Explanation of risk level: This is generally a low-risk change to avoid throwing an Error in certain cases and instead return an empty string. Since we know that this code lives on a critical path during browser startup, I would bump up the risk to medium.
• String changes made/needed: No
• Is Android affected?: no

Comment 6

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/e96722498b5d
https://hg.mozilla.org/releases/mozilla-beta/rev/954f2a9b5fc3