Is there a good reason not to hold tests to the same standard as code? Seems like it'll make it easier for people to get into bad habits. Or is this just temporary, to get it stood up at all?

We do not currently run LSan for CPPunit tests (see bug 1026663). It just goes from being off by default in whatever we're using now for ASan to on by default in clang 3.7.

Actually, I think it makes more sense to just globally build in disabling leak checking. Individual tests can opt in by setting the environment variable. This will maintain the status quo with our current version of Clang. This is also apparently what Chromium does per bug 1063011 comment 7.

Practically speaking, anybody who is trying to enable LSan for a new test will have to set an environment variable anyways to specify a whitelist file, so adding in detect_leaks=1 to ASAN_OPTIONS should not be a large burden.

This ensures that with Clang 3.7 we have the same default behavior for ASan in anything we build that links in mozglue: no leak checking is performed. I checked that we are still doing leak checking in Mochitests. try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a92b2576959

Comment on attachment 8677007 [details] [diff] [review] Disable LSan leak checking by default in mozglue. Review of attachment 8677007 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozglue/build/AsanOptions.cpp @@ +21,5 @@ > // while the other doesn't, ASan will report false positives. > // > extern "C" MOZ_ASAN_BLACKLIST > const char* __asan_default_options() { > + return "allow_user_segv_handler=1:alloc_dealloc_mismatch=0:detect_leaks=0"; What do these options apply to? the executable or library it's statically linked to only?

(In reply to Mike Hommey [:glandium] from comment #6) > What do these options apply to? the executable or library it's statically > linked to only? I think the way it works is that ASan itself tries to call into that method, so it should just be whatever it gets linked to. I don't think it answers your question, but for the curious, the code that deals with this in ASan is here: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/asan/asan_flags.cc?view=markup

Let me formulate the question differently: how does this change not disable leak detection globally, since everything uses mozglue?

(In reply to Mike Hommey [:glandium] from comment #8) > Let me formulate the question differently: how does this change not disable > leak detection globally, since everything uses mozglue? Oh I see. If the environment variable ASAN_OPTIONS is set, as it is any place where we do leak checking today, then the values set via the env var override those set by this function.

So we rely on ASAN_OPTIONS being set to a different value for leak detection to be enabled?

(In reply to Mike Hommey [:glandium] from comment #10) > So we rely on ASAN_OPTIONS being set to a different value for leak detection > to be enabled? Yes. This makes the behavior the same in 3.7 as it is in whatever version we're using right now in automation (which defaults to having leak detection disabled). In practice, almost anything complex we'd run leakchecking on would need a suppression file, which requires setting LSAN_OPTIONS, so setting an additional environment variable doesn't seem like a large burden.

Comment on attachment 8677007 [details] [diff] [review] Disable LSan leak checking by default in mozglue. Review of attachment 8677007 [details] [diff] [review]: ----------------------------------------------------------------- Could you add a small comment in the code summarizing comments 9 to 11 ?

(In reply to Mike Hommey [:glandium] from comment #12) > Could you add a small comment in the code summarizing comments 9 to 11 ? Good idea. I've done that.