249012 - tons of strict Javascript warnings in EM (137041)

Status: VERIFIED WORKSFORME

(Toolkit :: Add-ons Manager, defect)

Description

[Henrik Gemal]

Warning: redeclaration of var i
Source File:
file:///C:/Program%20Files/Mozilla/Releases/Mozilla%20Firefox/components/nsExtensionManager.js
Line: 524, Column: 13
Source Code:
    for (var i = 0; i < lines.length; ++i)

Warning: redeclaration of var isProfile
Source File:
file:///C:/Program%20Files/Mozilla/Releases/Mozilla%20Firefox/components/nsExtensionManager.js
Line: 555, Column: 10
Source Code:
      var isProfile = parts[1] == this.TOKEN_PROFILE;

Warning: redeclaration of var i
Source File:
file:///C:/Program%20Files/Mozilla/Releases/Mozilla%20Firefox/components/nsExtensionManager.js
Line: 1007, Column: 15
Source Code:
      for (var i = 0; i < this._packagesForExtension.length; ++i) {

etc etc etc etc...

do you want a patch to fix these?

Comment 1

[PikeUK]

*** Bug 249235 has been marked as a duplicate of this bug. ***

Comment 2

[Steffen Wilberg]

Henrik, I'd love to see this fixed. Are you going to make a patch?

Comment 3

[Henrik Gemal]

I dont have the time. Sorry. It should be very easy to fix those warnings.

Ben: Do you want to fix those yourself or do you want a patch?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

*** Bug 254130 has been marked as a duplicate of this bug. ***

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Great, please use a better summary to circumvent dupes. This one can't find
anybody. :(

Comment 6

[Peter Hendriks]

Attachment: Modified javascripts for Firefox components

I modified several Javascipt files in FireFox in order to prevent these strict
warnings. The modified files are in a zip file along with a readme file.

Please let FireFox devs always use strict javascript checking while developing
ff components. This should prevent these kind of errors and is an excellent
early warning system for other types of bugs.

Comment 7

[timeless]

[e-mail]: please use cvs diff -up instead.

Comment 8

[Steffen Wilberg]

Comment on attachment 159321 [details]
Modified javascripts for Firefox components

I'm sorry, but you need to modify the files from the source code. You've taken
the code from browser.jar and toolkit.jar, but that code is pre-processed.
The original source code contains a lot of #ifdef, which are splitting the code
between platforms and apps. For example, your code doesn't contain this large
block, because it is for Thunderbird only:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/mozapps/extensions/
content/extensions.js&rev=1.6.6.23&mark=831-886#831

This document tells you how to get the source: http://www.mozilla.org/cvs.html

But also read this document, especially the note (which I just added) about the
aviary branch:
http://www.mozilla.org/projects/firefox/build.html

Comment 9

[gekacheka]

Attachment: 29 warnings from JavaScript console

Comment 10

[gekacheka]

Attachment: nsExtensionManager.js.in patch: fix 29 warnings

Comment 11

[Steffen Wilberg]

Comment on attachment 162183 [details] [diff] [review]
nsExtensionManager.js.in patch:  fix 29 warnings

OK, this fixes the warnings.
But please declare variables outside if/else and switch/case clauses, 'for'
loops etc., instead of just removing all "var"s after the first one. That's
both clearer and less likely to become wrong because of future changes.

And please remove the "var"s instead of commenting them out.

Comment 12

[gekacheka]

Attachment: nsExtensionManager.js.in patch: fix 29 warnings

Renamed some vars so intended scope is still clear.

Comment 13

[Steffen Wilberg]

Why do you introduce a bunch of new variables lke i1, i2, i3, PRBool1, PRBool2,
win1, win2, etc., especially if they are not used outside their for-loop or the
same values are assigned to them?

For example, why don't you do this:
  var i;
  switch...
  case...:
    for (i=0...
  case...:
    for (i=0...

Comment 14

[gekacheka]

To make clear the intended scope of each variable's use, so the code easier to
understand and maintain.  

In cases where there were a series of for loops close together, the patched 
code does declare one variable i and reuse it. 

However, many of the functions are long, with large if-else blocks or switch
cases.  Declaring a variable in a nested block makes clear that a variable is
new and used only within that block.  People reading or modifying the code can
quickly see that:
 * The variable is new, so there's no need to think about what the previous
value was, and when it is safe to overwrite it.
 * The value is only used in the block, so there's no need to think about
whether the variable is read later outside the block.

The previous patch preserved the intended declaration block by commenting the
reused /*var*/.  This patch preserves the intended declaration block by giving
the conflicting vars different names in different branches.

Comment 15

[Andrés G. Aragoneses]

Can you attach a correct nsExtensionManager.js file (already patched) for us to
replace the existing one on our releases? This errors really annoy me because I
have installed the WebDeveloper extension and configured it to pop up the JS
Console whenever a warning/error is detected. Thanks.

Comment 16

[Jason Barnabe (np)]

Attachment: patch

This patch only creates one new variable, appExtensionsVersion in GetTarget
(the appVersion variable was being used for two purposes in that function).

Comment 17

[Mike Connor [:mconnor]]

Comment on attachment 172829 [details] [diff] [review]
patch

this is going to be rather far down my list, so you may want to look at the
current list of toolkit peers and find another reviewer, since its probably 3
weeks at the earliest before I'll have real time for reviews.

Comment 18

[Jason Barnabe (np)]

Thanks for replying so quickly, Mike. Where can I find that list? (this is my
first patch)

Comment 19

[Darin Fisher]

so why are we concerned with these javascript script warnings?  i profess to be
ignorant about what the significance of these warnings are.  if this is
important, then maybe someone should include this in the build system so that
each JS file is checked during a build.  that way we can keep the tree "clean"
if that is indeed important.

Comment 20

[Brendan Eich [:brendan]]

The first two could be bad, and should be fixed.  Look at
http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#320
and
http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#323
and you'll see the problem.

The rest are about redeclared vars, which generally are less of a problem.  It
would be tidier to declare once per name in each top-level or function scope,
but it's not a big deal.

/be

Comment 21

[Henrik Gemal]

perhaps the patch could also remove the dump functions

Comment 22

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I care about them because JS strict warnings are one of the few diagnostic tools
I have in extension/XBL/etc.-land, but enabling them treats me to an enormous
amount of spew in the JS console, obscuring the ones I'm looking for (such as my
own!).  The same reason that I care about warnings in C++, really.

Comment 23

[Benjamin Smedberg]

Comment on attachment 172829 [details] [diff] [review]
patch

>Index: nsExtensionManager.js.in

>-    var item = this._getResourceForItem(aItemID);
>+    //var item = this._getResourceForItem(aItemID);
>     var index = container.IndexOf(item);

Why did you move this, since it is used in the next line?

Comment 24

[gekacheka]

The previous version of the patch explained better: it is bound to the same
thing a few lines above.
https://bugzilla.mozilla.org/attachment.cgi?id=162933&action=diff#mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in_sec11

(In reply to comment #23)
> (From update of attachment 172829 [details] [diff] [review] [edit])
> >Index: nsExtensionManager.js.in
> 
> >-    var item = this._getResourceForItem(aItemID);
> >+    //var item = this._getResourceForItem(aItemID);
> >     var index = container.IndexOf(item);
> 
> Why did you move this, since it is used in the next line?
> 

Comment 25

[Jason Barnabe (np)]

Thanks for the review. Can you check this in?

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #19)
> so why are we concerned with these javascript script warnings?  i profess to be
> ignorant about what the significance of these warnings are.  if this is
> important, then maybe someone should include this in the build system so that
> each JS file is checked during a build.  that way we can keep the tree "clean"
> if that is indeed important.

I think this is a fine idea.  In general, it seems to me that --enable-debug
ought to force JS strict warnings to be on, so that people will see them.

Comment 27

[gekacheka]

The last patch skips the first 2 warnings about == instead of = in
stackTraceFunctionFormat.  Overlooked?

Comment 28

[Jason Barnabe (np)]

(In reply to comment #27)
> The last patch skips the first 2 warnings about == instead of = in
> stackTraceFunctionFormat.  Overlooked?
That function is commented out.
http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#318

Comment 29

[Darin Fisher]

what would it take to get JS strict warnings enabled by default in debug builds?
 or would that just be too painful?

Comment 30

[Benjamin Smedberg]

Just an #ifdef in all.js -- very simple, in fact. I don't know the pain level.

Comment 31

[timeless]

darin: you mean like bug 248528?

Comment 32

[Serge Gautherie (:sgautherie)]

Comment on attachment 172829 [details] [diff] [review]
patch

[Mozilla Thunderbird, version 1.0.1 (20050309)] (nightly) (W98SE)

Could someone check this in ?

>Index: nsExtensionManager.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v

>@@ -3992,7 +4001,7 @@ nsExtensionsDataSource.prototype = {
>                               .createInstance(Components.interfaces.nsIRDFContainer);
>     container.Init(ds, extensions);
>     
>-    var item = this._getResourceForItem(aItemID);
>+    //var item = this._getResourceForItem(aItemID);
>     var index = container.IndexOf(item);
>     if (index > 1) {
>       container.RemoveElement(item, false);

This should be removed, instead of commented out.

>@@ -4010,7 +4019,7 @@ nsExtensionsDataSource.prototype = {
>                               .createInstance(Components.interfaces.nsIRDFContainer);
>     container.Init(ds, extensions);
>     
>-    var item = this._getResourceForItem(aItemID);
>+    item = this._getResourceForItem(aItemID);
>     var index = container.IndexOf(item);
>     var count = container.GetCount();
>     if (index < count) {

This should be removed, instead of modified.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 172829 [details] [diff] [review]
patch

1.0.1 is a security update release, and Firefox 1.0.1 has already shipped

Comment 34

[Serge Gautherie (:sgautherie)]

Benjamin, do you agree with my comment 32 which answers your comment 23 ?

Comment 35

[Brendan Eich [:brendan]]

I'm this close to removing the strict warning for redeclaration of var as var. 
It might flag confused attempts to use block locals in JS, but more likely, it
just whines about localized var redeclaration that makes code more readable and
makes the parts of a larger function stand alone better.

I've talked to Ben about this, and we agree that such warnings should not be
"fixed" by minimizing var declarations to the earliest point in the file, at
least not without module owner agreement.  I don't think that's forthcoming
here, so this bug may be WONTFIX.

I hope the few more serious warnings were fixed.

/be

Comment 36

[Serge Gautherie (:sgautherie)]

(In reply to comment #35)
> I'm this close to removing the strict warning for redeclaration of var as var. 
> It might flag confused attempts to use block locals in JS, but more likely, it
> just whines about localized var redeclaration that makes code more readable and
> makes the parts of a larger function stand alone better.

Well, I "like" the warnings as the language seems to work that way... :-|

> I've talked to Ben about this, and we agree that such warnings should not be
> "fixed" by minimizing var declarations to the earliest point in the file, at
> least not without module owner agreement.  I don't think that's forthcoming
> here, so this bug may be WONTFIX.

Not without owner agreement, agreed.
(Why would module owner agreement not come on this patch ?)

> I hope the few more serious warnings were fixed.

(my 2 cts)

Comment 37

[Serge Gautherie (:sgautherie)]

(In reply to comment #35)
> I'm this close to removing the strict warning for redeclaration of var as var. 
> 
> I hope the few more serious warnings were fixed.

(Bug 291868 was fixed ... Time to revive the current one !?)

Comment 38

[Steffen Wilberg]

Comment on attachment 172829 [details] [diff] [review]
patch

The variable redeclaration strict warning has just been removed by bug 291868,
so we can concentrate on the more severe warnings, if any.

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I think we should close this bug. There are other ones for the remaining strict
warnings: bug 291371 and bug 291265.

Comment 40

[Brendan Eich [:brendan]]

Per comment 39, closing.

/be