Closed
Bug 124904
Opened 23 years ago
Closed 22 years ago
Gecko parser still recognises meta tag's content even when it's surrounded by comments
Categories
(Core :: DOM: HTML Parser, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla1.0
People
(Reporter: hansoodev, Assigned: harishd)
References
Details
(Keywords: topembed+, Whiteboard: Fix checked into 0.9.4 and 0.9.9 branch)
Attachments
(4 files, 6 obsolete files)
517 bytes,
text/html
|
Details | |
441 bytes,
text/html
|
Details | |
13 bytes,
patch
|
asa
:
approval+
|
Details | Diff | Splinter Review |
695 bytes,
patch
|
hjtoi-bugzilla
:
review+
darin.moz
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•23 years ago
|
||
In This case, since ISO-88590-1 is overwritten by UTF-8, all accented characters are replaced with ? marks.
Reporter | ||
Comment 2•23 years ago
|
||
In This case, ISO-88590-1 *IS* the encoding detected and all accented characters are displayed correctly.
Comment 3•23 years ago
|
||
wfm 2002021108 win98 Mozilla sets the char encoding to iso-8859-1 in both testcases.
Reporter | ||
Comment 4•23 years ago
|
||
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.
Assignee: harishd → yokoyama
Comment 5•23 years ago
|
||
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?
Assignee: yokoyama → harishd
Comment 6•23 years ago
|
||
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..
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.
Reporter | ||
Comment 8•23 years ago
|
||
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
Accepting and targeting to 1.0
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
Target Milestone: --- → mozilla1.0
Assignee | ||
Comment 10•23 years ago
|
||
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.
Assignee | ||
Comment 11•23 years ago
|
||
Assignee | ||
Comment 12•23 years ago
|
||
Assignee | ||
Comment 13•23 years ago
|
||
+ 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.
Assignee | ||
Comment 14•23 years ago
|
||
Assignee | ||
Comment 15•23 years ago
|
||
Comment 16•23 years ago
|
||
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 18•23 years ago
|
||
See Bugscape Bug: http://bugscape.mcom.com/show_bug.cgi?id=12252
Comment 19•23 years ago
|
||
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.
Attachment #71544 -
Flags: review+
Assignee | ||
Comment 20•23 years ago
|
||
Yes, return (++aStart != aEnd) ? *aStart : nsnull; is a bug will fix that.
Comment 21•23 years ago
|
||
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
Attachment #71544 -
Flags: superreview+
Assignee | ||
Comment 22•23 years ago
|
||
darin: dp suggested the same thing ( on AIM ). Will do that.
Assignee | ||
Comment 23•23 years ago
|
||
darin: dp suggested the same thing ( on AIM ). Will do that.
Comment 24•23 years ago
|
||
requesting edt0.9.4+
Assignee | ||
Comment 25•23 years ago
|
||
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!
Attachment #71428 -
Attachment is obsolete: true
Attachment #71429 -
Attachment is obsolete: true
Attachment #71431 -
Attachment is obsolete: true
Attachment #71543 -
Attachment is obsolete: true
Attachment #71544 -
Attachment is obsolete: true
Assignee | ||
Comment 26•23 years ago
|
||
Good to go. Except for the missing ';' near foundMatch = !foundMatch. :-) Thanx Nisheeth.
Assignee | ||
Comment 27•23 years ago
|
||
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.
Whiteboard: Fix checked into 0.9.4 branch
Assignee | ||
Comment 28•23 years ago
|
||
Sorry about the missing semicolon! This patch fixes that problem. This is what got checked into the 0.9.4 branch.
Attachment #71801 -
Attachment is obsolete: true
Updated•23 years ago
|
Keywords: fixed0.9.4
QA Contact: moied → teruko
Updated•23 years ago
|
Reporter | ||
Comment 29•23 years ago
|
||
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 )
Assignee | ||
Comment 30•23 years ago
|
||
Hansoo: Just make sure that international pages get the correct charset. Try: http://komodo.mozilla.org/buster
Comment 31•23 years ago
|
||
> 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•23 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 34•22 years ago
|
||
will take a look
Assignee | ||
Comment 35•22 years ago
|
||
Comment on attachment 72654 [details] [diff] [review] patch v1.4.2 [ fix the hang ] r=heikki
Attachment #72654 -
Flags: review+
Comment 37•22 years ago
|
||
Comment on attachment 72654 [details] [diff] [review] patch v1.4.2 [ fix the hang ] sr=darin
Attachment #72654 -
Flags: superreview+
Comment 38•22 years ago
|
||
approved by marek
Assignee | ||
Comment 39•22 years ago
|
||
Fixed on the branch.
Comment 40•22 years ago
|
||
How is this different from bug 105725?
Assignee | ||
Comment 41•22 years ago
|
||
*** Bug 105725 has been marked as a duplicate of this bug. ***
Comment 42•22 years ago
|
||
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
Attachment #72654 -
Flags: approval+
Comment 43•22 years ago
|
||
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
Attachment #71807 -
Flags: approval+
Assignee | ||
Comment 44•22 years ago
|
||
FIXED on the TRUNK & BRANCH.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 45•22 years ago
|
||
This was a very late "trunk" checkin. We need to push this fix to 099 ASAP. Reopening until 099 checkin occurs.
Assignee | ||
Comment 46•22 years ago
|
||
Landed into 0.9.9 branch. Marking FIXED.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Whiteboard: Fix checked into 0.9.4 branch → Fix checked into 0.9.4 and 0.9.9 branch
Reporter | ||
Comment 47•22 years ago
|
||
Verified the check-in with bonsai... Thank you, Harish...
Updated•22 years ago
|
Keywords: fixed0.9.9
You need to log in
before you can comment on or make changes to this bug.
Description
•