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

RESOLVED FIXED in Firefox 62

Status

enhancement
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

3 Branch
mozilla62
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

a year ago
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 hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
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.
Attachment #8974231 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee

Comment 3

a year ago
mozreview-review-reply
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.
Assignee

Comment 4

a year ago
mozreview-review-reply
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...
Assignee

Comment 5

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

Comment 6

a year ago
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4dae88293505
https://hg.mozilla.org/mozilla-central/rev/5c5c9cc183a0
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.