144768 - Selecting multiple products on query page causes script error in IE

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Zack Erhart]

When selecting multiple products on the query page in Bugzilla 2.16rc1, the 
following script error occurs in IE:

Line 170
'undefined' is null or not an object

Tested in IE6.0 and IE5.0 at http://landfill.bugzilla.org/bugzilla-
tip/query.cgi.

No errors seen when using Mozilla or NS4.x.

This error does not occur in the 2.15 version running on b.m.o.

Comment 1

[Bradley Baetz (:bbaetz)]

Line 170 is a blank line on the tip... This works in mozilla.

Comment 2

[Jouni Heikniemi]

The Javascript errors' line numbers sometimes differ from the actual source code
line numbers, so it's hard to say what's really going on there without having
the appropriate source.

WFM in bugzilla-tip, IE 5.0, WinNT4.

Reporter: if you still can make this happen, could you save the view source
output and attach it to this bug?

Comment 3

[Zack Erhart]

Line 170 is blank for me also. When looking at the problem in the debugger, the 
error is actually occuring on line 171. 

The problem is that although the debugger indicates that the 'a' array contains 
only two elements, a.length is 3. Thus, aitem is undefined at #171 when aitem 
gets assigned to be a[2].

This appears to be because the 'cpts' (and other) JS arrays that are created in 
query.cgi have an extra comma at the end. When I view the source generated by 
query.cgi, I see the following:

cpts[1] = [ 'Comp1', 'comp2', ];


FWIW, here is the stack trace:
merge_arrays (#171)
updateSelect (#106)
selectProduct (#276)
JScript - queryform anonymous function

Comment 4

[Jouni Heikniemi]

Well, that was a nice bug report. :-)

Gerv, you've touched this file recently. What is the coolest way fix this in
templates, since join cannot be trivially used?

Comment 5

[Gervase Markham [:gerv]]

Yes, excellent bug report. Hmm. We have several options:

1) JS filter the array elements before looping to print them. Then we can use
join().
2) Use some JS function to chop the last element off the array.
3) Do some sort of [% ", " UNLESS loop.last %] thing in the templates.
4) Stop supporting IE.

Gerv

Comment 6

[Bradley Baetz (:bbaetz)]

I prefer 3 - its the simplest, most obviously-correct, fix.

Is this an IE JS bug?

Comment 7

[Jouni Heikniemi]

I agree with bbaetz, solution 3 sounds the most sensible.

As for this being an IE JS bug, I don't know. A quick glance at ECMA-262 section
11.1.4 would indicate that IE does the right thing, but wouldn't know without
further examination. Guess this isn't that relevant since the fix is so easy,
though.

I can take a look at this tonight if nobody else does it by then.

Comment 8

[Gervase Markham [:gerv]]

I'm away this weekend, so go for it :-)

Gerv

Comment 9

[Jouni Heikniemi]

Oh well. Taking per comment 8.

Comment 10

[Bradley Baetz (:bbaetz)]

Hmm. Yeah, this looks like a moz js bug; I'll file one later this evening.

Comment 11

[Bradley Baetz (:bbaetz)]

actually, rereading the spec, this does look like an ie bug. from 11.1.4, we
have the production:

ArrayLiteral: [ ElementList, Elison (opt) ]

The opt elision bit is missing, so the rules have:

(2) Evaluate Elison, if not present, use the numeric value zero.

and then step 4 sets the length of the array to the length of ElementList +
Result(2)

This does seem to contradict the second paragraph of 11.1.4, though.

/me will ask people on irc

In any event, if IE does this differently we should just avoid teh situation.

Comment 12

[Bradley Baetz (:bbaetz)]

... and I've just reread theat second paragrpah for a thrid time, and now I do
think its an ie bug ;) Bleh...

Comment 13

[Jouni Heikniemi]

Attachment: Patch v1

Use fix option 3 per comment 5.

Comment 14

[Jouni Heikniemi]

Ok, there you go. Proposing 2.16, as it's an ugly bug and a trivial fix. 

I leave it up to bbaetz to decide whether to file a js bug report on some
browser. ;-)

Comment 15

[Bradley Baetz (:bbaetz)]

Comment on attachment 84060 [details] [diff] [review]
Patch v1

r=bbaetz, although I can't test it with ie

Comment 16

[Gervase Markham [:gerv]]

Comment on attachment 84060 [details] [diff] [review]
Patch v1

r=gerv. 

Gerv

Comment 17

[Gervase Markham [:gerv]]

Checked in on trunk and branch.

Checking in ./template/en/default/search/form.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/form.html.tmpl,v
<--  form.html.tmpl
new revision: 1.2.2.2; previous revision: 1.2.2.1
done

Checking in ./template/en/default/search/form.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/form.html.tmpl,v
<--  form.html.tmpl
new revision: 1.4; previous revision: 1.3
done

Gerv