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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Alexandre Poirot [:ochameau]]

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)

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1815937 - Test inline script reported columns

Comment 2

[Alexandre Poirot [:ochameau]]

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

Comment 3

[Alexandre Poirot [:ochameau]]

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.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1815937 - Set ScriptLoadContext column number for inline scripts. r=hsivonen.

Depends on D170579

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1815937 - [devtools] Remove inline script displacement handling in source actor. r=#devtools-reviewers.

Depends on D170580

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

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)

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 9

[Cristina Horotan [:chorotan]]

Backed out 2 changesets (Bug 1815937) for causing wpt failures at blockeduri-inline.htm

Backout: https://hg.mozilla.org/integration/autoland/rev/28a86c6f1e078c8a1bc465fead7f51425c1ef97e

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=66897132905164553bcee0cc58ef52fc00019178&selectedTaskRun=aFuHoI6aTrGkNqPNnwDGZQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=409281527&repo=autoland&lineNumber=3726

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 12

[Tom Schuster (MoCo)]

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.

Comment 13

[Tom Schuster (MoCo)]

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.

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

(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?

Comment 15

[Tom Schuster (MoCo)]

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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/103bf8d93883
Set ScriptLoadContext column number for inline scripts. r=smaug,devtools-reviewers,ochameau.

Comment 17

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/39089 for changes under testing/web-platform/tests

Comment 18

[Marian-Vasile Laza]

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

Comment 19

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 22

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/ac2fee8163f3
https://hg.mozilla.org/mozilla-central/rev/0f2ee243a2b4

Comment 23

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot