77699 - javascript error in quicksearch.js

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P2)

Description

[Stephen Lee]

I expect this is a browser bug rather than a problem with quicksearch.js, as it 
works on a different machine with windows 2000 and IE 5.5, but I don't know 
enough about javascript to be sure, and quicksearch.js may be able to work 
around it anyway.

I'm using Internet Explorer 5 on a windows NT machine.
User Agent: Mozilla/4.0 (compatible; MSIE 5.0; Windows NT; DigExt)

If I go to the main index page, either on our local bugzilla installation 
(2.12), or the main bugzilla site (2.11), and type (e.g.) "bugzilla 
quicksearch" into the quicksearch box, it says:
 Line: 605
 Char: 5
 Error: 'undefined' is undefined
 Code: 0
 URL: http://bugzilla.mozilla.org/

If I OK the error, it goes to
http://bugzilla.mozilla.org/show_bug.cgi?id=bugzilla+quicksearch
and offers to search by bug number.

It works fine (no error) if I type a bug number in instead of search terms.

Comment 1

[Jacob Steenhagen]

This is probably related to bug 71503.

Comment 2

[Stephen Lee]

Fault remains when I apply the patch from bug 71503 (attachment 27346 [details] [diff] [review]) to our 
local bugzilla, but line number is now reported as 632 rather than 605.

Comment 3

[Stephen Lee]

It would appear from a little experimentation that the problem is that my 
javascript is expecting "undefined" to be a variable name rather than a magic 
value (which I assume it is supposed to be).

If I add something like the following line to the top of the file:

 var undefined="!!!undefined!!!";

it makes the error go away, and appears to function correctly.

Comment 4

[Andreas Franke (gone)]

All your QuickSearch bugs are belong to me :-)

Comment 5

[Andreas Franke (gone)]

From http://www.devguru.com/Technologies/ecmascript/quickref/undefined.html :
>
> NOTE:
> 
>In Internet Explorer, if you attempt to utilize an undefined variable, you will
>get a runtime error message 

So I don't really know how to fix this. Maybe some client-side browser-sniffing
code is needed, but unless someone tells me exactly how to recognize the browser
in JavaScript and what the correct code for that browser is (I doubt that
"!!!undefined!!!" really gives us the same magic, it's proably just a string), I
can't help. (I don't have IE easily available here, since we are in a Linux
environment.) Thus giving the bug back to tara.

Comment 6

[Stephen Lee]

I think in the way that "undefined" is being used - "if ... return undefined" 
in the function and "if (function(...) != undefined)" by the caller, any 
unlikely value should work. Although I don't know javascript well enough to 
study the code to determine other causes of "undefined" being returned.


Further errors on that browser occur when using the "magic" keywords such 
as "ALL", "DUP", "FIXED", "OPEN" etc.

e.g. for "ALL" with both of my mods,
Line: 44
Char: 9
Error: Object doesn't support this property or method
Code: 0

for "OPEN", same but line 324, for others the same error occurs but on 
different lines.

In each case it corresponds to a "shift" or an "unshift". e.g.
for ALL: 
function add(s, l) {
    if (! member(s, l)) {
        l.unshift(s);     //  <<<< ERROR on this line
    }
}

for "OPEN":
    } else if (word[0] == "OPEN") {
        // special case: search for open bugs only
        word.shift();     //  <<<< ERROR on this line

Presumably IE5.0 doesn't know about the shift and unshift functions.

I'd guess it should be possible to implement an equivalent operation using 
simpler instructions.


Since IE is not the primary target browser, and IE users can upgrade to IE5.5 
to get this functionality, and AIUI (from bug 70907) the long-term aim is to 
make it server-side, you may want to WONTFIX this, or "fix" it by having the 
javascript output a suitable form of words to the document when running on an 
older browser to warn the user that the quicksearch may not work correctly and 
suggest upgrading.

Alternatively, if I manage to patch our local copy to work, I'll post the patch 
here.

Comment 7

[Stephen Lee]

Attachment: Patch to remove javascript errors on IE5

Comment 8

[Stephen Lee]

The attached patch makes quicksearch.js work on IE5. This is done by replacing 
all explicit references to "undefined" with "magic_value".

Also all calls to shift and unshift go through an indirect function.

There is a variable is_IE5 which determines whether the magic_value is 
initialised to undefined to to a suitable arbitrary string value, and whether 
shift and unshift are called directly or implemented longhand.

On IE 5.0, it only works when the variable is true, on IE 5.5, it works with 
the variable set either way. Ideally this needs to be initialised based on 
suitable browser detection code, but I'm not familiar enough with javascript to 
know how to do this.

Comment 9

[Stephen Lee]

Attachment: Updated patch, including bugfix to original patch, and browser detection

Comment 10

[Stephen Lee]

The updated patch attached does browser detection based on whether 
navigator.appVersion contains the string "MSIE 5.0". This is wrong because this 
would also affect older versions of other browsers.

Ideally we want some way of detecting whether the javascript interpreter 
understands the concepts of "undefined" and "shift", without generating an 
error. Any javascript gurus out there know how to do this?

Comment 11

[Andreas Franke (gone)]

Thanks for the patches! If you want to take the bug, feel free.

I applied the patch (the first one) on top of the patch from bug 71503, and it
worked fine. There were some strange linefeeds (^M before the newline); I tried
to remove them all, but I'm not sure if I missed some. Well, now that you have
an updated patch, it doesn't make much sense to attach the merged one anymore,
so I'll let you do this... ;)
Anyway, some more comments (on the first patch): 
* You are right that there is no need for an "undefined" magic. You can simply
use any string that isn't a legal value in the usage contexts, so "---" probably
works well, too.
* You don't even need to rename it to "magic_value". You can still use
"undefined" and simply add a statement
	var undefined="---" 
and it still works in Netscape 4.x.
* Some queries seem to work fine, but the query for all NEW bugs (simply "NEW")
gives a Javascript error in Netscape: 

JavaScript Error:
http://bugzilla.mathweb.org:8000/quicksearch.js, line
371:

w0 has no properties. 

Can you try this query (and maybe other examples from quicksearchhack.html) and
see if they work for you?

Comment 12

[Stephen Lee]

Re: your JavaScript Error:
http://bugzilla.mathweb.org:8000/quicksearch.js, line 371:

That was the bug that the updated patch fixed. I had missed the "return" from 
the IE5 version of the shift function.

Apologies for the CRLFs. I edited the patch file on a Windows machine after 
generation to remove other site-specific changes. I'll try to find an easy way 
to remove them for any future patches I submit.

There's not much point me taking the bug, as I have no way of accessing CVS.

I'll create a merged patch and attach it shortly...

Comment 13

[Stephen Lee]

Attachment: Combined patch for this bug and bug 71503, with CRs stripped

Comment 14

[Andreas Franke (gone)]

Attachment: slightly edited fix, against patched version from bug 71503

Comment 15

[Andreas Franke (gone)]

Attachment: same as above, but diff -u against CVS head, including 71503

Comment 16

[Andreas Franke (gone)]

I made only a small modifications to your patch:
- added you as a contributor, is this ok?
- renamed "magic_value" to "no_result"
  and "!!! magic undefined value !!!" to "---"

Stephen, if you are fine with these changes, then patch 35006 is ready for
review and checkin, as far as I'm concerned. I tried a few testcases; they are
all working fine, and I can't think of a reason why there should suddenly be
problems in any other cases.

Note that I don't have CVS access either. Targeting at 2.14 though.

Note to potential reviewers: Currently QuickSearch seems to be broken on IE5. It
would be very nice if fixing completely broken functionality could be seen as an
exception for 2.14, especially since there are some estimations that 2.16 is
only expected to be released at the end of the year, and there is no reason to
keep IE5 users from using this tool till then. (It may even happen that there
are no IE5 users left then because they have all upgraded...)

For maintainability reasons I strongly want the patch for bug 71503 included as
well. Since I have it in my bugzilla trees, it is really painful to apply and
create patches without it. Please have mercy and check it in, too. Thanks.

Comment 17

[Stephen Lee]

Looks like a much more sensible variable name and value  8-)

I had originally assumed that the value had to be something the user would 
never type and that using "---" would break searches such as
"NEW milestone:---", but apparently it doesn't, so I'm happy with attachment 
35006 [details] [diff] [review] for someone to review and check in.

Comment 18

[Dave Miller [:justdave]]

I'll take Andreas's word on this one since he wrote the original quickseach.js.

attachment 35006 [details] [diff] [review] is checked in.

Comment 19

[Dave Miller [:justdave]]

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

Comment 20

[Stephen Lee]

The following "patch" or an equivalent needs to be applied to make it work on 
IE4 as well:

-var is_IE5=(navigator.appVersion.search(/MSIE 5.0/g)>0);
+var is_IE5=(navigator.appVersion.search(/MSIE 5.0/g)>0) ||
+           (navigator.appVersion.search(/MSIE 4/g)>0);

Maybe there are other browsers that also require this workaround?

Comment 21

[[e-mail]]

Tests:
Mozilla and NS4 define "undefined" as null but also allow "var undefined".
NS3, IE3 and IE4 don't define "undefined". "var undefined;" defines it as null.
HOWEVER NS3 wouldn't let me compare undefined to itself! Only to null.
So the correct patch is to abandon "undefined" and use null instead.

Comment 22

[Stephen Lee]

The existing patch already checked in makes it use undefined only indirectly, 
via a no_result variable. This appears to work equally well when any value is 
used (e.g. "---" as done in IE5.0 (and IE3,IE4) compatibility mode).

There is also the other issue of the shift / unshift instructions, which were 
replaced with function calls for IE5.0 compatibility, with a different 
implementation based on browser type.

As the functionality is the same either way, perhaps we should strip out the 
browser check and use the IE5.0 compatible code in all cases. Does anyone know 
what the pros and cons of this vs the original method on a browser that can do 
both? Presumably the compatibility way is slower, but quicksearch.js is hardly 
very processor intensive anyway.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

This can surely move to the 2.16 milestone since it isn't security-related and
is mostly fixed anyway, no?

Comment 24

[Dave Miller [:justdave]]

hmm, a fix for this was checked in during the 2.14 cycle.  It should stay at 2.14 
in order to preserve the changelog.  If we want to put the additional fix off for 
2.16 we should open a new bug and re-resolve this one.

Comment 25

[Dave Miller [:justdave]]

removing patch/review keywords, since this needs code.  I'll let this sit in case 
Andreas gets around to it before we tarball 2.14.  If not, I'll close it again so 
it stays on the 2.14 changelog and open a new bug targetted at 2.16. 

Comment 26

[Stephen Lee]

Attachment: Patch against current tip to use compatibility code on ALL browsers

Comment 27

[Stephen Lee]

The above patch makes quicksearch use the compatibility code from my earlier 
patch on ALL browsers rather than just IE5, so that rather than potentially 
having to list every possible browser using older versions of javascript, the 
older code is "just used".

Are there any problems this might cause on browsers which have the built in 
primitives, or is this ok to go to the review and checkin stage?

Comment 28

[Andreas Franke (gone)]

Attachment: FOR REVIEW ONLY: diff -wu for the patch above

Comment 29

[Andreas Franke (gone)]

Looks good to me at first glance. Will have a closer look tomorrow, if time
permits. I vote for 2.14.

Comment 30

[Andreas Franke (gone)]

OK, I have installed the patch (attachment 42840 [details] [diff] [review]) on my local bugzilla, and
quicksearch queries still seem to work with Netscape 4.61 on Linux.

r=afranke. I think the risk of taking this is approx. zero, and with it we
should be on the safe side wrt. browser compatibility problems. Please take this
now.

Stephen, thanks for the latest patch :-)

Comment 31

[Dave Miller [:justdave]]

a= justdave

checked in.

Comment 32

[Dave Miller [:justdave]]

Moving to Bugzilla product