| 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: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 41115, 41187 | ||||||||||||||
| Attachments: |
|
||||||||||||||
@tonyg: Would you like to take a look at this one? (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? > 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.
It surely did work before. Updated the title. Created attachment 59609 [details]
Patch
Comment on attachment 59609 [details]
Patch
Why executingScript() and not !isFromNetwork?
(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. 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. (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. :) That flag already exists. The question is when to set it. Attachment 59609 [details] did not build on win: Build output: http://webkit-commit-queue.appspot.com/results/3280689 Created attachment 59683 [details]
Key off of isFromNetwork instead of executingScript()
Comment on attachment 59683 [details]
Key off of isFromNetwork instead of executingScript()
Beautiful. Thanks.
Created attachment 59687 [details]
Patch for landing
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.
Filed bug 41187. 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 Created attachment 59837 [details]
Patch for landing
Comment on attachment 59837 [details] Patch for landing Clearing flags on attachment: 59837 Committed r61956: <http://trac.webkit.org/changeset/61956> All reviewed patches have been landed. Closing bug. |
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.