1123418 - assertion when both refcount logging and IOInterposer are on

Status: NEW

(Core :: XPCOM, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

Refcount logging goes to initialize itself. SymInitialize calls CreateFile, caught by IOInterposer which does some string manipulation, triggering the refcount logger again, which asserts when it tries to grab the refcount mutex (which is already held by the current thread).

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1 - Add functionality to temporarily disable IOInterposer on current thread

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2 - Disable IOInterposer when entering refcount logging functions

Comment 3

[Nathan Froyd [:froydnj]]

Ugh ugh ugh.  Two pervasive monitoring systems in a battle royale.

I think your patches are a reasonable way to solve the problem, but I'm not r+'ing them yet because I'm trying to think if we can do something better than adding even more overhead to refcount logging.  Would it be feasible to move IOInterposer startup a little later, after refcounting logging is initialized, or move refcount logging initialization earlier?

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #3)
> Ugh ugh ugh.  Two pervasive monitoring systems in a battle royale.
> 
> I think your patches are a reasonable way to solve the problem, but I'm not
> r+'ing them yet because I'm trying to think if we can do something better
> than adding even more overhead to refcount logging.  Would it be feasible to
> move IOInterposer startup a little later, after refcounting logging is
> initialized, or move refcount logging initialization earlier?

Well, the main case that I wrote this for was to deal with Win32, and on that platform NS_StackWalk calls into dbghelp for its stack unwinding. Unfortunately this could potentially trigger lazy loading of debugging symbols during the unwind, so I don't think that moving startup around will help in this case.

Should I restrict the effect of these patches to Windows, perhaps?

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8555392 [details] [diff] [review]
Part 1 - Add functionality to temporarily disable IOInterposer on current thread

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

I think restricting these patches to Windows-only is reasonable.  That being said, if we're going to do that, I think we should do it...

::: xpcom/build/IOInterposer.h
@@ +250,5 @@
> + * Attempt to enable IOInterposer reporting for the current thread. Enabling
> + * might not occur if there were multiple, nested disable requests.
> + * @return true if the current thread has been enabled.
> + */
> +bool TryEnableCurrentThread();

These two functions should not be exported, I think, regardless of Windows-only-ness.  RAII classes should be the only way to do this.

@@ +267,5 @@
> +class MOZ_STACK_CLASS DisableOnCurrentThread
> +{
> +public:
> +  explicit DisableOnCurrentThread(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM)
> +    : mDidDisable(DisableCurrentThread())

The constructor and destructor should do something on Windows only; on other platforms, they should be no-ops.  You might have to #ifdef mDidDisable or make it |protected:| to work around warnings.

There should be a comment here about why we're making this Windows-only.  I'm not entirely sure how to write it to make it non-refcount-logging specific, though.

This is all Win32 only, right?  Win64 doesn't suffer from this issue because we don't have to use dbghelp.dll, correct?

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8555394 [details] [diff] [review]
Part 2 - Disable IOInterposer when entering refcount logging functions

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

::: xpcom/base/nsTraceRefcnt.cpp
@@ +565,5 @@
>  nsresult
>  nsTraceRefcnt::DumpStatistics(StatisticsType aType, FILE* aOut)
>  {
>  #ifdef NS_IMPL_REFCNT_LOGGING
> +  mozilla::IOInterposer::DisableOnCurrentThread disableIOInterposer;

Somewhere, either here or further up in the file, we need a description of why we're inserting these guard classes.  An illustrative sequence of steps for what can go wrong if we don't have them would be great.