Make MOZ_ASAN imply NS_FREE_PERMANENT_DATA

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mccr8, Assigned: jruderman)

Tracking

(Blocks: 1 bug)

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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.)
(Reporter)

Comment 1

4 years ago
"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.
(Reporter)

Comment 2

4 years ago
Also, part of this will include auditing existing code like IOInterposer that uses MOZ_ASAN for this purpose.
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8641372 [details] [diff] [review]
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
(Assignee)

Comment 5

3 years ago
Created attachment 8641396 [details] [diff] [review]
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
(Assignee)

Updated

3 years ago
Attachment #8641396 - Flags: review?(continuation)
(Reporter)

Comment 6

3 years ago
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
Attachment #8641396 - Flags: review?(continuation) → review+
(Assignee)

Updated

3 years ago
Assignee: nobody → jruderman
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...
(Reporter)

Comment 9

3 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/21501e7b09ec
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.