1890547 - [meta] eliminate inline event handlers from the browser window

Status: RESOLVED FIXED

(Firefox :: General, task)

Description

[:Gijs (he/him)]

The practice of oncommand and onclick inline event handlers in browser.xhtml (browser.xul as-was) dates back to the Netscape days.

As one might expect, in a quarter of a century some stuff has changed. This practice is anachronistic, makes linting more difficult, automated tools like searchfox less effective (as they can't find symbols in the JS-in-HTML) and stops us adding a restrictive CSP to browser windows which would help defence in depth against various types of browser exploits.

We should move away from doing this.

Discussing with :Mossop, I think for an initial pass we'll likely move listeners that are in markup that ends up attached to the main DOM immediately to a single DOMContentLoaded handler or similar.

For the things in e.g. https://searchfox.org/mozilla-central/source/browser/base/content/appmenu-viewcache.inc.xhtml that isn't immediately accessible using getElementById, we'd likely either take a more case-by-case approach or use PanelMultiView.getViewNode to get hold of the nodes to add listeners at the same time.

This isn't ideal in that it creates a bit of a giant list of listeners (which also ends up being a single point of failure), and it would be nicer if all these listeners ended up somewhere related to their individual purposes, but the size of the problem is such that to make progress this is likely the expedient way of addressing this (with follow-up bugs used to perhaps distribute some of these listeners to more topical/lazy places in the code).

Comment 1

[Frederik Braun [:freddy] (OOO until August 18th)]

If we want to do this for the sake of hardening our parent process, do we need to ensure that other windows are also cleaned up? I'm not sure what other windows might exist, but I can think of the bookmarks manager, maybe printing preview and all sorts of prompts being in the parent.

Comment 2

[:Gijs (he/him)]

(In reply to Frederik Braun [:freddy] from comment #1)

If we want to do this for the sake of hardening our parent process, do we need to ensure that other windows are also cleaned up?

Yes.

I'm not sure what other windows might exist, but I can think of the bookmarks manager, maybe printing preview and all sorts of prompts being in the parent.

Print-preview is normally tab-modal now. I don't know if we still have/support any other way of opening that. But we definitely have a few more, like the about dialog, the webrtc indicator window, etc. etc.

Anyway, the other windows I think would also be helpful and it would probably be useful to get a metabug on file for that. But I think the focus should be on the main browser window initially, both because it's large enough that it needs a dedicated burndown effort (can't easily be done in one giant patch) and because it is the main thing that also interacts with untrusted content processes (unlike the about dialog and bookmarks manager etc.).

Comment 3

[Tom Schuster (MoCo)]

Judging by our telemetry results we most likely found all inline event handlers in our own code.