1024259 - Make MOZ_ASAN imply NS_FREE_PERMANENT_DATA

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

njn points out the existence of NS_FREE_PERMANENT_DATA, which is not heavily used.  But in the world of LSAN it would make sense to have ASAN imply it.  I'm a little scared of how it affects X shutdown, as I've tried messing with that in ASAN and weird things were happening.  (See for instance bug 993747.)

Comment 1

[Andrew McCreight [:mccr8]]

"And that will require working out the difference between NS_BUILD_REFCNT_LOGGING (which NS_FREE_PERMANENT_DATA depends on) and FORCE_BUILD_REFCNT_LOGGING (which IOInterposer::Clear() relies on)."
I'm not sure what you mean here Nick.  NS_FREE_PERMANENT_DATA doesn't depend on anything as far as I can tell.  I'd just add a clause to the definition of NS_FREE_PERMANENT_DATA like "|| defined(MOZ_ASAN)" and everything should be okay as far as I can tell.

Comment 2

[Andrew McCreight [:mccr8]]

Also, part of this will include auditing existing code like IOInterposer that uses MOZ_ASAN for this purpose.

Comment 3

[Nicholas Nethercote [inactive]]

We have this in nscore.h:

> #if defined(NS_TRACE_MALLOC) || defined(NS_BUILD_REFCNT_LOGGING) || defined(MOZ_VALGRIND)
> #define NS_FREE_PERMANENT_DATA
> #endif

and this in IOInterposer::Clear():

> #if defined(DEBUG) || defined(FORCE_BUILD_REFCNT_LOGGING) || defined(MOZ_ASAN)
>   UnregisterCurrentThread();
>   sMasterList = nullptr;
> #endif

My natural inclination is to add MOZ_ASAN to the former and change the latter to this:

> #if defined(DEBUG) || defined(NS_FREE_PERMANENT_DATA)

but I wasn't sure what is the difference between NS_BUILD_REFCNT_LOGGING and FORCE_BUILD_REFCNT_LOGGING. So I looked it up:

> #if (defined(DEBUG) || defined(FORCE_BUILD_REFCNT_LOGGING))
> /* Make refcnt logging part of the build. This doesn't mean that
>  * actual logging will occur (that requires a separate enable; see
>  * nsTraceRefcnt and nsISupportsImpl.h for more information).  */
> #define NS_BUILD_REFCNT_LOGGING
> #endif

so it appears we can actually change Clear() just to this:

> #if defined(NS_FREE_PERMANENT_DATA)

which is nicer.

Comment 4

[Jesse Ruderman]

Attachment: incomplete patch

I searched for 
  MOZ_ASAN
  MOZ_VALGRIND
  *_REFCNT_LOGGING
and I think I found most of the places that should be changed to NS_FREE_PERMANENT_DATA.

But there are a few problems:

* all.js cannot include nscore.h because it is not a C++ file. I think the defintion of NS_FREE_PERMANENT_DATA should be moved to the build system. (This would also fix the problem of forgetting to include nscore.h, or copying code to files that don't include it, or removing the include thinking it is unused.)

* https://bugzilla.mozilla.org/show_bug.cgi?id=993747#c3

Comment 5

[Jesse Ruderman]

Attachment: patch

Instead of making build system changes, adding nscore.h includes and fixing up all.js separately.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03f6f75d0988

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8641396 [details] [diff] [review]
patch

Review of attachment 8641396 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this.

::: dom/ipc/ContentChild.cpp
@@ +1910,5 @@
>          NS_WARNING("shutting down early because of crash!");
>          QuickExit();
>      }
>  
> +#if !defined(NS_FREE_PERMANENT_DATA)

Please use #ifndef NS_FREE_PERMANENT_DATA instead.

::: gfx/thebes/gfxPlatform.cpp
@@ +714,5 @@
>      // cleanly as possible even in production code, so call this
>      // cairo_debug_* function unconditionally.
>      //
>      // because cairo can assert and thus crash on shutdown, don't do this in release builds
> +#if defined(NS_FREE_PERMANENT_DATA)

Please use #ifdef NS_FREE_PERMANENT_DATA

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +125,5 @@
>  #if defined(MOZ_WIDGET_COCOA)
>      SharedMemoryBasic::CleanupForPid(mChildProcessHandle);
>  #endif
>      ProcessWatcher::EnsureProcessTerminated(mChildProcessHandle
> +#if defined(NS_FREE_PERMANENT_DATA)

#ifdef NS_FREE_PERMANENT_DATA

::: modules/libpref/init/all.js
@@ +2482,5 @@
>  // How long in days we will allow a plugin to work after the user has chosen
>  // to allow it persistently.
>  pref("plugin.persistentPermissionAlways.intervalInDays", 90);
>  
> +#if !defined(DEBUG) && !defined(MOZ_ASAN) && !defined(MOZ_VALGRIND)

It would be good to add a comment with NS_FREE_PERMANENT_DATA in it, so somebody grepping can find this.

::: xpcom/build/IOInterposer.cpp
@@ +463,5 @@
>  {
> +  /* Clear() is a no-op on release builds so that we may continue to trap I/O
> +     until process termination. In leak-checking builds, we need to shut down
> +     IOInterposer so that all references are properly released. */
> +#if defined(NS_FREE_PERMANENT_DATA)

#ifdef NS_FREE_PERMANENT_DATA

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21501e7b09ec

Comment 8

[Nicholas Nethercote [inactive]]

Pretty much every modified guard now has a slightly different meaning. It's a more consistent meaning, which is good, but I'm just checking that this was deliberate...

Comment 9

[Andrew McCreight [:mccr8]]

Yes, the places where this is set are things that should be done in any build where we care about shutdown leak checking, be it debug builds, LSan or Valgrind.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21501e7b09ec