918057 - Make ASan try_by_default

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Phil Ringnalda (:philor)]

We currently have ASan builds as try_by_default: False, which means that nobody runs them on try unless it's after they've been backed out for bustage (both because trychooser is awful for producing the required -p all,linux64-asan, and because I apparently didn't realize it wasn't try_by_default, so trychooser doesn't mention "(not run by default)").

We expect people not to land security bustages which break ASan builds/tests, so we should be running them on try.

Comment 1

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(looping in dveditz and decoder who originally asked for ASan builds and were part of this initial setup. )

We can do default-on-try if people think its worth the big jump in load that comes with that. iirc, dveditz and decoder were happy to have ASan not-by-default, given how few people were likely to trip this, so they thought non-default-on-try was good enough. It would help to have some concrete data here, how often do you see backouts caused by ASan regressions?

separate note: trychooser should mention "not run by default", so depending on what dveditz/decoder decide here, we might spin out separate bug, or morph this bug into "fix trychooser to mention ASan not run by default"?

Comment 2

[Christian Holler (:decoder)]

I wouldn't say it doesn't make sense to do this, but I would like to wait until we have more data to answer John's question... that is, how often to we hit ASan only regressions that get backed out then. This is a very important question/statistic that we need to collect, and before we have that data, it's hard to judge if it's worth putting additional load on the releng infrastructure.

Comment 3

[Phil Ringnalda (:philor)]

Labelled "(not run by default)" or not, without a fix for bug 918058 so that someone who knows that their patch might affect ASan has a reasonable way to include it, this platform is skating out on the thin edge of meeting the visibility requirements.

What is the set of patches which might affect ASan? "Anything which touches C++, or which touches JS which uses XPConnect in any way it's not already being used"? What percentage of all patches is that?

Comment 5

[Nathan Froyd [:froydnj]]

As a datapoint, ASan builds can apparently change headers included in the build:

https://bugzilla.mozilla.org/show_bug.cgi?id=914826#c16

...which can cause the builds to fail despite having been green on a '-p all' try run.

Admittedly, this is probably a rare case.  But as a developer, I expect that '-p all' is going to run things on everything that m-i/m-c does.  Having that expectation violated was a rude awakening.

Comment 6

[Daniel Veditz [:dveditz]]

I don't think I have anything useful to add by way of needinfo?. We guessed that unexpected breakage would be seldom enough, and easy enough to spot and backout on -inbound, that we could save a few watts not running the tests. If the sheriffs are saying that breakage has been often enough to cause problems then I'd say give them what they want. machine time is cheap, people aren't.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We need to get this fixed. People who know they have risky patches push to try to check them out and then expect a green try run to mean a green m-c run.

Comment 8

[Phil Ringnalda (:philor)]

Yeah, sorry if I confused anyone into thinking this caused *sheriff* pain.

To us, it's a backout just like any other. The only pain we have to endure is listening to a developer complain about how he spent two solid days rebasing while writing a patch that touched stuff all over the tree, then rebased for 8-10 hours while waiting for try results, that being how long it regularly takes on weekdays anymore, then when he finally got his results and pushed, he was betrayed by being lied to by trychooser, which claimed that things were --all when they were not.

Comment 9

[Phil Ringnalda (:philor)]

Along with actual ASan fun, we have exactly one platform, ASan, which builds --disable-crashreporter and runs tests on such a build, causing unwary crashreporter-dependent tests to fail. So far today, I've backed out one surprise ASan failure, and one surprise --disable-crashreporter failure.

Comment 10

[Nathan Froyd [:froydnj]]

Just got bit by this again today: changes that we were green on try (-p all) induced linking failures in ASan builds.

Comment 11

[Phil Ringnalda (:philor)]

While I'll note that I've still not gotten any answer at all from anyone to my "okay, who should run it for which of their patches?" question, which leads me to believe that rather than "everyone knows what needs ASan testing" like having it not be try_by_default implies, in fact absolutely nobody knows what needs ASan testing, I'm still adding things on to my answer, which is now up to "any patch which touches C++, or JS which does XPCOM, or anything touching crashreporter, or anything which might possibly use any memory at all in any test since we're apparently perpetually right at the ragged edge of OOM in ASan tests."

Comment 13

[Nicholas Nethercote [inactive]]

I was bitten by this last month in bug 936964, i.e. I had patches backed out due to ASan failures. (Even more fun was that bz and I concluded, albeit hesitantly, that it was a false positive.)

Since then I've been consistently using |-p all,linux64-asan| instead of |-p all|.

Comment 14

[(no longer active)]

I was bitten by this in bug 940708 as well.  Can we please prioritize this?

Comment 15

[Chris AtLee [:catlee]]

Attachment: enable try by default for asan

this should just Just Work™