Closed Bug 1350729 Opened 5 years ago Closed 4 years ago

Refcount logging of nsStringBuffer fails due to nsFakeStringBuffer

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: handyman, Assigned: mccr8)

References

Details

Attachments

(1 file)

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
Blocks: 1366294
Assignee: nobody → continuation
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?
Attachment #8886711 - Flags: review?(dbaron) → review+
(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.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9033647b9a51
Implement fake refcount logging for nsFakeStringBuffer. r=dbaron
Bah, I did a try push, but maybe doing -u all[x64] stops it from doing static analysis?
Flags: needinfo?(continuation)
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3bc3266f999a
Implement fake refcount logging for nsFakeStringBuffer. r=dbaron
(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
https://hg.mozilla.org/mozilla-central/rev/3bc3266f999a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.