1569572 - Empty shortcut in toolbox.properties completely breaks DevTools

Status: RESOLVED FIXED

(DevTools :: General, task, P2)

Description

[Francesco Lodolo [:flod]]

Bulgarian build around last week-end were completely broken (DevTools open up blank), because of two empty shortcuts
https://hg.mozilla.org/l10n-central/bg/rev/c6c2bc1dc99d59ae0032470652106ff45cb4b2db

Error reported in Console

Exception while opening the toolbox TypeError: str is null TypeError: "str is null"
    parseElectronKey resource://devtools/client/shared/key-shortcuts.js:97
    _getPickerTooltip resource://devtools/client/framework/toolbox.js:1821
    _buildPickerButton resource://devtools/client/framework/toolbox.js:1802
    _buildButtons resource://devtools/client/framework/toolbox.js:1675
    open resource://devtools/client/framework/toolbox.js:710
    open resource://devtools/client/framework/toolbox.js:780
    createToolbox resource://devtools/client/framework/devtools.js:633
    showToolbox resource://devtools/client/framework/devtools.js:527
    selectToolCommand resource://devtools/client/framework/devtools-browser.js:303
    onKeyShortcut resource://devtools/client/framework/devtools-browser.js:339
    onKey resource:///modules/DevToolsStartup.jsm:759
    xulKey resource:///modules/DevToolsStartup.jsm:683
    createKey resource:///modules/DevToolsStartup.jsm:793
    attachKeys resource:///modules/DevToolsStartup.jsm:683
    hookKeyShortcuts resource:///modules/DevToolsStartup.jsm:661
    hookWindow resource:///modules/DevToolsStartup.jsm:451
    onWindowReady resource:///modules/DevToolsStartup.jsm:407
    _delayedStartup chrome://browser/content/browser.js:2095
    onLoad chrome://browser/content/browser.js:1843
    EventHandlerNonNull* chrome://browser/content/browser.xhtml:112
toolbox.js:782:17
    open resource://devtools/client/framework/toolbox.js:782
    open resource://devtools/client/framework/toolbox.js:781
    createToolbox resource://devtools/client/framework/devtools.js:633
    showToolbox resource://devtools/client/framework/devtools.js:527
    selectToolCommand resource://devtools/client/framework/devtools-browser.js:303
    onKeyShortcut resource://devtools/client/framework/devtools-browser.js:339
    onKey resource:///modules/DevToolsStartup.jsm:759
    xulKey resource:///modules/DevToolsStartup.jsm:683
    createKey resource:///modules/DevToolsStartup.jsm:793
    attachKeys resource:///modules/DevToolsStartup.jsm:683
    hookKeyShortcuts resource:///modules/DevToolsStartup.jsm:661
    hookWindow resource:///modules/DevToolsStartup.jsm:451
    onWindowReady resource:///modules/DevToolsStartup.jsm:407
    _delayedStartup chrome://browser/content/browser.js:2095
    onLoad chrome://browser/content/browser.js:1843
    <anonymous> chrome://browser/content/browser.xhtml:112

TypeError: this.frameButton is undefined toolbox.js:2979:24

You can download the build from here, or likely try changing the local en-US strings for testing
http://ftp.mozilla.org/pub/firefox/nightly/2019/07/2019-07-28-21-49-40-mozilla-central-l10n/

I assumed this was fixed by bug 1464132, but it doesn't seem to be the case.

Comment 1

[Julian Descottes [:jdescottes]]

I assumed this was fixed by bug 1464132, but it doesn't seem to be the case.

key shortcuts are a special case of localized strings for devtools. They need to be localized but if they don't follow the expected format, our current code throws.
We can swallow the exception here and simply skip the shortcut, but it will make this kind of localization issues harder to detect.

Comment 2

[Julian Descottes [:jdescottes]]

I think the options here are:

• either swallow the exception and skip the shortcut
• or keep throwing, but with a better error message (eg Localized DevTools shortcut toolbox.elementPicker.key could not be parsed (value: ""). Check the localization for this shortcut is a valid DevTools shortcut).

Maybe there is a third option? Is there any continuous integration running for localized builds where we can add a test that checks devtools shortcuts?

Comment 3

[Francesco Lodolo [:flod]]

• either swallow the exception and skip the shortcut

Skip and log sounds like a good plan.

Maybe there is a third option? Is there any continuous integration running for localized builds where we can add a test that checks devtools shortcuts?

There's compare-locales, but empty strings are valid in a DTD world, and even beyond that (I've seen secondary shortcuts kept empty in English). We couldn't safely throw away those localized strings, as we would do with other errors.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1569572 - Do not throw in DevTools key-shortcuts.js when called with a null object

This situation can happen if a locale does not set a value for a localized devtools shortcul, ie writes
toolbox.elementPicker.key=
(with nothing after the = sign)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9aa60b80b9b
Do not throw in DevTools key-shortcuts.js when called with a null object r=nchevobbe

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e9aa60b80b9b