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

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase

Load the testcase and look at the line number reported for the error in the JS
console...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix v 1.0

Comment 3

[harishd]

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

Comment in bug 111576:
------- Additional Comment #46 From [e-mail] 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

[harishd]

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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: attempted testcase for some attribute values with leading/ending newlines

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

[harishd]

>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 ;)

Comment 8

[Boris Zbarsky [: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?

Comment 9

[harishd]

Boris: If I get the time I will run your patch through the page load tests today.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I don't think I care anymore.

Comment 11

[David G King]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to trunk

Comment 14

[Boris Zbarsky [:bzbarsky]]

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?  ;)

Comment 15

[Christopher Hoess (gone)]

Comment on attachment 141378 [details] [diff] [review]
Merged to trunk

Looks good to me.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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...

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 141378 [details] [diff] [review]
Merged to trunk

Looks good. sr=jst

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to Darin's string changes

Comment 19

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.7b.  Tinderbox shows no perfomance impact from this change.

Comment 20

[Walter K.]

attachment 141778 [details] [diff] [review]:
Nit: methods' comments have different number of parameters than actual methods.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

This caused bug 235171