[FIXr]Incorrect line numbers in HTML files (part 2)

RESOLVED FIXED in mozilla1.7beta

Status

()

Core
HTML: Parser
P3
normal
RESOLVED FIXED
16 years ago
14 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla1.7beta
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

This is a followup to bug 111576.  The patch there does not handle newlines
correctly in the following types of tokens:

1)  CDATA tokens
2)  Attribute value tokens
Created attachment 79084 [details]
testcase

Load the testcase and look at the line number reported for the error in the JS
console...
Created attachment 79085 [details] [diff] [review]
Proposed fix v 1.0

Comment 3

16 years ago
Yes, I'm aware of the attribute value issue.

Comment in bug 111576:
------- Additional Comment #46 From harishd@netscape.com 2002-03-21 15:56 -------
FYI: The patch still has a problem with newlines within quoted attribute values
--

I recluctant to fix it because I expected perf. hit. Boris, do you see any
performance problems with your patch?

Comment 4

16 years ago
Comment on attachment 79085 [details] [diff] [review]
Proposed fix v 1.0

>   static const PRUnichar theTerminalCharsQuote[] = { 
>-    PRUnichar(kQuote), PRUnichar('&'),  PRUnichar(0) };
>+    PRUnichar(kQuote), PRUnichar('&'), PRUnichar(kCR),
>+    PRUnichar(kNewLine), PRUnichar(0) };
>   static const PRUnichar theTerminalCharsApostrophe[] = { 
>-    PRUnichar(kApostrophe), PRUnichar('&'), PRUnichar(0) };
>+    PRUnichar(kApostrophe), PRUnichar('&'), PRUnichar(kCR),
>+    PRUnichar(kNewLine), PRUnichar(0) };

I'm a bit worried about this because it changes the current behavior. Boris,
how well have to tested this? Have you made sure that cases like name="value\n" 
,name=\nvalue\n etc. work correctly?
Created attachment 79281 [details]
attempted testcase for some attribute values with leading/ending newlines
I haven't noticed any perf issues, but I haven't rigorously tested performance
either....  Having an idea of what to test, would be great.  Should I just try
to get someone to run the pageload tests?

I tested attr="foo\nbar", and attached is a testcase that tests the other
possibilities you mention.  In all cases our behavior is the same as it was
before my change.

If attr values in particular are an issue, I can split out the CDATA part of the
patch and land that separately (that part is a lot less scary  :)).

Now that I read bug 111576 again, the newline counter in the scanner that you
mention sounds like a much better idea... if you're going to implement that
anyway, I think I'd be ok with just fixing CDATA for 1.0 and leaving attribute
values to be fixed by the "real fix".

Comment 7

16 years ago
>Now that I read bug 111576 again, the newline counter in the scanner that you
>mention sounds like a much better idea... if you're going to implement that
>anyway, I think I'd be ok with just fixing CDATA for 1.0 and leaving attribute
>values to be fixed by the "real fix".

If you're confident with your changes then go ahead [ provided page-load perf. /
browser buster tests (komodo.mozilla.org/buster) pass :) ].

Anyway, since you've done all the work reassigning bug to you ;)
Assignee: harishd → bzbarsky
jrgm, could you run this patch through the pageload test?  Or is there a way I
can land it on a tinderbox machine that runs pageload tests and see what the
result is?
(Assignee)

Updated

16 years ago
Priority: -- → P2
Summary: Incorrect line numbers in HTML files (part 2) → [FIX]Incorrect line numbers in HTML files (part 2)
Target Milestone: --- → mozilla1.1beta

Comment 9

16 years ago
Boris: If I get the time I will run your patch through the page load tests today.
(Assignee)

Updated

15 years ago
Priority: P2 → P1
Target Milestone: mozilla1.1beta → mozilla1.2alpha
(Assignee)

Updated

15 years ago
Target Milestone: mozilla1.2alpha → mozilla1.2beta
I don't think I care anymore.
Assignee: bzbarsky → harishd
Priority: P1 → --
Target Milestone: mozilla1.2beta → ---

Updated

15 years ago
Priority: -- → P3
QA Contact: moied → dsirnapalli

Updated

15 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.5alpha
(Assignee)

Updated

14 years ago
Blocks: 207748

Comment 11

14 years ago
Out of interest, has Bug #208030 fixed this issue as well? I don't have a
working build with that patch in, so I can't check.
It couldn't possibly have fixed it, since it was an internal JS engine fix,
whereas this bug is about incorrect line numbers being reported to the JS engine
as "start" line numbers for scripts.

Harish, the more I think about this the more I think it should not be a big perf
issue... If there is no way to get perf results, I would like to just land this
during a quiet spell, watch tinderbox, and back out if there are perf issues. 
If you're ok with that, could you please, r= the patch?
Created attachment 141378 [details] [diff] [review]
Merged to trunk
Attachment #79085 - Attachment is obsolete: true
Comment on attachment 141378 [details] [diff] [review]
Merged to trunk

choess, since I'm not likely to get r=harish, could I get r=you?  ;)
Attachment #141378 - Flags: review?(choess)
Comment on attachment 141378 [details] [diff] [review]
Merged to trunk

Looks good to me.
Attachment #141378 - Flags: review?(choess) → review+
Comment on attachment 141378 [details] [diff] [review]
Merged to trunk

jst, what do you think?  This should make our line number reporting in the JS
console finally right...
Attachment #141378 - Flags: superreview?(jst)
Comment on attachment 141378 [details] [diff] [review]
Merged to trunk

Looks good. sr=jst
Attachment #141378 - Flags: superreview?(jst) → superreview+
(Assignee)

Updated

14 years ago
Assignee: harishd → bzbarsky
Status: ASSIGNED → NEW
Summary: [FIX]Incorrect line numbers in HTML files (part 2) → [FIXr]Incorrect line numbers in HTML files (part 2)
Target Milestone: mozilla1.5alpha → mozilla1.7beta
Created attachment 141778 [details] [diff] [review]
Merged to Darin's string changes
Attachment #141378 - Attachment is obsolete: true
Checked in for 1.7b.  Tinderbox shows no perfomance impact from this change.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 20

14 years ago
attachment 141778 [details] [diff] [review]:
Nit: methods' comments have different number of parameters than actual methods.
I'm aware of that.  Note that this is a problem without this patch too... and
since I knew that someone else (choess) is working on comments in the parser in
general I didn't change them for now to save him the pain of merging in fixes
that he's making anyway.
This caused bug 235171 
You need to log in before you can comment on or make changes to this bug.