Bug 71099 - Reset line numbers for scripts generated with document.write.
Summary: Reset line numbers for scripts generated with document.write.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-28 02:12 PDT by Pavel Feldman
Modified: 2011-10-28 11:30 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.86 KB, patch)
2011-10-28 05:31 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
[Patch] With inline handler test case. (8.92 KB, patch)
2011-10-28 06:04 PDT, Pavel Feldman
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2011-10-28 02:12:47 PDT
We need to distinguish scripts generated with document.write from the ones generated by the parser of the original resource. Otherwise we bind them to the resource and hence lose them from the debugging UI.

Consider the following example:

1: <html>
2: <script>function aa() {}</script>
3: <script>function bb() {}</script>
4: <script>document.write("<scrip" + "t>\n\n\n\n\nfunction cc() {}</sc" + "ript>");</script>
5: <script>function dd() {}</script>
6: </html>

As a result, following scripts are reported to be parsed (all sharing the main resource url):

line 2-2
line 3-3
line 4-10
line 5-5

As a result of this patch, scripts will be annotated:

line 2-2
line 3-3
line 0-6
line 5-5

We will then render scripts starting at 0 as standalone entities.
Comment 1 Pavel Feldman 2011-10-28 05:31:29 PDT
Created attachment 112851 [details]
Patch
Comment 2 Yury Semikhatsky 2011-10-28 05:46:49 PDT
Comment on attachment 112851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112851&action=review

> LayoutTests/http/tests/inspector-enabled/document-write.html:5
> +<script>console.log("Line 5");</script>

Please test also that JS handlers written in html attributes will have positions different from scripts created with document.write.
Comment 3 Pavel Feldman 2011-10-28 06:04:04 PDT
Created attachment 112852 [details]
[Patch] With inline handler test case.
Comment 4 Yury Semikhatsky 2011-10-28 06:17:22 PDT
Comment on attachment 112852 [details]
[Patch] With inline handler test case.

View in context: https://bugs.webkit.org/attachment.cgi?id=112852&action=review

> LayoutTests/http/tests/inspector-enabled/document-write.html:27
> +Tests that console reports zero line number for scripts generated with document.write.

Please add a link to the bug in this description.
Comment 5 Pavel Feldman 2011-10-28 06:27:53 PDT
Committed r98724: <http://trac.webkit.org/changeset/98724>
Comment 6 Adam Barth 2011-10-28 11:15:05 PDT
Comment on attachment 112852 [details]
[Patch] With inline handler test case.

View in context: https://bugs.webkit.org/attachment.cgi?id=112852&action=review

> Source/WebCore/dom/ScriptElement.cpp:201
> +    Document* document = m_element->document();

In general it's dangerous to store references like this in a raw pointer.  If anything in this script executes script between this program point and any use of this variable, you're going to have a security vulnerability.  It's much better to continue to access the document via some protected point.
Comment 7 Pavel Feldman 2011-10-28 11:28:34 PDT
 > In general it's dangerous to store references like this in a raw pointer.  If anything in this script executes script between this program point and any use of this variable, you're going to have a security vulnerability.  It's much better to continue to access the document via some protected point.

I went through the code originally and have not noticed any uncontrolled lack of control flow / re-enterability. I can roll this refactoring back though in order to maintain the security pattern you suggest in case it is common to WebCore.
Comment 8 Adam Barth 2011-10-28 11:30:00 PDT
I checked them too and I didn't see any problem.  It's just a trap waiting to catch someone.  It's up to you whether you'd like to change it back.