1505294 - Assertion failure: serialno != 0 (Serial number requested for unrecognized pointer! Are you memmoving a MOZ_COUNT_CTOR-tracked object?), with XPCOM_MEM_LOG_CLASSES=nsTArray_base

Status: NEW

(Core :: XPCOM, enhancement, P3)

Description

[Mike Hommey [:glandium]]

Currently, this happens because GMP code has a nsTArray<GMPCapabilityData>, and GMPCapabilityData contains a nsTArray of GMPAPITags. So when the array buffer of the former is reallocated, the addresses of the latter instances change, so they don't match what's stored in gSerialNumbers.

This seems to happen pretty early during startup, which makes XPCOM_MEM_LOG_CLASSES=nsTArray_base essentially impossible to use anymore.

Comment 1

[Mike Hommey [:glandium]]

The only way that I can think of to make XPCOM_MEM_LOG_CLASSES=nsTArray_base work is to add a field to nsTArray_base to store its serial, and have dedicated NS_Log{C,D}tor functions for it. Which is fine when debugging specific cases (which I happen to have right now), but might be too much to pay for all debug builds?

Comment 2

[Nathan Froyd [:froydnj]]

I'm confused: when we reallocate nsTArray<GMPCapabilityData>, why aren't the original nsTArray<GMPAPITags> members destructed properly?

I see that GMPCapabilityData and GMPAPITags are defined in IPDL, so maybe the IPDL-generated code is doing something wonky that confuses the leak checking mechanism?

Comment 3

[Nathan Froyd [:froydnj]]

Oh, oh, I see, these are memmovable by default, so we don't actually call destructors.  Maybe we should make all IPDL-generated structs use explicit constructors/destructors, or at least ones where we can tease out they're used in arrays?

Comment 4

[Mike Hommey [:glandium]]

I think IPDL is a red herring. I've hit the problem with style stuff too, and I don't think the types involved in nesting nsTArrays came from IPDL.

Comment 5

[Mike Hommey [:glandium]]

It feels like we need some kind of static analysis that all Ts in nsTArray<T> are safe to memcpy/memmove, or that they have a DECLARE_USE_COPY_CONSTRUCTORS for them.

Comment 6

[Mike Hommey [:glandium]]

Heh, that check already exists... we "just" need nsTArray_base to be marked as MOZ_NON_MEMMOVABLE in debug builds.

Comment 7

[Mike Hommey [:glandium]]

Oh the fun. There are *tons* of uses of (indirect) nested nsTArrays, and some of the involved types are nested classes... which can't be forward declared. But it's also not possible to DECLARE_USE_COPY_CONSTRUCTORS() after their declaration because the "template specialization for nsTArray_CopyChooser must occur at global scope", and declaring them at global scope is not possible because it then comes "after instantiation".

One way out would be to move the nested classes to the global scope, but some of them are in private sections, and moving them to the global scope would make them public...

I have two random ideas here:
- Adding "an annotation" to all the types that embed an nsTArray, in the form of some base class, allowing to template-define nsTArray_CopyChooser based on std::is_base_of.
- Just making nsTArray never memcpy/memmove on debug builds.

Considering there are already 15 "exceptions" via DECLARE_USE_COPY_CONSTRUCTORS, and that I added 10 after barely scratching the surface, it seems like the latter might be the right option.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #7)
> I have two random ideas here:
> - Adding "an annotation" to all the types that embed an nsTArray, in the
> form of some base class, allowing to template-define nsTArray_CopyChooser
> based on std::is_base_of.
> - Just making nsTArray never memcpy/memmove on debug builds.
> 
> Considering there are already 15 "exceptions" via
> DECLARE_USE_COPY_CONSTRUCTORS, and that I added 10 after barely scratching
> the surface, it seems like the latter might be the right option.

Doing the latter is roughly equivalent to flipping the default for nsTArray, which is a world of pain.  We've wanted that for a little while (there's a bug I can CC you on if I find it today), but my initial foray indicated that there were several hundred classes that needed auditing.  It's not just DECLARE_USE_COPY_CONSTRUCTORS, but you have to make sure that the move/copy constructor(s) the class has actually makes sense, which is sometimes not the case.

Comment 9

[Mike Hommey [:glandium]]

Plus there's the "simple" problem that some of the copy constructors are currently deleted, so just switching the default doesn't even compile.