Bug 41060

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>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, commit-queue, eric, podivilov, timothy, tonyg, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

41115, 41187

Attachments:

Description	Flags
[TEST] Layout test for line number.	none
Patch	none
Key off of isFromNetwork instead of executingScript()	none
Patch for landing	none
Patch for landing	none

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	Details Formatted Diff Diff
Key off of isFromNetwork instead of executingScript() (3.68 KB, patch) 2010-06-24 11:37 PDT, Tony Gentilcore	no flags	Details Formatted Diff Diff
Patch for landing (3.66 KB, patch) 2010-06-24 13:13 PDT, Tony Gentilcore	no flags	Details Formatted Diff Diff
Patch for landing (17.25 KB, patch) 2010-06-26 09:32 PDT, Tony Gentilcore	no flags	Details Formatted Diff Diff
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.