230104 - Web site using JavaScript broken when migrating from 1.4 to 1.6b (can't decode JavaScript from bogus charset)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Luke Ross]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6b) Gecko/20031210
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6b) Gecko/20031210

On waitrosedeliver.com their JavaScript based pages of items have somehow become
broken when I upgraded from Mozilla 1.4 to 1.6b (all using mozilla.org RPMs).
Previously the product lists were displayed correctly, but now they never have
any other items under them and a JavaScript error is logged. The pages require
you are logged in, I believe.

Reproducible: Always

Steps to Reproduce:
1. Log in to waitrosedeliver.com
2. Click "My Orders" in the top right
3. Click "Show mw all the prodicts I have ever ordered"

Actual Results:  
A series of green categories are displayed, with no items under them.

Expected Results:  
A series of green categories should be displayed with items under them.

I borrowed a friend's Mozilla 1.01 to check, and the site still works correctly
under the older versions. Sorry if this is an evangelism bug, but it seems odd
it has just stopped working on new versions.

Comment 1

[Luke Ross]

Attachment: Old behaviour

Product list displayed.

Comment 2

[Luke Ross]

Attachment: New behaviour

Categories shown as blank

Comment 3

[Luke Ross]

Apologies, some info I left off.

I don't know if it works under Mozilla 1.5 because I never used it.

The JavaScript error given on 1.6b is the rather unhelpful:

Error: getAProblem is not defined
Source File:
http://www.waitrosedeliver.com/wdeliver/javaScripts/display_lists_1.js?v=v35p3ugo
Line: 14

Comment 4

[Boris Zbarsky [:bzbarsky]]

Luke, could you look through the page and the scripts it loads for the
definition of the getAProblem function?  The error message just says that we
have no record of such a function ever being defined by the page....

Comment 5

[Luke Ross]

I'm not having much headway here, not helped by the auto-generated HTML and JS
being completely unreadable. Under both browsers, Save As Complete saves a set
of files, and both have a function declaration for 'getAProblem' (the JS file
they are in is identical between saves). The HTML does seem to differ but both
seem to use <script src="..."></script> to load the right JS file to draw the
categories.

It might be possible to reproduce the problem without logging in. I'm seeing
something similar if I go to http://www.waitrosedeliver.com/, click "Browse the
shop" in the top right, and select a category (eg. Product Aisles > Bakery and
Patisserie > Cakes). Previously the items were displayed in the right hand
frame, but now it says 'Cakes Products 1-30 of 122 "Waitrose" to "Waitrose"' but
listing no items.

I'll try and drag a finer toothcomb over the saved file as best I can, but any
essential changes might be lost because of the way Save As Complete and
JavaScript interact :-( I can email zip files of both versions if interested,
but they're likely to be around 1M.

Comment 6

[Jungshik Shin]

just hoping this might help a little...

The stylesheet used by 'browse the shop' page is probably at

http://www.waitrosedeliver.com//wdeliver/styleSheets/ns_6_common.css?v=v1hc3uf8

(the sniffing code is at 
http://www.waitrosedeliver.com//wdeliver/javaScripts/browser.js?v=utec4ji0 )

The URL cited in comment #3 uses 'escape' in two places ('escape' has changed
since 1.4)

Comment 7

[Hermann Schwab]

changing OS to ALL, as this is broken with win98 too:
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7a) Gecko/20040104

tested working: Mozilla 1.4.1, Mozilla 1.5, Mozilla 1.6a BuildID 2003110115
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6a) Gecko/20031030

tested as described in comment 5 looking for cakes, and got first 30 displayed.
1.4.1 was showing a missing bracket in lists.js, but I didn´t see it, and the
website was working. 1.5 and 1.6a didn´t show JS warnings or errors.

1.7a shows a lot of warnings, on every page. after selecting 'cakes' i get over
200 warnings of this one:
Warning: reference to undefined property treeMenu[i][j][k]
Source File:
http://www.waitrosedeliver.com/wdeliver/javaScripts/tree_menu_code.js?v=v0161hgg
Line: 322

Also some:
Warning: reference to undefined property treeMenu[i][j]
Source File:
http://www.waitrosedeliver.com/wdeliver/javaScripts/tree_menu_code.js?v=v0161hgg
Line: 321

Warning: reference to undefined property treeMenu[i]
Source File:
http://www.waitrosedeliver.com/wdeliver/javaScripts/tree_menu_code.js?v=v0161hgg
Line: 320

Comment 8

[Hermann Schwab]

I made local copys of the 'cakes'-page using 1.4.1 and 1.7a.
1.4.1 did save all the images, 1.7a had the cake names as contents in a
function, but did save less files and no images.
1.7a was displaying the local copy made with 1.4.1, so there is a regression.

the saved js was working with each other version, seems, that there were only
the  images not saved by 1.7a, and the main file display.jsp.html was 12kb vs.
107 kb.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The problem with http://www.waitrosedeliver.com/ appeared between build
2003-11-10-05 and build 2003-11-13-12 (I have no nightlies in that range). 
Possible culprits:  bug 221975 (not too likely), bug 217225, bug 224867 (neither
very likely either), bug 222346 (just checked over that patch and it seems ok
still), bug 219848, bug 44272 (my bet's on this one, esp. if the site
browser-sniffs and uses different escaping based on the result), bug 224306 (I
don't know enough to evaluate this one), bug 191699 (not likely).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Actually, in a debug build selecting "Cakes" gives me:

###!!! ASSERTION: Could not convert external JavaScript to Unicode!:
'NS_SUCCEEDED(rv)', file
/home/bzbarsky/mozilla/xlib/mozilla/content/base/src/nsScriptLoader.cpp, line 856

The charset involved is 'ISO8859_1'; note that when you load the URI
http://www.waitrosedeliver.com/wdeliver/servlet/JSPs/shop/flags_javascript_creator.jsp
the server sends the header:

Content-Type: text/html;charset=ISO8859_1

If I add the line:

iso8859_1=ISO-8859-1

to charsetalias.properties, the page shows all the images.

So we have the following options:

1)  Make this code handle bogus server charsets like it used to (it would fall
    back on the document, etc, if it failed to get a resolved alias for what the
    server sent)
2)  Add this string as an alias for ISO-8859-1
3)  Just evangelize this site to send something sane.

We already have aliases of "iso_8859-1" and "iso8859-1", so it's a short step to
"iso8859_1", right?  ;)

In any case, this is not a JS engine bug, and I would say that we should log the
encoding conversion failure to the JS console; that would have made debugging
this a snap.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I should clarify that this is all for the issue in comment 5 that does not
require logging in, but I bet the same will help the rest of the site.

Comment 12

[Boris Zbarsky [:bzbarsky]]

One other option would be to canonicalize aAlias by replacing all '_' with '-'
in nsCharsetAlias2::GetPreferred (right where we ToLowerCase() it).  This may be
the best solution in general... (and update/remove the charsetalias.properties
entries that use '_').

Comment 13

[Hermann Schwab]

working: BuildID 2003111209
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6b) Gecko/20031112

broken: BuildID 2003111309
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6b) Gecko/20031113

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yeah, that's the time range the bug 222346 fix landed in.

Comment 15

[Luke Ross]

Comments 10 and 11 are correct - I hacked a proxy to detect and fix the broken
Content-Type (crudely) and the "show all products" page as shown in the
screenshots starts working again.

Comment 16

[Jungshik Shin]

re comment #12
I'm for it assuming we can do it w/o perf. loss. BTW, iirc, one canonical name
we use has '_', Shift_JIS. Anyway, we have to write to the web master to fix
their problem. 

Comment 17

[Simon Montagu :smontagu]

I think nsScriptLoader should at least do _some_ fallback when
GetUnicodeDecoder() fails, or maybe GetUnicodeDecoder() itself should fallback
to ISO-8859-1 if GetCharsetAlias() fails. 

Comment 18

[Jungshik Shin]

 

> nsScriptLoader should at least do _some_ fallback when
> GetUnicodeDecoder() fails,

  I agree. Alternatively, GetUnicodeDecoderRaw can fallback not because
GetCharsetAlias fails (it never fails but it just returns the original if
there's no alias) but because there's no decoder for the 'resolved' charset.
However, there may be some consumers that rely on the current behavior. So,
probably just adding a fallback in the script loader seems 'safer' than the
alternative.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yeah, I don't like GetUnicodeDecoderRaw doing a fallback....  For comment 16,
does the Shift_JIS thing matter for purposes of the key string we use in the
properties file?  I don't know this code all that well...

I suppose just changing nsScriptLoader to fallback to ISO-8859-1 is ok with me.

Comment 20

[Luke Ross]

So is the fix here going to be to the effect of?

mozilla/content/base/src/nsScriptLoader.cpp 
812     if (NS_SUCCEEDED(rv) && charsetConv) {
813       rv = charsetConv->GetUnicodeDecoder(characterSet.get(),
814                                           getter_AddRefs(unicodeDecoder));
+++       if (NS_FAILED(rv)) {
+++         rv = charsetConv->GetUnicodeDecoder("ISO-8859-1",
+++                                           getter_AddRefs(unicodeDecoder));
+++       }
815     }

Or is something more clever needed?

Comment 21

[Jungshik Shin]

Attachment: patch

Luke, sorry for the delay. This is the same as yours except that 'Raw' version
is used because 'ISO-8859-1' is canonical (no alias resolution is necessary). I
guess this is too late for 1.6. You should really write to the web master of
the site to  use 'ISO-8859-1'. Just adding one line to their jsp files will do
that. 

re: Shift_JIS,	I'd love to change it to Shift-JIS (as internal keys, actually
shiftjis, iso88591, euckr, utf8, koi8r, windows1252, etc would be best), but
things are a bit more complex because that is the preferred MIME name in the
IANA MIME charset registry and we want to use it for outgoing emails / html
documents (produced by Composer).

Comment 22

[Luke Ross]

I've sent Waitrose a message explaining the issue, and I'll add a comment here
when I get a reply.

Comment 23

[Jungshik Shin]

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

asking for r/sr.

btw, I fixed the indentatio n in my tree and added the following comment:

 // fall back to ISO-8859-1 if charset is not supported. (bug 230104)

Comment 24

[Boris Zbarsky [:bzbarsky]]

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

Yeah, looks good.

Comment 25

[Simon Montagu :smontagu]

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

r=smontagu

Comment 26

[Jungshik Shin]

thanks for r/sr. patch checked into the trunk, but it won't be landed in 1.6 (at
least in the 1.6.0). So, Luke, they have to fix their bug (hope they will listen
to you :-))

Comment 27

[Luke Ross]

Just for the record, I got the following reply from waitrosedeliver.com (the
reference to Netscape was because I referred to Mozilla as being the Netscape
development version, to avoid the "we don't support strange browsers" response):

Dear Luke,
                                                                                
Thank you for your e-mail.
                                                                                
Our Technical team were very grateful for all your comments and help regarding
the problems with the Netscape browser.
                                                                                
We are going to be making the changes that you suggested on our next update in
early February.

...

Regards,
                                                                                
Nicky Lyons
                                                                                
Waitrose Customer Service.