982856 - 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"

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[Daniel Holbert [:dholbert]]

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.)

Comment 1

[Daniel Holbert [:dholbert]]

Looks like this warning was turned on in the last ~24 hours, in bug 948227.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Partial patch

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.

Comment 3

[Jeff Walden [:Waldo]]

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.

Comment 4

[[e-mail]]

(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 ;-)

Comment 6

[Adam Moore]

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.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Rebased version of original patch

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/

Comment 12

[Edouard Oger [:eoger]]

Attachment: 982856.patch

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 ?

Comment 13

[Jeff Walden [:Waldo]]

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

Comment 14

[:Gijs (he/him)]

(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. :-(

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Ready to go

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

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 17

[Jeff Walden [:Waldo]]

Attachment: With _ioSvc removed, _prefSvc using Services.prefs

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).

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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 :)

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Rename Preferences._prefBranch to Preferences._branchStr

And the followup patches now.

Comment 20

[Jeff Walden [:Waldo]]

Attachment: Rename Preferences._prefSvc to Preferences._prefBranch

Comment 21

[Jeff Walden [:Waldo]]

Attachment: De-indent Preferences.jsm contents that are now over-indented

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3b042bd445f5 for the lot

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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 :)

Comment 23

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d3000ff0431
https://hg.mozilla.org/integration/mozilla-inbound/rev/c023d6805859
https://hg.mozilla.org/integration/mozilla-inbound/rev/88f73846cf1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d6476e1fcfe

Comment 24

[Jeff Walden [:Waldo]]

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.  :-)

Comment 25

[Edouard Oger [:eoger]]

No problem Jeff, I was happy to help you finalize your patch!

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d3000ff0431
https://hg.mozilla.org/mozilla-central/rev/c023d6805859
https://hg.mozilla.org/mozilla-central/rev/88f73846cf1c
https://hg.mozilla.org/mozilla-central/rev/5d6476e1fcfe