Refcount logging of nsStringBuffer fails due to nsFakeStringBuffer

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: handyman, Assigned: mccr8)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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
(Assignee)

Updated

a year ago
Blocks: 1366294
(Assignee)

Updated

a year ago
Assignee: nobody → continuation
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
This is really hacky but it seems to work in local testing.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=465141ea5308bc8953a6941ab659b523fd3736e3
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
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+
(Assignee)

Comment 5

a year ago
(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 hidden (mozreview-request)

Comment 7

a year ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9033647b9a51
Implement fake refcount logging for nsFakeStringBuffer. r=dbaron
(Assignee)

Comment 9

a year ago
Bah, I did a try push, but maybe doing -u all[x64] stops it from doing static analysis?
Flags: needinfo?(continuation)
Comment hidden (mozreview-request)

Comment 11

a year ago
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

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3bc3266f999a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.