Tone down debug output from nsAtomTable.cpp#397

RESOLVED FIXED in Firefox 52

Status

()

Core
General
P3
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

unspecified
mozilla54
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed, firefox54 fixed, firefox-esr52 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
+++ 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.)
(Assignee)

Comment 1

3 months ago
Created attachment 8832600 [details] [diff] [review]
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.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8832600 - Flags: review?(nfroyd)
Comment on attachment 8832600 [details] [diff] [review]
1335854-tone-down-atoms.patch (v1a).

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

Thanks.
Attachment #8832600 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 3

3 months ago
Thanks ;-)
Keywords: checkin-needed
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.
status-firefox53: --- → affected
status-firefox54: --- → affected

Comment 5

3 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18d1694428a0
Tone down non-zero refcount message for dynamic atoms. r=froydnj
Keywords: checkin-needed
(Assignee)

Comment 6

3 months ago
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.
(Assignee)

Comment 7

3 months ago
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 ;-)
Attachment #8832600 - Flags: approval-mozilla-aurora?

Comment 8

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/18d1694428a0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8832600 [details] [diff] [review]
1335854-tone-down-atoms.patch (v1a).

Nicer asserts on debug builds, seems fine to bring to aurora.
Attachment #8832600 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 10

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/485b40055cff
status-firefox53: affected → fixed
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.
Attachment #8832600 - Flags: approval-mozilla-beta?
Comment on attachment 8832600 [details] [diff] [review]
1335854-tone-down-atoms.patch (v1a).

should go in beta52 together with bug 1276669
Attachment #8832600 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/97ce4f0843b9
status-firefox52: --- → fixed

Comment 15

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/97ce4f0843b9
status-firefox-esr52: --- → fixed
Setting qe-verify- based on Comment 7.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.