985742 - fuelApplication.js:1512 - 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

(Firefox :: General, defect)

Description

[Szabolcs Hubai (:xabolcs)]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20140207 Firefox/24.0 Iceweasel/24.3.0 (Nightly/Aurora)
Build ID: 20140209162632

Steps to reproduce:

1. Start Nightly (e.g. Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140319030201 CSet: 3bc3b9e2cd99) in safe mode
2. Open Browser Console
3. Instantiate somehow the Application Object
3a. Open about:addons
3b. Open Web Console
3c. Type A for Application



Actual results:

Notice the warning from Bug 948227 complaining about resource://app/components/fuelApplication.js:1512 [1]


[1]: http://hg.mozilla.org/mozilla-central/file/d6e549d77ab2/browser/fuel/src/fuelApplication.js#l793

Comment 1

[Jeff Walden [:Waldo]]

Szabolcs, if you feel slightly motivated, bug 985729's approach is easily applied to fix this.  ;-)  Same for any other instances of this warning that show up from code that's doing

X.prototype = { ... };
X.prototype.__proto__ = someObj;

in run-once code.

Comment 2

[Szabolcs Hubai (:xabolcs)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> Szabolcs, if you feel slightly motivated, bug 985729's approach is easily
> applied to fix this.  ;-)  Same for any other instances of this warning that
> show up from code that's doing
> 
> X.prototype = { ... };
> X.prototype.__proto__ = someObj;
> 
> in run-once code.

Sure, but I don't have build environment, so just an untested patch is what I could send. :)

I tried the omni.ja unpack - edit *.js - repack omni.ja but that was unsuccessful.
The omni.ja wasn't a clear zip file.

Comment 3

[Szabolcs Hubai (:xabolcs)]

Attachment: Patch v1, untested

This should work, but I'm unsure about the |this.__proto__.__proto__| → |Object.getPrototypeOf(Object.getPrototypeOf(this))| change.

Jeff could you push it to try, or test it locally?

Comment 4

[Jeff Walden [:Waldo]]

Yeah, omni.ja *used* to be a zip file, then people started optimizing its format for our particular use case and it's deviated.  I might or might not be imagining the existence of a tool to do "repacks" of omni.ja with changed files, available...somewhere.

If you're interested, I suspect someone on Mozilla's #introduction IRC channel would be willing to help you set up a build environment.  See http://irc.mozilla.org/ for more info.  If not, no worries -- we need people to file bugs just as much as people to fix them.  Just figured if you could do both (and it seemed like you might be going on a filing spree for this particular warning ;-) ), that'd be ideal.  :-)  Totally up to you, of course.  Thanks for the patch here even if you can't test it!

In the meantime, here's a try-push of your patch, with one minor tweak to move that #include line up above the definition of Application and Application.prototype.  Cross your fingers!

https://tbpl.mozilla.org/?tree=Try&rev=7ed6c044a4cd

Seems like breaking up the Application definition with an intervening #include, for not huge reason, is worth avoiding.  But that could be just me.  :-)

Comment 5

[Szabolcs Hubai (:xabolcs)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> Yeah, omni.ja *used* to be a zip file, then people started optimizing its
> format for our particular use case and it's deviated.  I might or might not
> be imagining the existence of a tool to do "repacks" of omni.ja with changed
> files, available...somewhere.

Writing that repacker (if not exists yet) sounds like as a good first python bug. :)


> 
> If you're interested, I suspect someone on Mozilla's #introduction IRC
> channel would be willing to help you set up a build environment.  See
> http://irc.mozilla.org/ for more info.  If not, no worries -- we need people
> to file bugs just as much as people to fix them.  Just figured if you could
> do both (and it seemed like you might be going on a filing spree for this
> particular warning ;-) ), that'd be ideal.  :-)  Totally up to you, of
> course.  Thanks for the patch here even if you can't test it!

Surely, I would be interested, but I don't own a build capable machine currently,
just an oldschool laptop with 768 MB RAM. :)
Thankfully most fix of these warnings are pretty easy, as you wrote in comment 2.

> 
> In the meantime, here's a try-push of your patch, with one minor tweak to
> move that #include line up above the definition of Application and
> Application.prototype.  Cross your fingers!
> 
> https://tbpl.mozilla.org/?tree=Try&rev=7ed6c044a4cd

Thanks for the try-push, as I see it has enough green. :)

OK, will move that #incule.
And what about the |Object.getPrototypeOf(Object.getPrototypeOf(this))| change?
This will be answered by the reviewer? ;)

> 
> Seems like breaking up the Application definition with an intervening
> #include, for not huge reason, is worth avoiding.  But that could be just
> me.  :-)

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Patch

Given it's just moving two lines around, no need for a new patch, here's the one I try-servered.

Comment 7

[Jeff Walden [:Waldo]]

The Object.getPrototypeOf changes are fine, btw.  I might introduce an extra temporary to reduce line length, or maybe inline the actual object that's the prototype, but those are micro-quibbles.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8397949 [details] [diff] [review]
Patch

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

Can't wait to see FUEL disappear from our code, the maintenance cost has really no benefit balance.

::: browser/fuel/src/fuelApplication.js
@@ +748,4 @@
>  
>    // for nsISupports
> +  this.QueryInterface = XPCOMUtils.generateQI([Ci.fuelIApplication, Ci.extIApplication,
> +                                         Ci.nsIObserver, Ci.nsISupportsWeakReference]);

please better indent this as

this.QueryInterface = XPCOMUtils.generateQI([
  one,
  two
]);

@@ +755,5 @@
>                                      contractID: APPLICATION_CONTRACTID,
>                                      interfaces: [Ci.fuelIApplication,
>                                                   Ci.extIApplication,
>                                                   Ci.nsIObserver],
> +                                    flags: Ci.nsIClassInfo.SINGLETON});

this one should be reindented, as well

@@ +763,2 @@
>      // Call the extApplication version of this function first
> +    Object.getPrototypeOf(Object.getPrototypeOf(this)).observe.call(this, aSubject, aTopic, aData);

nit: for readability would be nice to assign Object.getPrototypeOf(Object.getPrototypeOf(this)) to a temporary nicely-named var

@@ +794,4 @@
>  
> +  Object.defineProperty(this, "activeWindow", {
> +    get: function () {
> +      return getWindow(Utilities.windowMediator.getMostRecentWindow("navigator:browser"));

while here, would be nice to make this use RecentWindow.jsm and RecentWindow.getMostRecentBrowserWindow();

Comment 9

[Szabolcs Hubai (:xabolcs)]

(In reply to Marco Bonardo [:mak] from comment #8)
> Comment on attachment 8397949 [details] [diff] [review]
> Patch
> 
> Review of attachment 8397949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can't wait to see FUEL disappear from our code, the maintenance cost has
> really no benefit balance.

Just FUEL, or the whole Application object too?

> 
> ::: browser/fuel/src/fuelApplication.js
> @@ +748,4 @@
> >  
> >    // for nsISupports
> > +  this.QueryInterface = XPCOMUtils.generateQI([Ci.fuelIApplication, Ci.extIApplication,
> > +                                         Ci.nsIObserver, Ci.nsISupportsWeakReference]);
> 
> please better indent this as
> 
> this.QueryInterface = XPCOMUtils.generateQI([
>   one,
>   two
> ]);
> 

Thanks! I like this identation style more.

> @@ +755,5 @@
> >                                      contractID: APPLICATION_CONTRACTID,
> >                                      interfaces: [Ci.fuelIApplication,
> >                                                   Ci.extIApplication,
> >                                                   Ci.nsIObserver],
> > +                                    flags: Ci.nsIClassInfo.SINGLETON});
> 
> this one should be reindented, as well
> 
> @@ +763,2 @@
> >      // Call the extApplication version of this function first
> > +    Object.getPrototypeOf(Object.getPrototypeOf(this)).observe.call(this, aSubject, aTopic, aData);
> 
> nit: for readability would be nice to assign
> Object.getPrototypeOf(Object.getPrototypeOf(this)) to a temporary
> nicely-named var
> 

|superPrototype| is nice enough? :) I'm not good on naming things.

> @@ +794,4 @@
> >  
> > +  Object.defineProperty(this, "activeWindow", {
> > +    get: function () {
> > +      return getWindow(Utilities.windowMediator.getMostRecentWindow("navigator:browser"));
> 
> while here, would be nice to make this use RecentWindow.jsm and
> RecentWindow.getMostRecentBrowserWindow();

I also shortened the Component.utils here.



I'm uploading the updated patch soon with r+, but I'll require f? from you Marco.
A try run would also nice, due the "activeWindow" modification at least.

Comment 10

[Szabolcs Hubai (:xabolcs)]

Attachment: Patch v2, untested

Addressed nits: styling, nice prototye variable, RecentWindow.jsm

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Szabolcs Hubai (:xabolcs) from comment #9)
> Just FUEL, or the whole Application object too?

Application is a piece of FUEL, the whole thing has been replaced by better APIs in the sdk... btw will happen in future.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8398989 [details] [diff] [review]
Patch v2, untested

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

no need for further review, the previous one is still valid, thanks

::: browser/fuel/src/fuelApplication.js
@@ +808,5 @@
> +
> +      Object.defineProperty(this, "activeWindow", {
> +        get: function () {
> +          return RecentWindow.getMostRecentBrowserWindow();
> +        },

nit: I think you may use an arrow function to compact this code, like
get: () => RecentWindow.getMostRecentBrowserWindow(),

Comment 13

[Szabolcs Hubai (:xabolcs)]

Attachment: Patch v3, untested

Compact call forward.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8400276 [details] [diff] [review]
Patch v3, untested

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

as I said, there's no need for further review

Comment 15

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/f3b94fd3b799

Comment 16

[Carsten Book [:Tomcat]]

sorry had to backout this change for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=37263241&tree=Fx-Team

Comment 17

[Jeff Walden [:Waldo]]

So the activeWindow adjustments bounced, clearly.  How about we just keep doing it as it was done before (without the .jsm), and leave circling around to that for some other bug?

Comment 18

[Szabolcs Hubai (:xabolcs)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> So the activeWindow adjustments bounced, clearly.  How about we just keep
> doing it as it was done before (without the .jsm), and leave circling around
> to that for some other bug?

Now I have a build machine, I'm able to build locally.

Could you point me to a wiki page, where I could find how to run the failing test?

Comment 19

[Szabolcs Hubai (:xabolcs)]

(In reply to Szabolcs Hubai (:xabolcs) from comment #18)
> Could you point me to a wiki page, where I could find how to run the failing
> test?


Run
> ./mach mochitest-browser browser/fuel/test/
to run the FUEL tests!

Comment 20

[Szabolcs Hubai (:xabolcs)]

Attachment: Patch v4, locally tested

Reverted activeWindow's change to use RecentWindow.


It's very interesting. I think Application got instantiated too early to use RecentWindow.jsm.
This code slice works like a charm copied into example code in browser console.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/a2b5f542f8a8

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a2b5f542f8a8

Comment 23

[Jeff Walden [:Waldo]]

Not worth testing.