1316527 - XPCOM refcount logging doesn't work

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Cameron McCormack (:heycam)]

Since bug 1309051, I can't use XPCOM refcount logging.

$ XPCOM_MEM_REFCNT_LOG=1 XPCOM_MEM_LOG_CLASSES=nsStandardURL ./obj/dist/bin/firefox -P test -no-remote
### XPCOM_MEM_REFCNT_LOG defined -- logging refcounts to stdout
### XPCOM_MEM_LOG_CLASSES defined -- only logging these classes: nsStandardURL 
Assertion failure: aCreate (If an object does not have a serial number, we should be creating it.), at /z/moz/central/xpcom/base/nsTraceRefcnt.cpp:590

Comment 2

[Andrew McCreight [:mccr8]]

You can work around this by commenting out the assertion. I'm not sure what is going wrong. I guess I didn't test the COM ptr case.

Comment 3

[Andrew McCreight [:mccr8]]

I could have sworn I tried this locally, but I don't see how it could have ever worked. Anyways, no matter what class you are logging, NS_LogCOMPtrAddRef

Comment 4

[Andrew McCreight [:mccr8]]

I could have sworn I tested this locally, but I don't see how it could have ever worked. Anyways, no matter what class you are logging, NS_LogCOMPtrAddRef always looks up every object it gets passed, to try to get a serial number. Before my patch, when it passed in false, it would just get zero back, and then it would just return without doing any additional work. Now, it asserts. The most direct fix is to add a third state to getting a serial number, where you don't assert on failure to get a serial number.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1316527 - Return 0 when GetSerialNumber fails to find an existing serial number.

NS_LogCOMPtrAddRef and NS_LogCOMPtrRelease always pass false to
GetSerialNumber, because they pass in everything they get without
regard to whether it is being logged or not, so they don't want to
create a serial number if none exists. This causes the assertions
added in bug 1309051 to be hit. To work around this, I hoist the
assertion into the other callers of this method. Two of them already
had this check, but it was non-fatal.

This also makes the asserts not happen in release builds, as I decided
it doesn't really matter what happens if somebody tries to use it
there.

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

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8811472 [details]
Bug 1316527 - Return 0 when GetSerialNumber fails to find an existing serial number.

https://reviewboard.mozilla.org/r/93580/#review93630

Do you think we could add tests for these, with some manual calls to the appropriate functions?

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> Do you think we could add tests for these, with some manual calls to the
> appropriate functions?

That's a good idea. I'm not sure exactly how that would work, though. Do we have any test suites we can use that start XPCOM but don't involve the full browser? Testing in a browser would involve starting up per-class logging during a run which might be weird.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/811084dd7ec0
Return 0 when GetSerialNumber fails to find an existing serial number. r=froydnj

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #7)
> (In reply to Nathan Froyd [:froydnj] from comment #6)
> > Do you think we could add tests for these, with some manual calls to the
> > appropriate functions?
> 
> That's a good idea. I'm not sure exactly how that would work, though. Do we
> have any test suites we can use that start XPCOM but don't involve the full
> browser? Testing in a browser would involve starting up per-class logging
> during a run which might be weird.

I was thinking we could do a gtest (gtests start XPCOM), that had #ifdef DEBUG tests, with calls like:

int placeholder;
NS_LogAddRef(&placeholder, 1, "foo", sizeof(placeholder));

I mean, we could make up our own classes, but all we'd be doing with calls like these would be smoke-testing that things work.  Maybe we could have introspection code into refcnt code so we could verify that when we log the final release, the object no longer exists in our tables or things like that.  But the smoke tests were all I was thinking about initially, as nothing really checks the code until someone decides to run refcount logging on their browser.

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/811084dd7ec0

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8811472 [details]
Bug 1316527 - Return 0 when GetSerialNumber fails to find an existing serial number.

Approval Request Comment
[Feature/regressing bug #]: bug 1309051
[User impact if declined]: No direct impact. This makes investigating leaks harder.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: Almost none. This is only a change to debug builds.
[String/UUID change made/needed]: none

Comment 12

[areya]

https://hg.mozilla.org/mozilla-central/rev/811084dd7ec0

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8811472 [details]
Bug 1316527 - Return 0 when GetSerialNumber fails to find an existing serial number.

fix refcount logging issue in aurora52

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7863150b9191