1335854 - Tone down debug output from nsAtomTable.cpp#397

Status: RESOLVED FIXED

(Core :: General, defect, P3)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1276669 +++

Firefox (and Thunderbird) debug builds have become very noisy due to nsAtomTable.cpp line 397:
  ASSERTION: dynamic atom with non-zero refcount ...

Using the profile manager alone I get about 250 lines of output, and since the lines are long and wrap on the debug console, that makes 500 lines of pretty useless output going past every single time. See bug 1303637 about the profile manager leaking a JS runtime and therefore atoms.

This output is making the lives of contributors harder than it needs to be.

This output should be compacted into a few lines. Investing a few more lines of code could convert this
      nsAutoCString name;
      atom->ToUTF8String(name);
      nsPrintfCString msg("dynamic atom with non-zero refcount %s!", name.get());
      NS_ASSERTION(false, msg.get());
to using an append and then dumping out the result, or only dumping the first 50 leaked atoms since I doubt anyone is interested in the whole lot.

In comparison:
WARNING: YOU ARE LEAKING THE WORLD
https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/js/src/vm/Initialization.cpp#141
is (only) showing 150 short lines of output which don't wrap.
(I don't know where the list of leaked URLs comes from.)

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1335854-tone-down-atoms.patch (v1a).

Sample output:

[1344] ###!!! ASSERTION: 253 dynamic atom(s) with non-zero refcount: stringBundl
eBindings,no-bar,autocomplete-richlistbox,buttonBindings,autocomplete-result-pop
upset,outset,TB 38,dialogBindings,extra2,arrowbox,integration,panelopen,preferen
ce,More Info,toolbarbutton-icon,enablehistory,autoSelectLastProfile,onDOMDocElem
entInserted,chromeclass-status,resizable,...: 'nonZeroRefcountAtomsCount == 0',
file c:/mozilla-source/comm-central/mozilla/xpcom/ds/nsAtomTable.cpp, line 411

I tried printing 50 atoms, but the line got so long that it was truncated on the Windows debug console.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8832600 [details] [diff] [review]
1335854-tone-down-atoms.patch (v1a).

Review of attachment 8832600 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 3

[Jorg K (CEST = GMT+2)]

Thanks ;-)

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Might as well request Aurora approval on this as well since bug 1276669 will be landing there Really Soon Now. Still a bit unclear what the patch that lands on Beta is going to look like, though.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18d1694428a0
Tone down non-zero refcount message for dynamic atoms. r=froydnj

Comment 6

[Jorg K (CEST = GMT+2)]

I guess Aurora uplift doesn't hurt to keep the code in sync. The patch here only makes the developers life easier, so it's not strictly needed on Aurora since I don't expect any developer to build this. That said, I'll request it now.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8832600 [details] [diff] [review]
1335854-tone-down-atoms.patch (v1a).

Small tweak to the NS_ASSERTION() output implemented in bug #1276669.

Approval Request Comment
[Feature/Bug causing the regression]: bug #1276669
[User impact if declined]: None. It's just to keep the M-C and M-A code in sync to make further uplifts easier.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: On a local build.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: No. Well, bug 1276669 of course.
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]: None.

Not risky, just "pretty prints" the NS_ASSERT() message which is only shown in debug builds. The program crashes afterwards anyway ;-)

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18d1694428a0

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8832600 [details] [diff] [review]
1335854-tone-down-atoms.patch (v1a).

Nicer asserts on debug builds, seems fine to bring to aurora.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/485b40055cff

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8832600 [details] [diff] [review]
1335854-tone-down-atoms.patch (v1a).

See comment 7. Note that this is only needed if bug 1276669 gets approved.

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 8832600 [details] [diff] [review]
1335854-tone-down-atoms.patch (v1a).

should go in beta52 together with bug 1276669

Comment 13

[Carsten Book [:Tomcat]]

(In reply to Julien Cristau [:jcristau] from comment #12)
> Comment on attachment 8832600 [details] [diff] [review]
> 1335854-tone-down-atoms.patch (v1a).
> 
> should go in beta52 together with bug 1276669

which has uplift conflicts, so waiting for this to get fixed

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/97ce4f0843b9

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/97ce4f0843b9

Comment 16

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Comment 7.