Columns are wrong for the first line of inline scripts (for exception, stacks, console message,...)
Categories
(Core :: JavaScript Engine, task, P3)
Tracking
()
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 ofdata: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 fordata:<span>foo<script>exception()</script>:1:1
whereas chrome reportsdata: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)
Reporter | ||
Comment 1•2 years ago
|
||
Reporter | ||
Comment 2•2 years ago
|
||
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
Reporter | ||
Comment 3•2 years ago
|
||
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.
Updated•2 years ago
|
Assignee | ||
Comment 4•2 years ago
|
||
Depends on D170579
Assignee | ||
Comment 5•2 years ago
|
||
Depends on D170580
Assignee | ||
Comment 6•2 years ago
|
||
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)
Assignee | ||
Comment 7•2 years ago
|
||
(also, we might move this to DOM Product as there's nothing to do in SpiderMonkey)
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 9•2 years ago
|
||
Backed out 2 changesets (Bug 1815937) for causing wpt failures at blockeduri-inline.htm
Backout: https://hg.mozilla.org/integration/autoland/rev/28a86c6f1e078c8a1bc465fead7f51425c1ef97e
Failure log: https://treeherder.mozilla.org/logviewer?job_id=409281527&repo=autoland&lineNumber=3726
Assignee | ||
Comment 10•2 years ago
|
||
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
<!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 forcolumnNumber
- 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•2 years ago
|
||
302 to Tom Schuster who is best suited to answer here.
Comment 12•2 years ago
|
||
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•2 years ago
|
||
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.
Assignee | ||
Comment 14•2 years ago
|
||
(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•2 years ago
|
||
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•2 years ago
|
||
Comment 18•2 years ago
•
|
||
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
Assignee | ||
Comment 20•2 years ago
|
||
looks like this need one of the patch I didn't include in my push
Comment 21•2 years ago
|
||
Comment 22•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ac2fee8163f3
https://hg.mozilla.org/mozilla-central/rev/0f2ee243a2b4
Description
•