1345567 - GetSerialNumber asserts (crashes) incorrectly when logging ref counts of inheritance-related types

Status: NEW

(Core :: XPCOM, defect, P3)

Description

[David Parks [:handyman]]

Attachment: WIP: Failed attempt to make the ASSERT inheritance-aware

If you set the XPCOM refcount logging variable to record a class as well as a subclass of that class then the refcount mechanism will complain about a bad refcount on the first AddRef call.

For example, if classes A is a superclass of B (B is declared with NS_DECL_ISUPPORTS_INHERITED) and we run with XPCOM_MEM_LOG_CLASSES="A,B" then this assert will trip while logging B [1].

The issue is that the logging mechanism has already created a serial number for the object when it logged the base class A but doesn't consider that when doing the test while logging the subclass B.

This is probably a regression from bug 1309051.

---

Extra notes on what I've learned:

One work-around is for the user to not log both classes -- logging A alone would have produced the same events.  But this isn't always obvious.  There are also good reasons to sometimes do both (since the refcount logging procedure usually takes too long to do twice).

The attached patch doesn't fix the issue.  I initially thought I could create an NS_LOG_ADDREF_INHERITED to check the condition but thats wrong.  While the current setup never allows logging an ancestral relationship like A and B above, the code in the patch ends up requiring you to _always_ log the _base_ of any subtype, which is obviously awful.  The problem with the idea was that the subclasses AddRef needs to know whether a superclass created the serial number, not just whether a base class exists (since the base class may not be logged!)

The real problem is the cemented APIs in this module and the fact that this is heavily-used code.  We could e.g. check the condition before calling Super::AddRef to know if we might need to create the serial number but there is no decent mechanism for communicating this to Super::AddRef and even an extra hash-lookup per call is a huge penalty.  So I have no good ideas.

---

[1] https://hg.mozilla.org/mozilla-central/annotate/58753259bfeb3b818eac7870871b0aae1f8de64a/xpcom/base/nsTraceRefcnt.cpp#l578

Comment 1

[Nathan Froyd [:froydnj]]

Thanks for the report.

One possibility would be to have the uniqueness of GetSerialNumber be dependent on the class name as well as the pointer being AddRef'd, not just the pointer alone, similar to GetBloatEntry.  I'm not sure if that would negate some of the benefits of bug 1309051 or not.  Andrew, thoughts?

Comment 2

[Andrew McCreight [:mccr8]]

These asserts seem like more trouble than they are worth.