Closed Bug 1049214 Opened 10 years ago Closed 10 years ago

Debug builds spam 20+ copies of this warning at shutdown: "System JS : WARNING file:///$OBJDIR/dist/bin/components/nsContentPrefService.js:180 - setting a property that has only a getter"

Categories

(Toolkit :: Preferences, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: dholbert, Assigned: poiru)

References

Details

Attachments

(1 file, 1 obsolete file)

I've noticed many copies of the following warning in the shutdown-spew of my debug build in the past few days (after just having it open for a few minutes, viewing a testcase):
{
System JS : WARNING file:///scratch/work/builds/mozilla-central/mozilla-central-11-11-01.13-56/obj/dist/bin/components/nsContentPrefService.js:180 - setting a property that has only a getter
}

e.g. just now, I quit my debug build, and I got 25 consecutive copies of that warning in my terminal.

This warning comes from this code:
> 175     // Delete references to XPCOM components to make sure we don't leak them
> 176     // (although we haven't observed leakage in tests).  Also delete references
> 177     // in _observers and _genericObservers to avoid cycles with those that
> 178     // refer to us and don't remove themselves from those observer pools.
> 179     for (var i in this) {
> 180       try { this[i] = null }
> 181       // Ignore "setting a property that has only a getter" exceptions.
> 182       catch(ex) {}
> 183     }
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/nsContentPrefService.js?rev=b04759a8cbde#180

Note the comment at line 181, which mentions the warning & is attempting to work around it.

I think we're spamming it anyway, due to some recent strictness-upgrading of Firefox frontend code. (At least, I've noticed a lot more warnings for long-untouched JS code recently. This code here was added by Myk back in 2007, in http://hg.mozilla.org/mozilla-central/rev/656c65019719 -- but I'm only noticing the warnings now.)

Anyway, we need to figure out a better way to suppress this warning, or else it's going to flood the terminals of everyone shutting down debug builds indefinitely.
(Perhaps we could detect objects that only have a getter here, before we try to set them? Then we could drop the try/catch. Sorry if this is a silly suggestion; I am not a JS hacker. :))
Summary: System JS : WARNING file:///$OBJDIR/dist/bin/components/nsContentPrefService.js:180 - setting a property that has only a getter → Debug builds spam many copies of this warning at shutdown: "System JS : WARNING file:///$OBJDIR/dist/bin/components/nsContentPrefService.js:180 - setting a property that has only a getter"
FWIW: I can reproduce this when starting Firefox with a fresh profile (empty folder), pointed at about:blank, and immediately quitting. e.g.:
  mkdir deleteme
  ./dist/bin/firefox -profile deleteme/ -no-remote about:blank
  *quit with Ctrl+Q*

I see 22 copies of this warning in the shutdown-spew.  So, this should be easy for anyone to reproduce, I think.

This is with an up-to-date linux debug build (based on mozilla-central changeset 191e834ff32b).
Summary: Debug builds spam many copies of this warning at shutdown: "System JS : WARNING file:///$OBJDIR/dist/bin/components/nsContentPrefService.js:180 - setting a property that has only a getter" → Debug builds spam 20+ copies of this warning at shutdown: "System JS : WARNING file:///$OBJDIR/dist/bin/components/nsContentPrefService.js:180 - setting a property that has only a getter"
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
Attachment #8468663 - Flags: review?(myk)
(In reply to Daniel Holbert [:dholbert] from comment #1)
> (Perhaps we could detect objects that only have a getter here, before we try
> to set them? Then we could drop the try/catch. Sorry if this is a silly
> suggestion; I am not a JS hacker. :))

Yep, that is what the patch above does. I left the try/catch alone as I wasn't sure if they are still needed. If someone knows better, I can drop them as well.
what about using "delete this[i]" instead of "this[i] = null", that shouldn't have any problem with getters
Comment on attachment 8468663 [details] [diff] [review]
Fix 'setting a property that has only a getter' warnings on shutdown for nsContentPrefService.js and nsLoginManager.js

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

Hmm, the original code seems over-engineered, especially since only a few properties of the object hold references to XPCOM objects, and extra-especially since the comment says "we haven't observed leakage in tests."

My inclination would be to remove this code entirely and replace it with code that deletes only the specific properties whose references need to be broken.  For nsContentPrefService, that should be _observers, _genericObservers, and possibly the three properties that refer to XPCOM objects, i.e. __observerSvc, __consoleSvc, and __prefSvc.  I'm not sure what it is for nsLoginManager.js.

(Alternately, for the *Svc properties, you could replace them with global objects that are lazily retrieved by XPCOMUtils.defineLazyServiceGetter or simply import Services.jsm, which provides them.  But that seems like a bigger change than is warranted.  Simply deleting them or even leaving them alone should be sufficient.)

Also note that I'm not a peer of the Toolkit module, so I can't review this code.  :mossop should be able to do so, however, or find someone who can.
Attachment #8468663 - Flags: review?(myk)
Attachment #8471647 - Flags: review?(dtownsend+bugmail) → review+
https://hg.mozilla.org/mozilla-central/rev/6fbd6981ccbf
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: