1350729 - Refcount logging of nsStringBuffer fails due to nsFakeStringBuffer

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[David Parks [:handyman]]

In short, running the refcount logger with XPCOM_MEM_LOG_CLASSES=nsStringBuffer quickly fails due to missing serial number.

nsFakeStringBuffer is a bit of a hack that "has the same binary layout" as nsStringBuffer and, as such, is reinterpret_casted to an nsStringBuffer.  However, the nsFakeStringBuffer has its own refcount that it initializes to 1.  So, when these are first encountered by the refcount logger (after the cast to nsStringBuffer), the refcount is '2' and XPCOM ASSERTs that it never saw the refcount == 1, which is where it creates the object's Serial Number.  The stack at failure, where is AddRef's to bring the count to 2, is:

 	xul.dll!NS_LogAddRef(void * aPtr=0x1addbd60, unsigned int aRefcnt=0x00000002, const char * aClass=0x17860900, unsigned int aClassSize=0x00000008) Line 1042	C++
 	xul.dll!nsStringBuffer::AddRef() Line 191	C++
>	xul.dll!StaticAtom::StaticAtom(nsStringBuffer * aStringBuffer=0x1addbd60, unsigned int aLength=0x00000000, unsigned int aHash=0x00000000) Line 147	C++
 	xul.dll!RegisterStaticAtoms(const nsStaticAtom * aAtoms=0x178991dc, unsigned int aAtomCount=0x00000001) Line 654	C++
 	xul.dll!NS_RegisterStaticAtoms<1>(const nsStaticAtom[1] & aAtoms=...) Line 50	C++
 	xul.dll!NS_InitAtomTable() Line 556	C++
 	xul.dll!NS_InitXPCOM2(nsIServiceManager * * aResult=0x0053f92c, nsIFile * aBinDirectory=0x0124a1c0, nsIDirectoryServiceProvider * aAppFileLocationProvider=0x0053f8f0) Line 477	C++
 	xul.dll!XRE_XPCShellMain(int argc=0x00000009, char * * argv=0x012010a8, char * * envp=0x00719a80, const XREShellData * aShellData=0x0053fb78) Line 1453	C++
 	xul.dll!mozilla::BootstrapImpl::XRE_XPCShellMain(int argc=0x0000000b, char * * argv=0x012010a0, char * * envp=0x00719a80, const XREShellData * aShellData=0x0053fb78) Line 53	C++
 	xpcshell.exe!NS_internal_main(int argc=0x0000000b, char * * argv=0x012010a0, char * * envp=0x00719a80) Line 68	C++
 	xpcshell.exe!wmain(int argc=0x0000000b, wchar_t * * argv=0x00718d08) Line 115	C++
 	xpcshell.exe!__scrt_common_main_seh() Line 253	C++
 	kernel32.dll!@BaseThreadInitThunk@12()	Unknown
 	ntdll.dll!__RtlUserThreadStart()	Unknown
 	ntdll.dll!__RtlUserThreadStart@8()	Unknown

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1350729 - Implement fake refcount logging for nsFakeStringBuffer.

Running with XPCOM_MEM_LOG_CLASSES=nsStringBuffer triggers an
assertion in refcount logging for nsFakeStringBuffers. These are given
an initial refcount of 1, without calling NS_LOG_ADDREF. Then,
AddRef() is called on these objects in StaticAtom::StaticAtom(), and
we tell the refcount logging system about the fake buffer, and that it
has a refcount of 0, triggering the assertion.

The first part of the fix is to call NS_LOG_ADDREF for this initial
refcount, in StaticAtom().

This first fix causes refcount logging to start reporting that the
fake string buffers leak, when XPCOM_MEM_LOG_CLASSES is not set. This
is because refcount logging is now getting told about these objects
being AddRefed at 1, which it takes to mean that an object is created.

To work around this issue, I add an array gFakeBuffers that contains
every fake string buffer we create, and tell the refcount logging
system that these objects are all being destroyed, when the atom table
is being shut down. This could result in some bogosity if the fake
buffers are "leaked" but hopefully this is still an improvement over
the current state.

Review commit: https://reviewboard.mozilla.org/r/157506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157506/

Comment 2

[Andrew McCreight [:mccr8]]

This is really hacky but it seems to work in local testing.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=465141ea5308bc8953a6941ab659b523fd3736e3

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 8886711 [details]
Bug 1350729 - Implement fake refcount logging for nsFakeStringBuffer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157506/diff/1-2/

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8886711 [details]
Bug 1350729 - Implement fake refcount logging for nsFakeStringBuffer.

https://reviewboard.mozilla.org/r/157506/#review163016

It might have been nice to have a little less code to do this, but this does have the advantage that it's clear it's going to do the right thing, so r=dbaron.

::: xpcom/ds/nsAtomTable.cpp:178
(Diff revision 2)
>      mLength = aLength;
>      mIsStatic = true;
>      mString = static_cast<char16_t*>(aStringBuffer->Data());
> +
> +#if defined(NS_BUILD_REFCNT_LOGGING)
> +    if (!gFakeBuffers) {

maybe worth asserting here (outside the if) that you're on the main thread?

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #4)
> It might have been nice to have a little less code to do this, but this does
> have the advantage that it's clear it's going to do the right thing, so
> r=dbaron.
Yeah, it might be possible to change how nsFakeStringBuffer works so that it is less weird and thus requires less hackery, but I don't really understand it enough.

> maybe worth asserting here (outside the if) that you're on the main thread?
Sounds like a good idea.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8886711 [details]
Bug 1350729 - Implement fake refcount logging for nsFakeStringBuffer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157506/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9033647b9a51
Implement fake refcount logging for nsFakeStringBuffer. r=dbaron

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=114937943&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/0a46f78aab92d128306574e407c9439e76ba60b8

Comment 9

[Andrew McCreight [:mccr8]]

Bah, I did a try push, but maybe doing -u all[x64] stops it from doing static analysis?

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8886711 [details]
Bug 1350729 - Implement fake refcount logging for nsFakeStringBuffer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157506/diff/3-4/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3bc3266f999a
Implement fake refcount logging for nsFakeStringBuffer. r=dbaron

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> Bah, I did a try push, but maybe doing -u all[x64] stops it from doing
> static analysis?

Static analysis is a separate build, specifiable in things like:
try: -b d -p linux64,linux64-st-an -u all -t none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bc3266f999a