58436 - javascript strict warnings in query.cgi

Status: VERIFIED FIXED

(Bugzilla :: Query/Bug List, defect, P3)

Description

[Henrik Gemal]

The javascript output from query.cgi has a lot of javascript script warnings.

Some of them:
JavaScript strict warning:
http://bugzilla.mozilla.org/query.cgi line 783: reference to undefined property
vsel[v]

JavaScript strict warning:
http://bugzilla.mozilla.org/query.cgi line 817: reference to undefined property
tmsel[tm]

Expected:
No javascript strict warnings...

Comment 1

[Henrik Gemal]

Attachment: Diff to fix all javascript strict warnings from output of query.cgi

Comment 2

[Tara Hernandez]

My javascript console is not complaining.  What browser are you using?

Comment 3

[Henrik Gemal]

Mozilla with:
user_pref("javascript.options.strict", true);

Comment 4

[Henrik Gemal]

what are the chances that the patch is checked in?

Comment 5

[Henrik Gemal]

please: someone review and checkin!
Could someone please explain what I need to do, for something like this getting
checked in? I hate creating patches that doesn't get used.

Comment 6

[Andreas Franke (gone)]

Anyone else out there with CVS access to Bugzilla?

Comment 7

[Andreas Franke (gone)]

If there is no response, you can post to n.p.m.webtools. 

Comment 8

[Andreas Franke (gone)]

Gerv, do you know the official way to get a review for a bugzilla patch?

Comment 9

[Andreas Franke (gone)]

These are all bugzilla people I'm aware of. Are there more?

Comment 10

[Dave Miller [:justdave]]

I'm not a JavaScript expert, but what I see changed in this patch makes sense to 
me (I've had to do similar things in Perl to avoid dealing with undefined 
variables).

Comment 11

[Dave Miller [:justdave]]

I suppose I should clarify...  I'm willing to check it in if we get a couple more 
people who are more familiar with JavaScript that will vouch for it not breaking 
anything.  Having it tried out with a variety of browsers on someone's system 
will be a plus.

Comment 12

[Gervase Markham [:gerv]]

Looking at it, it seems very unlikely indeed to break anything... but it should 
go on Landfill first.

Gerv

Comment 13

[Dave Miller [:justdave]]

Bug 61110 looks like a duplicate of this one.  It also has a patch, and the 
patches are not identical...

Comment 14

[Dave Miller [:justdave]]

Strike that last comment.  61110 is indeed the same bug, but it's for tinderbox, 
not Bugzilla.   :-)  similar patch, but for a different program.

Comment 15

[Zach Lipton [:zach]]

I posted this patch on nuevaschool.org/cgi-bin/zach/bugzilla/index.cgi (you 
must use .cgi, not .html for it to work) since landfill is messed up and 
[e-mail] asked me to. I'll look at this in a bit and see what I 
get.

Comment 16

[Zach Lipton [:zach]]

The patch is up and it works fine, but I don't seem to notice the difference. 
I turned on the pref in my pref file, but I don't get the error with the 
bugzilla.mozilla.org version either.

Comment 17

[Zach Lipton [:zach]]

Sorry for the problem. I had to clbr my install, I reposted this patch now.

Comment 18

[Henrik Gemal]

what's the status here?

Comment 19

[Gervase Markham [:gerv]]

Putting it on Tara's 2.12 radar might produce some traction...

Gerv

Comment 20

[Tara Hernandez]

Alright, finally saw this (thanks for the heads up Gerv).

I still get one warning, which is:

reference to undefined property f.target_milestone
line 121

To be honest after a preliminary inspection of the code I'm not sure why it's
bitching about this (especially at that line number), but I'm willing to either
wait for you to have a look or take it as is with all the other warnings removed.

Comment 21

[Henrik Gemal]

I'm trying to fix this, so wait a day and I'll post a patch that works...
Is there a place, with a newer query.cgi, where this patch has been applied?
just to make sure that all js warnings go away....:)

Comment 22

[Henrik Gemal]

Attachment: patch to fix all kwown js strict warnings

Comment 23

[Tara Hernandez]

I've got an up to date install that unfortunately is not publically available 
yet, but I'll let you know what I find.  (Thanks for all your help, by the way)

Comment 24

[Tara Hernandez]

All good.  Thanks again.

Comment 25

[Henrik Gemal]

this one hasn't been checked in yet, right?

Comment 26

[Jacob Steenhagen]

It seems that part of this bugfix is causing errors in Netscape 4.7 (for
[e-mail],Mac OS 9.1) and IE 5 (for me, Win2k).  It seems to be:
-    if (f.target_milestone) {
+    if ("target_milestone" in f) {

As changing that line back to f.target_milestone makes the errors go away. 
Please note that these aren't strict warnings being seen, they are full-fledged
"this script won't run" type errors.  I think for the time being it'd be better
to change this particular line back to it's pre-fix state and then see what can
be done about the strict warnings later.

Comment 27

[Chris Yeh]

It looks like it's in the trunk. I'm trying to find IE 5/Win2K to test with.

Comment 28

[Chris Yeh]

Grrr. I don't have access to a Win2K machine so I can't test that changing the 
line back to f.targetmilestone works. Can I get a witness from the congregation 
so this fix can be checked in?

Comment 29

[Jacob Steenhagen]

I know changing it back to f.target_milestone makes the script errors in IE on
Win2k go away... and Dave said in IRC that it worked for him w/NS on the Mac.  I
don't know how this effects the strict warnings in Mozilla (as I've never used
the JavaScript console).

Comment 30

[Chris Yeh]

changed to f.target_milestone

Comment 31

[Sidney Markowitz]

I'm running into problems with this using IE 5.0 on Win NT. The if
("target_milestone" in f) syntax isn't accepted, as has been pointed out. But
the if (f.target_milestone) leads to a different script error if target
milestones are disabled in the project. That was what bug #39044 was about and
why the conditional was added in the first place. But I don't see a syntax that
allows this in IE JScript.

The change that works on my system is to split the javascript text into three
parts and only conditionally add the section referring to target_milestone by
putting it inside of

if (Param("usetargetmilestone")) {
  $jscript .= q{
      if (f.target_milestone) {
  ...
  }
}

If someone knows a way that IE JScript can test for existence of an element of
an object that doesn't error out when the element is not defined, you can use
that instead.

Comment 32

[Chris Yeh]

Reopening bug, still breaks on IE 5.0

Comment 33

[Chris Yeh]

Attachment: patch to wrap f.target* around param

Comment 34

[Chris Yeh]

Can someone please apply this patch and test it with IE 5.0, I.E 5.5, 
Communicator 4.5 or greater, and Mozilla .8?

Comment 35

[Sidney Markowitz]

It still doesn't work in IE 5.0 under Windows NT. Notice that in my last 
comment I showed the if Param("usetargetmilestone") being followed by a 
$jscript .= q{ line. The Param test is in Perl, not in the JScript, and I meant 
to show the JScript being split into three parts so none of the 
target_milestone stuff even appears on the web page if target_milestones are 
turned off in the preferences.

Note that the test case for this is when you turn off use of target milestones 
in the preferences.

If nobody gets to this before I can in a few hours I'll submit a patch that 
shows it all exactly.

Comment 36

[Chris Yeh]

You're right, I don't even know what I was thinking when I did that.

Comment 37

[Sidney Markowitz]

Attachment: Patch to keep target_milestone out of javascript when usetargetmilestone parm is off

Comment 38

[Sidney Markowitz]

Ok, I've tried my patch with IE 5.0 under WinNT and Netscape 4.76 in Linux, 
with use target milestones both on and off in the project preferences.

Comment 39

[Dave Miller [:justdave]]

moving to real milestones...

Comment 40

[Chris Yeh]

patch verified and checked into trunk

Comment 41

[Gervase Markham [:gerv]]

VERIFIED no strict warnings from query.cgi. Loads from quicksearch.js, though... 
bug 71503 opened.

Gerv

Comment 42

[Jacob Steenhagen]

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

Comment 43

[Jacob Steenhagen]

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

Comment 44

[Henrik Gemal]

has this been put into http://bugzilla.mozilla.org/query.cgi ?

Comment 45

[Jacob Steenhagen]

[e-mail]: Yes.  Although JavaScript has changed again since this was added
due to bug 72379.

Comment 46

[Henrik Gemal]

console is again flooded when going to http://bugzilla.mozilla.org/query.cgi

Comment 47

[Henrik Gemal]

if you're running with strict js warns on mozilla becomes unavailable because 
of *all* the warnings the query.cgi javascript:

Warning: reference to undefined property tmsel[tm]
Source File: http://bugzilla.mozilla.org/query.cgi
Line: 915

Comment 48

[Henrik Gemal]

Attachment: diff to fix all known warnings.

Comment 49

[Henrik Gemal]

any chance for a checkin????

Comment 50

[Dave Miller [:justdave]]

checkins are frozen for the 2.14 release.  As soon as 2.14 ships we'll be making 
an effort to get all the patches that are sitting around fixed up and checked in.

Comment 51

[Alfonso Martinez]

This is not a strict warning, but an error that I get with win32 2001071104 
build after loading http://bugzilla.mozilla.org

Error: f.product has no properties
Source File: http://bugzilla.mozilla.org/query.cgi
Line: 830

Comment 52

[Henrik Gemal]

The real multiplatform multibrowser solution to the:
("target_milestone" in f)
ala checks seems to be:
(typeof(f.target_milestone) != 'undefined')
works in IE5.5, IE5.0, Mozilla and Netscape 4

Comment 53

[Henrik Gemal]

Attachment: final diff....

Comment 54

[Dave Miller [:justdave]]

Taking all of cyeh's Bugzilla bugs.

Comment 55

[Christopher Aillon (sabbatical, not receiving bugmail)]

Mass moving to new product Bugzilla...

Comment 56

[Jacob Steenhagen]

After recent query.cgi JavasScript changes (see bug 96534), the only console
warnings I see are:

Warning: assignment to undeclared variable i
Source File: http://bugzilla.mozilla.org/query.cgi
Line: 278

Warning: assignment to undeclared variable comp
Source File: http://bugzilla.mozilla.org/query.cgi
Line: 122

Warning: assignment to undeclared variable bitem
Source File: http://bugzilla.mozilla.org/query.cgi
Line: 175

Warning: assignment to undeclared variable aitem
Source File: http://bugzilla.mozilla.org/query.cgi
Line: 179

-----
Please note that I really don't know how to use the JavaScript console, I simply
went into prefs.js and added:
user_pref("javascript.options.strict", true);
then fired up Mozilla, went to Tasks/Tools/Javascript Console, loaded
http://bugzilla.mozilla.org/query.cgi, then clicked on a few of the products.

Comment 57

[Christian Reis]

Attachment: remove JS warning from current query.cgi code

Comment 58

[Christian Reis]

Thanks for the pointer, Jake. I've attached a fix that is quite simple but
solves all warnings I can see in the code. Looking for an r= --- Christopher?

Comment 59

[Christian Reis]

Thanks for the pointer, Jake. I've attached a fix that is quite simple but
solves all warnings I can see in the code. Looking for an r= --- Christopher?

Comment 60

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 49072 [details] [diff] [review]
remove JS warning from current query.cgi code

>Index: query.cgi
>@@ -539,10 +540,10 @@

>-    if ( !f ) {
>+    if ( ( !f ) || ( ! ( "product" in f ) ) || ( ! f.product ) ) {


This should be:   if (!f.product) {

|"product" in f| is the same as |f.product| but needs to iterate through 
the objects properties whereas f.product just looks for the property
And if f.product returns false, we don't care whether or not f is
there.  If it returns true, then f is obviously true.

Comment 61

[Christian Reis]

Christopher, I'm afraid you're wrong :)

|"product" in f| is NOT the same as |f.product| -- if f is set but
f.product not, the second test generates a warning, whereas the first one
does not. 

At times, f also comes in false in other browsers (Opera, IIRC) during
which the form exists but is not ready. For this reason, I need to check
for it too. 

Is that your only complaint? Perhaps I should comment that part a bit
more? :)

Comment 62

[Christopher Aillon (sabbatical, not receiving bugmail)]

I wasn't aware that some browsers horked that. Really those should be bugs that
are in the other browsers but I guess we need to account for them anyway.

Is there any reason then that we need all 3 checks?  Even with your comments, it
makes sense for only:  if (!f || !f.product).  Is there some other horkage going on?

Comment 63

[Christian Reis]

Christopher, the onSelect() event is generated a couple of times before
the form and controls are "ready" AFAICT. If I leave the checks out, a
series of warnings and errors are generated:

Warning:  reference to undefined property f.property
Warning:  reference to undefined property f.property
Warning:  reference to undefined property f.property
Warning:  reference to undefined property f.property
Error: f.product has no properties

I should point out that these could be browser bugs (why is onSelect()
being called if we're not selecting things) but other browsers present
some of them too AFAICT.

I've decided, however, to leave out the | if ( ! ( 'product' in f ) |
check now because this test seems unimplemented in NS4. :( New patch and
bug being filed for the extra events.

Comment 64

[Christian Reis]

Attachment: v3: change onload to use form name instead of number, avoid future breakage. 0 warnings.

Comment 65

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 49229 [details] [diff] [review]
v3: change onload to use form name instead of number, avoid future breakage. 0 warnings.

r=me

Comment 66

[Jacob Steenhagen]

Comment on attachment 49229 [details] [diff] [review]
v3: change onload to use form name instead of number, avoid future breakage. 0 warnings.

I don't see any more warnings...
r= jake

Comment 67

[Jacob Steenhagen]

-> Patch author

Comment 68

[Jacob Steenhagen]

Checked in.

Comment 69

[Henrik Gemal]

verfified fixed..