Last Comment Bug 137315 - [FIXr]Incorrect line numbers in HTML files (part 2)
: [FIXr]Incorrect line numbers in HTML files (part 2)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla1.7beta
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: dsirnapalli
:
Mentors:
Depends on:
Blocks: 207748
  Show dependency treegraph
 
Reported: 2002-04-13 14:30 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2004-02-22 16:11 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (641 bytes, text/html)
2002-04-13 14:37 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Proposed fix v 1.0 (5.50 KB, patch)
2002-04-13 14:37 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
attempted testcase for some attribute values with leading/ending newlines (367 bytes, text/html)
2002-04-15 10:14 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Merged to trunk (9.71 KB, patch)
2004-02-13 19:35 PST, Boris Zbarsky [:bz] (still a bit busy)
choess: review+
jst: superreview+
Details | Diff | Splinter Review
Merged to Darin's string changes (9.70 KB, patch)
2004-02-19 16:24 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2002-04-13 14:30:50 PDT
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 [:bz] (still a bit busy) 2002-04-13 14:37:04 PDT
Created attachment 79084 [details]
testcase

Load the testcase and look at the line number reported for the error in the JS
console...
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2002-04-13 14:37:50 PDT
Created attachment 79085 [details] [diff] [review]
Proposed fix v 1.0
Comment 3 harishd 2002-04-15 09:10:30 PDT
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 harishd 2002-04-15 09:21:16 PDT
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 [:bz] (still a bit busy) 2002-04-15 10:14:15 PDT
Created attachment 79281 [details]
attempted testcase for some attribute values with leading/ending newlines
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2002-04-15 10:18:39 PDT
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 2002-04-15 10:53:40 PDT
>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 [:bz] (still a bit busy) 2002-04-15 11:03:26 PDT
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 2002-04-29 10:54:02 PDT
Boris: If I get the time I will run your patch through the page load tests today.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2002-10-27 09:55:16 PST
I don't think I care anymore.
Comment 11 David G King 2003-07-27 13:25:48 PDT
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 [:bz] (still a bit busy) 2003-07-27 13:44:40 PDT
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 [:bz] (still a bit busy) 2004-02-13 19:35:11 PST
Created attachment 141378 [details] [diff] [review]
Merged to trunk
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2004-02-13 19:36:25 PST
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) 2004-02-13 20:52:54 PST
Comment on attachment 141378 [details] [diff] [review]
Merged to trunk

Looks good to me.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2004-02-13 20:56:58 PST
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, jst@mozilla.com) 2004-02-16 09:33:45 PST
Comment on attachment 141378 [details] [diff] [review]
Merged to trunk

Looks good. sr=jst
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2004-02-19 16:24:59 PST
Created attachment 141778 [details] [diff] [review]
Merged to Darin's string changes
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2004-02-19 17:52:09 PST
Checked in for 1.7b.  Tinderbox shows no perfomance impact from this change.
Comment 20 Walter K. 2004-02-20 13:10:15 PST
attachment 141778 [details] [diff] [review]:
Nit: methods' comments have different number of parameters than actual methods.
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2004-02-20 13:22:15 PST
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 [:bz] (still a bit busy) 2004-02-22 16:11:10 PST
This caused bug 235171 

Note You need to log in before you can comment on or make changes to this bug.