Closed Bug 1024259 Opened 9 years ago Closed 8 years ago



(Core :: XPCOM, defect)

Not set



Tracking Status
firefox42 --- fixed


(Reporter: mccr8, Assigned: jruderman)


(Blocks 1 open bug)



(2 files)

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.)
"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.
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)
> #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).  */
> #endif

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


which is nicer.
Attached patch incomplete patchSplinter Review
I searched for 
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.)

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

Attachment #8641396 - Flags: review?(continuation)
Comment on attachment 8641396 [details] [diff] [review]

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)


::: 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)

Attachment #8641396 - Flags: review?(continuation) → review+
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...
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.
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.