1129013 - Enable cross-compartment checks by default and put it on the trains

Status: RESOLVED WONTFIX

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

It is time.

Comment 1

[Terrence Cole [:terrence]]

Attachment: enable_xcompartment_checks_by_default-v0.diff

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'd like to see some measurements about how this affects benchmark scores (and not just shell tests).

Comment 3

[Terrence Cole [:terrence]]

Good thought! Let's see if it survives talos:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=98926fca2aa1

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

You're comparing to nightly, where compartment checks are already enabled.

Comment 5

[Bobby Holley (:bholley)]

What do we gain from this, exactly? Are we actually seeing many compartment mismatches these days?

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Well, the benefit to having them on by default is that compartment mismatches would no longer be security bugs.

Even if we don't enable them on release, it might be nice to enable them on beta. It seems like we might get a lot more weird add-on usage there.

But no, I don't think we see them much at all on crash-stats. Andrew was telling me last week that he was worried we don't see any, which might be a signal that something is broken in the reporting.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #6)
> Well, the benefit to having them on by default is that compartment
> mismatches would no longer be security bugs.

Well, mismatches that we assert against. I'm concerned that we'll also want to assert against them in performance-critical cases (stuff used in dom bindings for example), and then we'll need 2 kinds, and then we'll always have to decide which to use.

> Even if we don't enable them on release, it might be nice to enable them on
> beta. It seems like we might get a lot more weird add-on usage there.

This is fair, though I think beta is supposed to behave as much like release as possible.
 
> But no, I don't think we see them much at all on crash-stats. Andrew was
> telling me last week that he was worried we don't see any, which might be a
> signal that something is broken in the reporting.

Or maybe the couple of buggy addons finally got marked incompatible or something, or maybe we've just removed enough of the old codepaths that were crashing (like the stuff in nsDOMClassInfo). In general, the only addons that can create unavoidable mismatches are binary addons, of which there are fewer and fewer. Script-only addons can't create compartment mismatches if we don't have bugs.

Comment 8

[Andrew McCreight [:mccr8]]

Even before they disappeared entirely, there was just a thin smattering of these crashes, where you'd see one or two that looked like one thing, and another that looked like another.  This included things like the JS interpreter.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8558615 [details] [diff] [review]
enable_xcompartment_checks_by_default-v0.diff

Canceling this for now.

Comment 10

[Terrence Cole [:terrence]]

It seems we're happy enough with the status-quo here for now.