124904 - Gecko parser still recognises meta tag's content even when it's surrounded by comments

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Hansoo Kim]

The page already contains meta tag about charset info for the whole page.
Later on in that page, there is another meta tag section defining another 
charset info, but this time, it's surrounded by the comment marks.
( e.g. <!--    and  -->  )

In this case, Gecko overwrites the charset info with later one, which is not 
supposed to be interpreted by the browser engine.


EX )
< other meta_tag's contents;    charset=A >
   .
   .
   .
Begin_comment  < other meta_tag's contents;    charset=B >   End_comment


In this case, the final charset is set to B, resulting some of characters on 
the page not readable.


I am also including the test case files, one of which include the extra comment 
with extra meta tag content, and one withou it.

Thanks.

Comment 1

[Hansoo Kim]

Attachment: Page with Extra meta tag's content as a comment

In This case, since ISO-88590-1 is overwritten by UTF-8,
all accented characters are replaced with ? marks.

Comment 2

[Hansoo Kim]

Attachment: Page *WITHOUT* Extra meta tag's content as a comment

In This case, ISO-88590-1 *IS* the encoding detected
and 
all accented characters are displayed correctly.

Comment 3

[Gilles Durys]

wfm 2002021108 win98
Mozilla sets the char encoding to iso-8859-1 in both testcases.

Comment 4

[Hansoo Kim]

Roy,

Would you put appropriate "priority", "severity", and "target mailstone"
for me ?

I will also update how much this needs to be fixed on my end.
However, as the testing cases show since there is a work around,
it should not be high priority. (?)

Thanks.

-------------------------------------------------------

Gilles Durys

I have tested with NS 6.2.
---> Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.4) Gecko/20011128 
Netscape6/6.2.1 

The result are the same as what I have described.

It looks like it is fixed in trunk, then.
However, I wonder the fix has been applied to most of branches.

Thank you for your input.

Comment 5

[Roy Yokoyama]

Parser owner:  Correct me if I am wrong; but I recall 
that we have fixed this problem.  Do we have it in 0_9_4_BRANCH?

Comment 6

[Matthias Versen [:Matti]]

Reporter: Can you test this with a mozilla build (0.9.8 or later) ?
If this works, then this bug is "worksforme" because this is not a Netscape bug 
database..

Comment 7

[harishd]

This is not fixed yet! I'm sure there is another open bug on this.  I don't
think this bug exists in 0.9.4 since the code on the branch is a bit different
than the tip.

Comment 8

[Hansoo Kim]

Harish,

I think the bug exists in both 9.4 and 9.4.2 branches.

Is there any update on this bug ?

Thank you very much.

HK

Comment 9

[harishd]

Accepting and targeting to 1.0

Comment 10

[harishd]

Attachment: patch v1.0 [ Needs additional testing ]

FYI: With the patch mozilla did not hang / crash. However, need to make sure
that I did not regress anything ( related to charset ). Yokoyama, do you know
of any test suite that I can use to test the patch before landing? Thanks.

Comment 11

[harishd]

Attachment: patch v1.1 [ same as above but without nsDTDUtils changes ]

Comment 12

[harishd]

Attachment: patch v1.2 [ tiny bit simplified :-/ ]

Comment 13

[harishd]

+  while (tagStart != end) {
+    if (FindCharInReadable('<', ++tagStart, end)) {

++tagStart should probably go to the end of the while-loop since on our first
iteration we could, potentially, miss the first tag. 

Comment 14

[harishd]

Attachment: patch v1.3

Comment 15

[harishd]

Attachment: patch v1.3.1 [ diff with "uw" options - for reviewers ]

Comment 16

[Roy Yokoyama]

harishd: to start, we can use attached test files by Hansoo
         Generic i18n test pages can be found at Test Data/Double Byte under
         http://babel.mcom.com/index_new.html

Comment 17

[Michael Dunn]

Adding KWs

Comment 18

[Michael Dunn]

See Bugscape Bug:
http://bugscape.mcom.com/show_bug.cgi?id=12252

Comment 19

[Suresh Duddi (gone)]

Comment on attachment 71544 [details] [diff] [review]
patch v1.3.1 [ diff with "uw" options - for reviewers ]

1. Make GetNextChar() into something like

 return (aStart != aEnd) ? *(++aStart) : '\0';

2. while (!foundMDC || !foundMatch)
can become
while (!foundMDC)

as foundMDC will turn true only if foundMatch is true.

Comment 20

[harishd]

Yes, return (++aStart != aEnd) ? *aStart : nsnull; is a bug will fix that.

Comment 21

[Darin Fisher]

Comment on attachment 71544 [details] [diff] [review]
patch v1.3.1 [ diff with "uw" options - for reviewers ]

>Index: nsParser.cpp

>+            foundMatch = (foundMatch) ? PR_FALSE : PR_TRUE; // toggle until we've matching "--"

how about "foundMatch = !foundMatch;" instead?

otherwise, sr=darin

Comment 22

[harishd]

darin: dp suggested the same thing ( on AIM ). Will do that. 

Comment 23

[harishd]

darin: dp suggested the same thing ( on AIM ). Will do that. 

Comment 24

[Nicholas B]

requesting edt0.9.4+

Comment 25

[harishd]

Attachment: Patch 1.4

I've made the changes suggested by Darin and dp.  Attaching a new patch. 
Harish, please take a look and give it a green light for checkin. Thanks!

Comment 26

[harishd]

Good to go. Except for the missing ';' near foundMatch = !foundMatch. :-)

Thanx Nisheeth.

Comment 27

[harishd]

Fix has been checked in to the 0.9.4 branch on behalf of Harish (This is
Nisheeth typing on Harish's computer).

Harish will check this in on the trunk later.

Comment 28

[harishd]

Attachment: Patch Version 1.4.1 that got checked into 0.9.4 branch

Sorry about the missing semicolon!  This patch fixes that problem.  This is
what got checked into the 0.9.4 branch.

Comment 29

[Hansoo Kim]

Harish...

I tried the patch posted... it works...

Thank you for your hard work.

FYI, I did not do any regression tests since not sure which ones to be done.

HK ( Hansoo Kim )

Comment 30

[harishd]

Hansoo: Just make sure that international pages get the correct charset. 

Try: http://komodo.mozilla.org/buster

Comment 31

[Katsuhiko Momoi]

> Hansoo: Just make sure that international pages get the correct charset. 

> Try: http://komodo.mozilla.org/buster

I don't know if this URL helps completely. This page loads target sites 
in one of the frames, but the top frame does not have any met-charset info. 
So, if you look at the Character Coding menu,it will always be set to your 
default encoding. 
You need to check the view frame info to get the info on charset for
loaded pages. I prefer a more straightforward page loading for this
type of test.

Comment 32

[Katsuhiko Momoi]

Interestingly, Hansoo's test page does not fail if one of
multi-byte auto detectors is turned on. All, Russian, Ukrainian
don't work but all others can avoid this problem. I guess we are
eliminating UTF-8 from the possibillity in these auto-detectors?
If harishd is going to check in the fix into the trunk, let's make
sure that auto-detect cases are also covered.

Comment 33

[[e-mail]]

This patch in 094 is causing www.disney.com to hang. (exact url is 
http://disney.go.com/park/homepage/today/flash/index.html)

The page contains serveral meta tags which mess up the new parsing codes. 

Please confirm.

Comment 34

[harishd]

will take a look

Comment 35

[harishd]

Attachment: patch v1.4.2 [ fix the hang ]

Comment 36

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 72654 [details] [diff] [review]
patch v1.4.2 [ fix the hang ]

r=heikki

Comment 37

[Darin Fisher]

Comment on attachment 72654 [details] [diff] [review]
patch v1.4.2 [ fix the hang ]

sr=darin

Comment 38

[Marek Z. Jeziorek]

approved by marek

Comment 39

[harishd]

Fixed on the branch.

Comment 40

[Boris Zbarsky [:bzbarsky]]

How is this different from bug 105725?

Comment 41

[harishd]

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

Comment 42

[Asa Dotzler [:asa]]

Comment on attachment 72654 [details] [diff] [review]
patch v1.4.2 [ fix the hang ]

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 43

[Asa Dotzler [:asa]]

Comment on attachment 71807 [details] [diff] [review]
Patch Version 1.4.1 that got checked into 0.9.4 branch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 44

[harishd]

FIXED on the TRUNK & BRANCH.

Comment 45

[Michael Dunn]

This was a very late "trunk" checkin. We need to push this fix to 099 ASAP.
Reopening until 099 checkin occurs.

Comment 46

[harishd]

Landed into 0.9.9 branch. Marking FIXED.

Comment 47

[Hansoo Kim]

Verified the check-in with bonsai...

Thank you, Harish...

Comment 48

[Teruko Kobayashi]

Verified as fixed in 04-05 0.99 build.