Closed Bug 982856 Opened 9 years ago Closed 8 years ago

Debug build-spew: "WARNING resource://gre/modules/Preferences.jsm:378 - mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create"

Categories

(Toolkit :: Preferences, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: dholbert, Assigned: Waldo, Mentored)

References

Details

(Whiteboard: [good second bug][lang=js])

Attachments

(4 files, 4 obsolete files)

When starting my debug build today, I noticed this towards the beginning of the output in my terminal:
===========
System JS : WARNING resource://gre/modules/Preferences.jsm:378 - mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create
===========



That points to this chunk of code:
> 375 // Give the constructor the same prototype as its instances, so users can access
> 376 // preferences directly via the constructor without having to create an instance
> 377 // first.
> 378 Preferences.__proto__ = Preferences.prototype;
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Preferences.jsm?rev=a2bcdf9b6a2a&mark=375-378#375


Filing this bug on doing whatever's necessary to address this warning.

(I'm hitting this with up-to-date mozilla-central, at cset b1b1f0e5f8da.)
Looks like this warning was turned on in the last ~24 hours, in bug 948227.
Depends on: 948227
Attached patch Partial patch (obsolete) — Splinter Review
I wrote up a partial patch to fix this.  It had issues on try when I tested it, that I never got around to working through.  But this is certainly most of the way there.
Couple extra notes on that patch.  The changes in that patch did their best to minimize whitespace-only changes -- you'd probably want a second patch that reindented everything, afterward.  And the comments on the |Preferences.prototype = Preferences;| bit of it are a bit off, because I didn't quite understand how Preferences.<foo>() differed from |new Preferences(...).<foo>()| and from |new Preferences().<foo>()|.  The comment's probably an easy fix for someone who knows the API a little better.  The whitespace is pretty simple too.  I did some debugging for why the patch had issues, but I didn't ultimately figure out what the issue was.
(In reply to Jeff Walden from comment #3)
> you'd probably want a second patch that reindented everything

And a third patch that switched to using Services.jsm too ;-)
Duplicate of this bug: 985830
It would be nice to get this fixed -- and if the conversion is documented, it would be a useful reference for people that need to convert their own code.
Duplicate of this bug: 1056845
OS: Linux → All
Product: Firefox → Toolkit
Hardware: x86_64 → All
Duplicate of this bug: 1055527
Mentor: bmcbride
Whiteboard: [good second bug][lang=js]
Duplicate of this bug: 1037786
Duplicate of this bug: 1089127
Still probably doesn't work, but given a recent question about this, seems worth dumping the patch rebased against tip.

One possible avenue for investigation of the issues encountered here, is that my understanding of Preferences.<something>(...) versus new Preferences(...).<something>(...) may have been wrong.  I think I was assuming the former was equivalent to new Preferences().<something>(...), but that may have been wrong.

One way in particular things might have gone wrong, is that an assignment like |this.foo = ...| could be *shadowing* a Preferences.prototype.foo value, where it didn't before.  But that's just a hazy guess.

In any event, this is still probably a fairly short bit of investigation away from being landable.  Fortunately a poke on IRC suggests such investigation is in the offing, so hopefully this'll be fixed soon.  \o/
Attachment #8390090 - Attachment is obsolete: true
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Assignee: jwalden+bmo → nobody
Status: ASSIGNED → NEW
Attached patch 982856.patch (obsolete) — Splinter Review
All right here's what I found:

The health-report module wasn't working for me.
After investigation, it seems that some module (X) is using the "Preferences.get(...)" form before health-report (which is using the "new Preferences().get(...)" form).

It means that the getter of _prefSvc is called somewhere in X.
But in the end of this getter, it is redefining itself :
> Object.defineProperty(this, "_prefSvc", { value: prefSvc });

And since Preferences.prototype === Preferences, when we create a new instance of Preferences in health-report, _prefSvc is already set to a -wrong- value since _defaultBranch is different.

I deleted the defineProperty line, but it also means that the full getter will be executed every time we call "get(...)" on Preferences. Is that OK performance-wise ?
Bah, this slipped below the top of my queue.  Sorry for the delay in responding...  :-(

Currently, we replace the laborious getter with a getter that returns a constant value.  So we regress nothing retaining a getter here.  But we probably do need to ensure that we're not constantly regetting the service and branch each time -- we still need to cache, even if the cache goes through a getter.

Here's an attempt to do that -- completely untested, could have stupid typos, might blow up try, who knows -- but if it passes, this is what I think we want.  Cross your fingers!

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=aeff65afe86a
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> Bah, this slipped below the top of my queue.  Sorry for the delay in
> responding...  :-(
> 
> Currently, we replace the laborious getter with a getter that returns a
> constant value.  So we regress nothing retaining a getter here.  But we
> probably do need to ensure that we're not constantly regetting the service
> and branch each time -- we still need to cache, even if the cache goes
> through a getter.
> 
> Here's an attempt to do that -- completely untested, could have stupid
> typos, might blow up try, who knows -- but if it passes, this is what I
> think we want.  Cross your fingers!
> 
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=aeff65afe86a

assertion failures and outside network connections and such. :-(
Attached patch Ready to go (obsolete) — Splinter Review
Previous push had a hasty typo in it.  Running the single test that was failing there, locally, passes, and preliminary try results suggest bc* is back to green, so let's do this now.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e34ab791ca9c
Assignee: nobody → jwalden+bmo
Attachment #8514598 - Attachment is obsolete: true
Attachment #8514719 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8522515 - Flags: review?(bmcbride)
Comment on attachment 8522515 [details] [diff] [review]
Ready to go

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

::: toolkit/modules/Preferences.jsm
@@ +385,5 @@
> +  {
> +    get: function _ioSvc() {
> +      let ioSvc = Cc["@mozilla.org/network/io-service;1"].
> +                  getService(Ci.nsIIOService);
> +      Object.defineProperty(this, "_ioSvc", { value: ioSvc });

Rather than rewriting this getter, I'd rather you switched to using Services.jsm. Should do the same for the pref service too.
Attachment #8522515 - Flags: review?(bmcbride) → review-
It turns out _ioSvc is entirely unused, so I just deleted it.

As for _prefSvc, I think you're being partly misled by a bad name.  _prefSvc isn't the service, it's a branch retrieved from the service.  So the getter there must remain.  Still, I guess the Components-based access to the service is replaceable with Services.prefs, so I did that.  (And tweaked a comment, and got rid of reuse of the |prefSvc| name that may have been misleading here.)

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bf5171c1ad5a (not completed, but slight local tests suggest it's okay)

If you think the _prefSvc name should be redone, I'm happy to do that in a followup patch.  We sort of need one anyway to de-indent most of the file (which was left as-is in this patch for readability purposes).
Attachment #8522745 - Flags: review?(bmcbride)
Attachment #8522515 - Attachment is obsolete: true
Attachment #8522745 - Flags: review?(bmcbride) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> Still, I guess the Components-based access to the
> service is replaceable with Services.prefs, so I did that. 

Yea, sorry, this is actually what I meant :)
And the followup patches now.
Attachment #8523199 - Flags: review?(bmcbride)
Comment on attachment 8523199 [details] [diff] [review]
Rename Preferences._prefBranch to Preferences._branchStr

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

I appreciate the cleanups you're doing here :)
Attachment #8523199 - Flags: review?(bmcbride) → review+
Attachment #8523201 - Flags: review?(bmcbride) → review+
Attachment #8523202 - Flags: review?(bmcbride) → review+
Thanks for the help with the last bit of debugging,  Edouard!  This would have sat much longer without your help figuring out what was wrong.  :-)
No problem Jeff, I was happy to help you finalize your patch!
You need to log in before you can comment on or make changes to this bug.