1703953 - Prevent instanceof uses against DOM interfaces and prefer .isInstance()

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

So I now have a working rule, but not sure how can I selectively run it on privileged scripts. I see use-ownerGlobal (which also is for chrome-context only) is manually turned off on a few directories, but instanceof is used in many many more files than ownerGlobal, so I need to detect chrome context automatically and reliably. Is there an existing way to do that?

Comment 2

[Dave Townsend [:mossop]]

Standard8 is probably the best person to answer such questions.

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1703953 - Part 1: Implement mozilla/use-isinstance rule r=gijs,mossop

Comment 4

[Mark Banner (:standard8)]

(In reply to Kagami :saschanaz from comment #1)

So I now have a working rule, but not sure how can I selectively run it on privileged scripts. I see use-ownerGlobal (which also is for chrome-context only) is manually turned off on a few directories, but instanceof is used in many many more files than ownerGlobal, so I need to detect chrome context automatically and reliably. Is there an existing way to do that?

Unfortunately we don't really have a good proxy for what's privileged or not. We know that jsms are definitely privileged, and we can filter on those. We could probably skip it on tests as well.

For the rest of a files, we don't have a good way to tell, some are in-content pages, some are privileged code. Unless we could just assume everything is privileged?

Comment 5

[Dave Townsend [:mossop]]

Why does it need to be restricted to chrome contexts?

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

We could probably skip it on tests as well.

There are many tests that runs on chrome context, so no 😬

Why does it need to be restricted to chrome contexts?

Because .isInstance() is only available in chrome context. (TC39 has been reluctant to standardize it.)

Comment 7

[Mark Banner (:standard8)]

Gijs has just reminded me that we do have the browser-window environment.

Unfortunately environments can't specify rules, and we can't define configurations in the same way as we do for environments. Ideally we'd make sure these files were split with directory names or something, but that is likely to be a bigger job.

What might get us to a place where we can at least have a curated list is:

• Create a new configuration (see tools/lint/eslint/eslint-plugin-mozilla/lib/configs/) which enables the new rule.
• Enable the configuration for all jsms and every file that is in the browser-window environment.
  • I suspect we'd have to manually curate and update the list of browser-window environment,
• Potentially move the privileged environment from the recommended config to the new configuration, and also ensure it is enabled for jsms.
  • Doing this additional step would help us find other files that are privileged but not currently jsms nor browser-window.

Comment 8

[Mark Banner (:standard8)]

(In reply to Kagami :saschanaz from comment #6)

We could probably skip it on tests as well.

There are many tests that runs on chrome context, so no 😬

Is this rule for a transition phase? Is there a point where we could get to where using instanceof in chrome code would simply throw an error and that is all handled by JS?

I'm just wondering if it might be better/easier to handle this from a js perspective rather than linting.

Side note: enabling on tests would be easy, though we'd have to assume that all files in the test directory are chrome privileged.

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

What might get us to a place where we can at least have a curated list is:

Hmm, I could cause crash for every chrome context instanceof call and enable the config for all the failing files.

Is there a point where we could get to where using instanceof in chrome code would simply throw an error and that is all handled by JS?

That should be possible, but doing so still requires converting existing expressions or otherwise so many tests will start failing that fixing them by hand will be impractical.

Comment 10

[Mark Banner (:standard8)]

(In reply to Kagami :saschanaz from comment #9)

Is there a point where we could get to where using instanceof in chrome code would simply throw an error and that is all handled by JS?

That should be possible, but doing so still requires converting existing expressions or otherwise so many tests will start failing that fixing them by hand will be impractical.

As a starting point, we could at least enable the rule as a warning for jsms - similar to what we've done for the OS.File removal. That would cover a lot of code and start helping to get it changed.

Comment 11

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1703953 - Part 2: Apply mozilla/use-isInstance rules for .jsm files r=gijs,mossop

Depends on D111354

Comment 12

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1703953 - Part 3: Apply mozilla/use-isInstance to devtools r=#devtools-reviewers

Depends on D141785

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eaf6d4c353be
Part 1: Implement mozilla/use-isinstance rule r=Gijs,Standard8
https://hg.mozilla.org/integration/autoland/rev/8151244bda18
Part 2: Apply mozilla/use-isInstance rules for .jsm files r=webdriver-reviewers,pip-reviewers,mhowell,Gijs,whimboo
https://hg.mozilla.org/integration/autoland/rev/871a1fac289e
Part 3: Apply mozilla/use-isInstance to devtools r=julienw

Comment 14

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing multiple failures. CLOSED TREE

Backout link: https://hg.mozilla.org/integration/autoland/rev/d3e9970cf8e618eb403e4d405900f0c909627762

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=871a1fac289e86625d14b5a5f68f7a9e3519068d&selectedTaskRun=RTxfaKP9SxOoirggPVAkQw.0

Link to failure logs:
https://treeherder.mozilla.org/logviewer?job_id=372691970&repo=autoland&lineNumber=94
https://treeherder.mozilla.org/logviewer?job_id=372691913&repo=autoland&lineNumber=5282
https://treeherder.mozilla.org/logviewer?job_id=372691986&repo=autoland&lineNumber=4920
https://treeherder.mozilla.org/logviewer?job_id=372691718&repo=autoland&lineNumber=1836
https://treeherder.mozilla.org/logviewer?job_id=372692207&repo=autoland&lineNumber=12744

Comment 15

[Cristian Tuns]

We also got this type of failures caused by this bug: https://treeherder.mozilla.org/logviewer?job_id=372699045&repo=autoland

Comment 16

[Kagami Rosylight [:saschanaz] (they/them)]

Okay, so I found an unexpected result.

Modifying getFromWindow() in toolkit/components/extensions/WebNavigationFrames.jsm as below causes error:

  getFromWindow(target) {
    if (target instanceof Window) {
      console.debug(`isInstance: ${Window.isInstance(target)}`);
      console.debug(`instanceof: ${target instanceof Window}`);
    }
    if (Window.isInstance(target)) {
      return getFrameId(BrowsingContext.getFromWindow(target));
    }
    return -1;
  },

With ./mach mochitest toolkit/components/extensions/test/mochitest/test_ext_contentscript_getFrameId.html, the log says:

 0:37.35 INFO Content script loaded on: https://example.org/tests/toolkit/components/extensions/test/mochitest/file_with_xorigin_frame.html
 0:37.39 GECKO(8200) console.debug: "isInstance: true"
 0:37.39 GECKO(8200) console.debug: "instanceof: true"
 0:37.39 GECKO(8200) console.debug: "isInstance: true"
 0:37.41 GECKO(8200) console.debug: "instanceof: true"
 0:37.41 GECKO(8200) console.debug: "isInstance: false"
 0:37.41 GECKO(8200) console.debug: "instanceof: true"
 0:37.41 GECKO(8200) JavaScript error: undefined, line 0: Error: Invalid argument
 0:37.41 GECKO(8200) Console message: [JavaScript Error: "Error: Invalid argument" {file: "undefined" line: 0}]

But per my understanding isInstance should be a strict superset of instanceof. Not sure what's happening here, since both should be using the same backend InterfaceHasInstance.

Hi WebIDL people, do you have any idea?

Comment 17

[Kagami Rosylight [:saschanaz] (they/them)]

Found the cause, will fix it in bug 1762965.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d046c96ef135
Part 1: Implement mozilla/use-isinstance rule r=Gijs,Standard8
https://hg.mozilla.org/integration/autoland/rev/5ba4b0a5dd34
Part 2: Apply mozilla/use-isInstance rules for .jsm files r=webdriver-reviewers,pip-reviewers,mhowell,Gijs,whimboo
https://hg.mozilla.org/integration/autoland/rev/b617178ef491
Part 3: Apply mozilla/use-isInstance to devtools r=julienw

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/d046c96ef135
https://hg.mozilla.org/mozilla-central/rev/5ba4b0a5dd34
https://hg.mozilla.org/mozilla-central/rev/b617178ef491