RESOLVED FIXED Bug 41060
REGRESSION (HTML5 tokenizer): document.write affects line numbers reported to the VM 
https://bugs.webkit.org/show_bug.cgi?id=41060
Summary REGRESSION (HTML5 tokenizer): document.write affects line numbers reported to...
Pavel Feldman
Reported 2010-06-23 05:55:43 PDT
Created attachment 59506 [details] [TEST] Layout test for line number. Following snippet results in console.log reporting line 108 instead of 8: <html> <head> <script> for (var i = 0; i < 100; ++i) document.write("<p></p>\n"); </script> <script> console.log("Line number should be == 8."); </script> </head> <body> <p>This test checks that document.write does not affect line numbering and script tags are reported to have correct line numbers</p> </body> </html> Attaching snippet as a draft for the layout test.
Attachments
[TEST] Layout test for line number. (324 bytes, text/html)
2010-06-23 05:55 PDT, Pavel Feldman 		no flags
Details
Patch (3.66 KB, patch)
2010-06-23 21:14 PDT, Tony Gentilcore 		no flags
DetailsFormatted DiffDiff
Key off of isFromNetwork instead of executingScript() (3.68 KB, patch)
2010-06-24 11:37 PDT, Tony Gentilcore 		no flags
DetailsFormatted DiffDiff
Patch for landing (3.66 KB, patch)
2010-06-24 13:13 PDT, Tony Gentilcore 		no flags
DetailsFormatted DiffDiff
Patch for landing (17.25 KB, patch)
2010-06-26 09:32 PDT, Tony Gentilcore 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2010-06-23 10:22:55 PDT
@tonyg: Would you like to take a look at this one?
Tony Gentilcore
Comment 2 2010-06-23 10:31:24 PDT
(In reply to comment #1) > @tonyg: Would you like to take a look at this one? Oh no, I hope I'm not the "line number guy" now ;-) But, yeah, I'm happy to take this one. Pavel, do you know if this passed with the legacy tokenizer?
Adam Barth
Comment 3 2010-06-23 10:34:04 PDT
> Pavel, do you know if this passed with the legacy tokenizer? From briefly testing the Chrome Dev channel, it appears to have worked with the old parser.
Pavel Feldman
Comment 4 2010-06-23 10:35:23 PDT
It surely did work before. Updated the title.
Tony Gentilcore
Comment 5 2010-06-23 21:14:36 PDT
Created attachment 59609 [details] Patch
Adam Barth
Comment 6 2010-06-23 21:39:26 PDT
Comment on attachment 59609 [details] Patch Why executingScript() and not !isFromNetwork?
Tony Gentilcore
Comment 7 2010-06-23 21:44:17 PDT
(In reply to comment #6) > (From update of attachment 59609 [details]) > Why executingScript() and not !isFromNetwork? Perhaps Eric should take a look at this too. But this is what the Legacy parser did and there is a comment in the isFromNetwork block that says something about getting data off the network in a nested call to write. That made me thing there is a scenario where isFromNetwork wouldn't do the right thing.
Adam Barth
Comment 8 2010-06-23 21:55:14 PDT
The old parser was confused about what that bool meant. It seems like you don't want to count line numbers that don't come from the network, which is what using isFromNetwork would give you. Whether we're currently executing script when we get the bytes might end up being the same thing, but only by accident.
Eric Seidel (no email)
Comment 9 2010-06-23 22:34:33 PDT
(In reply to comment #8) > The old parser was confused about what that bool meant. The old parser invented to bool. :) It just confused itself. > It seems like you don't want to count line numbers that don't come from the network, which is what using isFromNetwork would give you. Whether we're currently executing script when we get the bytes might end up being the same thing, but only by accident. You do want to count line numbers from the network, but not writes. Seems we might want to pump with a special flag, and only count when we're not in a nested write? Donno, I'll have to look at the patch. :)
Adam Barth
Comment 10 2010-06-23 22:38:41 PDT
That flag already exists. The question is when to set it.
WebKit Review Bot
Comment 11 2010-06-24 03:53:17 PDT
Attachment 59609 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3280689
Tony Gentilcore
Comment 12 2010-06-24 11:37:47 PDT
Created attachment 59683 [details] Key off of isFromNetwork instead of executingScript()
Adam Barth
Comment 13 2010-06-24 13:01:13 PDT
Comment on attachment 59683 [details] Key off of isFromNetwork instead of executingScript() Beautiful. Thanks.
Tony Gentilcore
Comment 14 2010-06-24 13:13:41 PDT
Created attachment 59687 [details] Patch for landing
Eric Seidel (no email)
Comment 15 2010-06-24 16:54:28 PDT
We're still going to get the lines reported to the console wrong. See: LegacyHTMLTreeBuilder::reportErrorToConsole With the blind cast to LegacyHTMLDocumentParser and call to processingContentWrittenByScript() LegacyHTMLDocumentParser* htmlTokenizer = static_cast<LegacyHTMLDocumentParser*>(m_document->parser()); if (htmlTokenizer->processingContentWrittenByScript()) That probably crashes or gives the wrong answer. Not the same as this bug, but related, definitely.
Eric Seidel (no email)
Comment 16 2010-06-24 18:03:44 PDT
Filed bug 41187.
WebKit Commit Bot
Comment 17 2010-06-25 21:11:16 PDT
Comment on attachment 59687 [details] Patch for landing Rejecting patch 59687 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1 Parsed 5 diffs from patch file(s). patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/fast/js/script-line-number-expected.txt patching file LayoutTests/fast/js/script-line-number.html patching file WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebCore/html/HTMLDocumentParser.cpp Hunk #1 FAILED at 231. 1 out of 1 hunk FAILED -- saving rejects to file WebCore/html/HTMLDocumentParser.cpp.rej Full output: http://webkit-commit-queue.appspot.com/results/3305712
Tony Gentilcore
Comment 18 2010-06-26 09:32:12 PDT
Created attachment 59837 [details] Patch for landing
WebKit Commit Bot
Comment 19 2010-06-26 09:49:46 PDT
Comment on attachment 59837 [details] Patch for landing Clearing flags on attachment: 59837 Committed r61956: <http://trac.webkit.org/changeset/61956>
WebKit Commit Bot
Comment 20 2010-06-26 09:49:51 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.