Closed Bug 1815937 Opened 2 years ago Closed 2 years ago

Columns are wrong for the first line of inline scripts (for exception, stacks, console message,...)

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED
113 Branch
Tracking Status
firefox113 --- fixed

People

(Reporter: ochameau, Assigned: nchevobbe)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Here is a few test pages:

  • data:text/html,<span>foo<script>console.log(""+new Error().stack)</script>
    will print @data:text/html,<span>foo<script>console.log(""+new Error().stack)</script>:1:16
    instead of data:text/html,<span>foo<script>console.log(""+new Error().stack)</script>:1:33 (on chrome)

  • data:text/html,<span>foo<script>exception()</script>
    The exception will be logged for data:<span>foo<script>exception()</script>:1:1
    whereas chrome reports data:text/html,<span>foo<script>exception()</script>:1:18.

The first test page highlights this isn't a DevTools issue, but something that should probably be handled within spidermonkey.
I'll attach a mochitest to ease testing this.

Note that we do workaround this in DevTools in a pretty gross way for breakpoints.
I wish we could remove this code:
https://searchfox.org/mozilla-central/rev/28c0d45a553fd2817ac14e1562435e86dc0aa403/devtools/server/actors/source.js#347-372
(Breakpoints somewhat work for this edgecase)

The issue is that columns are indexed by the beginning of the <script> content, instead of the beginning of the HTML line.

<span>foo<script>console.log(""+new Error().stack)</script>
^               ^--- column 0 is here
\------------------- instead of there

I tried looking into the various column attribute we have in spidermonkey but couldn't find any reporting the beginning of the script tag or beginning of the script source text content.

If fixing this for stack and everything is a complex task, it would already be handy if we could expose this offset to devtools via the Debugger API.
We could then more easily apply the shift without grepping strings in HTML :o

I investigated using option.columnno from ScriptSource::initFromOptions
https://searchfox.org/mozilla-central/rev/362882b13f8a23d93d547963d376485816947199/js/src/vm/JSScript.cpp#1872
But it was zero. But I'm not super confident about my C++ skills. So may be I messed up.

Severity: -- → N/A
Priority: -- → P3
Depends on: 1552008

With the patch from Bug 1552008 and https://phabricator.services.mozilla.com/D170580 , the issue seems to be fixed (the added test does pass)
One problem I'm seeing is that for some cases the column number is offset by 1 somehow, so I guess there might be something about column numbers starting at 0 or 1.
The good news is that it allows us to get rid of inline source specific code in the source actor (https://phabricator.services.mozilla.com/D170581)

(also, we might move this to DOM Product as there's nothing to do in SpiderMonkey)

Attachment #9319148 - Attachment description: WIP: Bug 1815937 - Set ScriptLoadContext column number → Bug 1815937 - Set ScriptLoadContext column number for inline scripts. r=dom-reviewers.
Attachment #9319149 - Attachment description: WIP: Bug 1815937 - [devtools] Remove inline script displacement handling in source actor. r=ochameau. → Bug 1815937 - [devtools] Remove inline script displacement handling in source actor. r=devtools-reviewers.
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Attachment #9319148 - Attachment description: Bug 1815937 - Set ScriptLoadContext column number for inline scripts. r=dom-reviewers. → Bug 1815937 - Set ScriptLoadContext column number for inline scripts. r=hsivonen.
Attachment #9319149 - Attachment description: Bug 1815937 - [devtools] Remove inline script displacement handling in source actor. r=devtools-reviewers. → Bug 1815937 - [devtools] Remove inline script displacement handling in source actor. r=#devtools-reviewers.
Attachment #9319148 - Attachment description: Bug 1815937 - Set ScriptLoadContext column number for inline scripts. r=hsivonen. → Bug 1815937 - Set ScriptLoadContext column number for inline scripts. r=smaug.
Attachment #9319148 - Attachment description: Bug 1815937 - Set ScriptLoadContext column number for inline scripts. r=smaug. → Bug 1815937 - Set ScriptLoadContext column number for inline scripts. r=hsivonen.
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a4a3430c9bab Set ScriptLoadContext column number for inline scripts. r=smaug,devtools-reviewers,ochameau. https://hg.mozilla.org/integration/autoland/rev/668971329051 [devtools] Remove inline script displacement handling in source actor. r=devtools-reviewers,ochameau.
Regressions: 1823018

This looks a bit strange to me
So the wpt test that fails is this one https://searchfox.org/mozilla-central/source/testing/web-platform/tests/content-security-policy/securitypolicyviolation/blockeduri-inline.html
It tests that we get a securitypolicyviolation with expected lineNumber and columnNumber

https://searchfox.org/mozilla-central/rev/841c6d76db55d110d1fd3f8891d0ed1632be7736/testing/web-platform/tests/content-security-policy/securitypolicyviolation/blockeduri-inline.html#1-19

<!doctype html>
<meta http-equiv="Content-Security-Policy" content="script-src 'nonce-abc'">
<script nonce="abc">
    async_test(t => {
        var watcher = new EventWatcher(t, document, 'securitypolicyviolation');
        watcher.wait_for('securitypolicyviolation').then(t.step_func_done(e => {
            assert_equals(e.blockedURI, "inline");
            assert_equals(e.lineNumber, 15);
            assert_equals(e.columnNumber, 1);
        }));
    }, "Inline violations have a blockedURI of 'inline'");
</script>
<script>
    test(t => {
        assert_unreached();
    }, "Blocked script shouldn't execute.");
</script>

With my patch, the columnNumber we get is the start of the actual script (so the location after <script>), which is obviously not 1
What's troubling me is that if I move the <script> location, the current behavior, without this patch, is always to have a columnNumber of 1

So let's say we have the following:

<meta http-equiv="Content-Security-Policy" content="script-src 'nonce-abc'">
<script nonce="abc">
  document.addEventListener('securitypolicyviolation', e => console.log(e, e.lineNumber, e.columnNumber));
</script>
<hr><script>alert("Blocked script shouldn't execute.");</script><hr><script>alert("Blocked script shouldn't execute.");</script>
<b><script>alert("Blocked script shouldn't execute.");</script><i><script>alert("Blocked script shouldn't execute.");</script>

we'll get 4 securitypolicyviolation, all of them with a columnNumber of 1, on all browsers¹, making it impossible to distinguish between events if multiple scripts are on the same line.

The spec isn't super explicit about what those numbers should be: https://w3c.github.io/webappsec-csp/#ref-for-violation-column-number

If the user agent is currently executing script, and can extract a source file’s URL, line number, and column number from the global, set violation’s source file, line number, and column number accordingly.

So either:

  • we only want to report a column number of 1, and we shouldn't look into the script column for columnNumber
  • we want the accurate column number, and the wpt test is erroneous

The column number check in the test was added in Bug 1418246, and if we look into https://bugzilla.mozilla.org/show_bug.cgi?id=1418246#c1 , we might have this 1 only because we didn't have the information yet (which was added in Bug 1552008)

Christoph, Baku, do you think we should update this wpt test so it actually assert the right column number?


¹ except on Blink, where it's 0, which is why they fail the test https://wpt.fyi/results/content-security-policy/securitypolicyviolation/blockeduri-inline.html?label=master&label=experimental&aligned&view=subtest&q=securitypolicyviolation%2Fblockeduri-inline.html

Flags: needinfo?(nchevobbe)
Flags: needinfo?(ckerschb)
Flags: needinfo?(amarchesini)

302 to Tom Schuster who is best suited to answer here.

Flags: needinfo?(tschuster)
Flags: needinfo?(ckerschb)
Flags: needinfo?(amarchesini)

Interesting question, but I don't think I could give any kind of definite answer. In the specification it actually says:

Is this kind of thing specified anywhere?

I can kind of see when 1 might makes sense, when we considering: <script>console.log(new Error("First").stack)</script><script>console.log(new Error("Second").stack)</script>. Here the stack says column 13 both times because we only count from the end of the <script> tag.
However considering your example with every script starting at 1 and being indistinguishable I would vote for updating the test with our new more helpful column number.

Flags: needinfo?(tschuster)

Here the stack says column 13 both times because we only count from the end of the <script> tag.

Interesting, testing in Chrome that actually says 21 and 75. I think that is even more evidence for improving this.

(In reply to Tom Schuster (MoCo) from comment #12)

Interesting question, but I don't think I could give any kind of definite answer. In the specification it actually says:

Is this kind of thing specified anywhere?

Yeah, I saw that :/

I can kind of see when 1 might makes sense, when we considering: <script>console.log(new Error("First").stack)</script><script>console.log(new Error("Second").stack)</script>. Here the stack says column 13 both times because we only count from the end of the <script> tag.
Interesting, testing in Chrome that actually says 21 and 75. I think that is even more evidence for improving this.

This is what the patch attached for this bug does, we should have the same result than Chrome now

However considering your example with every script starting at 1 and being indistinguishable I would vote for updating the test with our new more helpful column number.

Is there any kind of process to follow for updating a wpt test, or can I just change the assertion?

Flags: needinfo?(tschuster)

Is there any kind of process to follow for updating a wpt test, or can I just change the assertion?

You can just change the test and it will be synced automatically to https://github.com/web-platform-tests/wpt/ after landing.

Flags: needinfo?(tschuster)
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/103bf8d93883 Set ScriptLoadContext column number for inline scripts. r=smaug,devtools-reviewers,ochameau.
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/39089 for changes under testing/web-platform/tests

Backed out 3 changesets (Bug 1823335, Bug 1815937, Bug 1823399) for devtools failures on browser_dbg-features-breakable-positions.js
Backout link <--> dt12
Failure Log
Also dt3 Failure Log

Flags: needinfo?(nchevobbe)
Upstream PR was closed without merging

looks like this need one of the patch I didn't include in my push

Flags: needinfo?(nchevobbe)
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ac2fee8163f3 Set ScriptLoadContext column number for inline scripts. r=smaug,devtools-reviewers,ochameau. https://hg.mozilla.org/integration/autoland/rev/0f2ee243a2b4 [devtools] Remove inline script displacement handling in source actor. r=devtools-reviewers,ochameau.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch
Upstream PR merged by moz-wptsync-bot
Blocks: 1824950
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: