|Summary:||REGRESSION (HTML5 tokenizer): document.write affects line numbers reported to the VM|
|Product:||WebKit||Reporter:||Pavel Feldman <pfeldman>|
|Component:||Web Inspector (Deprecated)||Assignee:||Tony Gentilcore <tonyg>|
|Severity:||Normal||CC:||abarth, commit-queue, eric, podivilov, timothy, tonyg, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
|Bug Blocks:||41115, 41187|
Description Pavel Feldman 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.
Comment 1 Adam Barth 2010-06-23 10:22:55 PDT
@tonyg: Would you like to take a look at this one?
Comment 2 Tony Gentilcore 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?
Comment 3 Adam Barth 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.
Comment 4 Pavel Feldman 2010-06-23 10:35:23 PDT
It surely did work before. Updated the title.
Comment 6 Adam Barth 2010-06-23 21:39:26 PDT
Comment on attachment 59609 [details] Patch Why executingScript() and not !isFromNetwork?
Comment 7 Tony Gentilcore 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.
Comment 8 Adam Barth 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.
Comment 9 Eric Seidel (no email) 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. :)
Comment 10 Adam Barth 2010-06-23 22:38:41 PDT
That flag already exists. The question is when to set it.
Comment 11 WebKit Review Bot 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
Comment 12 Tony Gentilcore 2010-06-24 11:37:47 PDT
Created attachment 59683 [details] Key off of isFromNetwork instead of executingScript()
Comment 13 Adam Barth 2010-06-24 13:01:13 PDT
Comment on attachment 59683 [details] Key off of isFromNetwork instead of executingScript() Beautiful. Thanks.
Comment 14 Tony Gentilcore 2010-06-24 13:13:41 PDT
Created attachment 59687 [details] Patch for landing
Comment 15 Eric Seidel (no email) 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.
Comment 17 WebKit Commit Bot 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
Comment 18 Tony Gentilcore 2010-06-26 09:32:12 PDT
Created attachment 59837 [details] Patch for landing
Comment 19 WebKit Commit Bot 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>
Comment 20 WebKit Commit Bot 2010-06-26 09:49:51 PDT
All reviewed patches have been landed. Closing bug.