152814 - detecting BOM when loading script

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[David]

When a unicode javascript file is included, i have the following error in the
javascript console :

Error: illegal character
Source File: /login.js
Line: 1
Source Code:
��/

Comment 1

[Christopher Hoess (gone)]

Is the Javascript file being served with the right character set from the server?

Comment 2

[Phil Schwartau]

A unicode decoder problem? Not sure if this is Parser or International,
but I don't think this is JS Engine. Reassigning to International. 

[e-mail]: is there a URL we can go to that shows the problem?
Without a testcase or a URL, we won't be able to work on this; thanks -

Comment 3

[David]

unfortunatly the file is inside the intranet .
actually all the js files in my application are included and stored in unicode..
the main page which include them is encoded in ISO-8859-1... the error occurs
for each file inclusion..
to reproduce the bug, one just have to make a simple html page encoded with
ISO-8859-1 that include a javascript file (even empty) stored in unicode.

Comment 4

[Teruko Kobayashi]

I tested this in 6-21 branch Win32 build.  I could reproduce this. 
The beggining of the UTF8 js file caused the problem.  

Comment 5

[Teruko Kobayashi]

Attachment: html file

Comment 6

[Teruko Kobayashi]

Attachment: UTF8 JS file

Comment 7

[Teruko Kobayashi]

Attachment: Javascript console

Comment 8

[Teruko Kobayashi]

When I run the attached html file (id=88692) which included UTF8 file (id = 88693),
I got the error message in Javascript console (id=88694). 

David, is this what you see? 

Comment 9

[David]

yes teruko. it's exactly the same error i got, due to the fact that js file is
stored in UTF-8

Comment 10

[Roy Yokoyama]

If I manually change the attached html file from <ISO-8859-1> to <UTF-8> from
browser menu; then JS works fine. I guess the decoder is respecting the doc
charset.

Comment 11

[Frank Tang]

what happen is the html is in iso-8859-1 and the js file is UTF-8 file without
any labeling. (http charset)
So the browser assume the js file is in the same encoding of the html and try to
load it with iso-8859-1 converter. Since this utf-8 js file is created by window
notpad, it generate 3 bytes of BOM in utf8 in the beginning. This make the JS
engine think this is not a valid file.

What could we do? I think one thing we could do is in the code which convert JS
into unicode. look at the first several bytes, like what we do in html parser.
And detect UTF16 BOM or UTF-8 BOM

Reassign this to shanjian
nsbeta1+ for m1.2final 

Comment 12

[Daniel Wang]

setting charset solves the problem:
<script type="text/javascript" charset="UTF-8" src="test.js"></script>

unfortunately the code will still produce error in msie5.
can anyone check if this is also a prob with ns4.x ?

Comment 13

[Daniel Wang]

msie5 loads the file fine if the first 3 characters in the
js file are removed.

if the first 3 bytes are removed, but charset is not set, msie5 and
mozilla will load the script but produce (document.write) jibberish
chars in iso-8859-1

Comment 14

[Shanjian Li]

Attachment: patch

Comment 15

[Shanjian Li]

ftang, could you review?

Comment 16

[Frank Tang]

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

r=ftang
make sure there are space between if and (

Comment 17

[Shanjian Li]

I will take care of the space format issue before checkin. 
dbaron, could you sr?

Comment 18

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

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

I could, although I'd much prefer if jst did since he's the module owner for
this area of code.  (Other than the bizarre indentation, which should be made
consistent with the rest of the file, it seems fine to me, although the "should
change to" comments seem to have been done already.)

Comment 19

[Shanjian Li]

johny,I try to avoid you this time in order not to keep you too busy. But it
seems like I have to do ask you about this one since you are the best candidate. 

Comment 20

[Johnny Stenback (:jst)]

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

+
+// This function is copied from nsParser.cpp. It was simplied though,
unnecessary part is removed.

"simlified" is mis-spelled above.

+static PRBool DetectByteOrderMark(const unsigned char* aBytes, PRInt32 aLen,
nsString& oCharset) {

Please put the static keyword and the return type on its own line, and opening
brace on its own line as well.

+ if (aLen < 2)
+   return false;
+
+ switch(aBytes[0])
+	 {
...

Please clean up this indentation mess. No tabs, 2-space indentation.

Other than that (and what dbaron pointed out about the comments), sr=jst

Comment 21

[Shanjian Li]

fix checked in. 

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

urg. this patch is bad. it uses AssignWithConversion, which should no longer be
used, and uses nsString as function argument.

It should use a more general type as the argument (nsAString seems appropriate).
as for
+        oCharset.AssignWithConversion("UTF-8"); 

it would be better written as:
oCharset.Assign(NS_LITERAL_STRING("UTF-8"));
which can also be faster, if the compiler supports doing the conversion at
compile time.

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

oh yeah, one more thing:
+ return oCharset.Length() > 0;

can be written as:
return !oCharset.IsEmpty();

which can be faster.

Comment 24

[Roland Mainz]

This checkin broke the OS/2 tinderbox on
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey-Ports ... is anyone
looking at this ?

Comment 25

[Katsuhiko Momoi]

If you have a serious concern with the current fix, please
re-open this bug or file another one. 

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

reopening bug so that the issues from comment 22 and comment 23 can get addressed.

Comment 27

[Shanjian Li]

bug 170339 filed to code cleanup. Close this bug.

Comment 28

[Teruko Kobayashi]

Changed QA contact to [e-mail].

Comment 29

[Yuying Long]

Verified it's fixed in 10-31 trunk build.  However it's still in 1.0.2 branch build.

I'm marking this as verified, if any one think it's important for branch build,
feel free to nominate it.