1460092 - Add ESLint rule to prevent new usage of XPCOMUtils.generateQI and JS-implemented QueryInterface methods

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Kris Maglione [:kmag]]

Bug 1456035 added a ChromeUtils.generateQI helper, which is much more efficient than JS-implemented QueryInterface methods. XPCOMUtils.generateQI is currently an alias for that method, and should be removed in the near future.

In the mean time, we should add an ESLint rule to prevent people from manually implementing QueryInterface functions, or using XPCOMUtils.generateQI.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1460092: Add ESLint rule to enforce use of ChromeUtils.generateQI.

Also fixes existing code which fails the rule.

Review commit: https://reviewboard.mozilla.org/r/242532/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242532/

Comment 2

[:Gijs (he/him)]

Comment on attachment 8974231 [details]
Bug 1460092: Add ESLint rule to enforce use of ChromeUtils.generateQI.

https://reviewboard.mozilla.org/r/242532/#review248434

Thanks!

::: devtools/server/actors/csscoverage.js:106
(Diff revision 1)
>      this._running = true;
>      this._tooManyUnused = false;
>  
>      this._progressListener = {
> -      QueryInterface: XPCOMUtils.generateQI([ Ci.nsIWebProgressListener,
> +      QueryInterface: ChromeUtils.generateQI([ Ci.nsIWebProgressListener,
>                                                Ci.nsISupportsWeakReference ]),

Nit: alignment.

::: toolkit/components/passwordmgr/test/mochitest/test_prompt_promptAuth_proxy.html:83
(Diff revision 1)
>  var resolveCallback = SpecialPowers.wrapCallbackObject({
>    QueryInterface(iid) {

Given that this doesn't work with SpecialPowers, I wonder if it makes sense to disable the rule for all mochitest-plain tests. I believe this is possible by adding it at the bottom of https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/mochitest-test.js ? That might save a number of `// eslint-disable` statements, and/or confusion for future consumers. I don't much care either way, but figured I'd flag this up.

::: toolkit/content/widgets/editor.xml:28
(Diff revision 1)
>        </constructor>
>        <destructor/>
>  
>        <field name="_editorContentListener">
>          <![CDATA[
> +          /* eslint-disable mozilla/use-chromeutils-generateqi */

For the XBL binding, are we not able to rely on ChromeUtils being present, or something? Seems a bit odd... we should only be using these in privileged scopes, so it should be available, right?

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/use-chromeutils-generateqi.js:59
(Diff revision 1)
> +        if (node.callee.type !== "MemberExpression") {
> +          return;
> +        }

I think you don't need this given you're calling `isMemberExpression` afterwards, and that does the same check.

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8974231 [details]
Bug 1460092: Add ESLint rule to enforce use of ChromeUtils.generateQI.

https://reviewboard.mozilla.org/r/242532/#review248434

> Given that this doesn't work with SpecialPowers, I wonder if it makes sense to disable the rule for all mochitest-plain tests. I believe this is possible by adding it at the bottom of https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/mochitest-test.js ? That might save a number of `// eslint-disable` statements, and/or confusion for future consumers. I don't much care either way, but figured I'd flag this up.

I thought about it, but honestly, I hate that we do things like this in mochitests at all, so I decided to stick with the approach that adds more friction to it.

> For the XBL binding, are we not able to rely on ChromeUtils being present, or something? Seems a bit odd... we should only be using these in privileged scopes, so it should be available, right?

No, it should work in XBL bindings. But `eslint --fix` does not work in XBL files, and XBL is going away anyway, so I decided o just stick with the simpler approach for now.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8974231 [details]
Bug 1460092: Add ESLint rule to enforce use of ChromeUtils.generateQI.

https://reviewboard.mozilla.org/r/242532/#review248434

> No, it should work in XBL bindings. But `eslint --fix` does not work in XBL files, and XBL is going away anyway, so I decided o just stick with the simpler approach for now.

I went ahead and manually converted these, anyway. It's been nagging at me that I didn't do it in the first place...

Comment 5

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4dae882935055cd75a80139b0e17e232072b763c
Bug 1460092: Add ESLint rule to enforce use of ChromeUtils.generateQI. r=Gijs

Comment 6

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c5c9cc183a0a4cd14a4a16aab4ec228adbffb16
Bug 1460092: Follow-up: Fix eslint test failure caused by added comment. r=bustage CLOSED TREE

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/4dae88293505
https://hg.mozilla.org/mozilla-central/rev/5c5c9cc183a0