Bug 86943 – Blocking `setTimeout` and `setInterval` evaluation with CSP should include a stack trace in the console warning.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mike West]

Along the same lines as http://webk.it/86848, the error message for CSP-blocked `setTimeout` and `setInterval` calls don't give developers enough information to debug the issue. We should include a stack trace with the error so they can jump directly to the offending line.

I've got a patch together that does this, but I'm a bit worried that it's making the CSP interface pretty complex (especially in conjunction with 86848, and some CSP 1.1 work).

I'd also like to add a test to verify that a call stack is present, but I'm not exactly sure how I can track that down from JavaScript. I dug through the inspector tests currently in the system, but nothing seemed to fit the use case. Suggestions?

[Mike West]

Created attachment 142866 [details]
Patch

[Build Bot]

Comment on attachment 142866 [details]
Patch

Attachment 142866 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12729378

(In reply to comment #0)
> I'd also like to add a test to verify that a call stack is present, but I'm not exactly sure how I can track that down from JavaScript. I dug through the inspector tests currently in the system, but nothing seemed to fit the use case. Suggestions?

I think that InspectorTest.dumpConsoleMessages() should do the trick (see LayoutTests/inspector/console/)

[Mike West]

After digging a bit, it looks like the InspectorTest framework isn't playing nicely with CSP. If I inject `<meta http-equiv="X-WebKit-CSP" content="script-src 'unsafe-inline' 'self'">` into http/tests/inspector/console-cd.html, for example, it times out. I'm not exactly sure where it's dying, but it's entirely possible that it relies on `eval` (in the form of `RuntimeAgent.evaluate`).

[Adam Barth]

The code in this patch looks fine, but we should have a test.  I'm not sure what to do about the test harness using eval...

(In reply to comment #5)
> The code in this patch looks fine, but we should have a test.  I'm not sure what to do about the test harness using eval...

I would recommend to add a function to window.internals that returns the recorded console messages, doing basically the same as InspectorTest.dumpConsoleMessages, just without depending on InspectorTest

[Pavel Feldman]

(In reply to comment #6)
> (In reply to comment #5)
> > The code in this patch looks fine, but we should have a test.  I'm not sure what to do about the test harness using eval...
> 
> I would recommend to add a function to window.internals that returns the recorded console messages, doing basically the same as InspectorTest.dumpConsoleMessages, just without depending on InspectorTest

+1

[Mike West]

As it turns out, this isn't an `eval` problem. It's an inspector test issue.

Long story short, if I copy my test into the `http/tests/inspector` directory, things work as expected. If I don't, I get timeouts. Jochen suggested that there might be a separate inspector agent process that's only spun up if certain tests are executed.

It looks like `http/tests/inspector-enabled` is there for exactly this purpose; I'm adding tests there.

[Mike West]

Created attachment 143014 [details]
Patch

[Build Bot]

Comment on attachment 143014 [details]
Patch

Attachment 143014 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12733602

[Adam Barth]

Comment on attachment 143014 [details]
Patch

Looks like you've got some sort of problem on apple-win.

[Mike West]

Yeah, it looks like the compiler is picking the wrong `isAllowedByAll` and throwing type errors. Would you mind terribly if I simply renamed the function to something like `isAllowedByAllWithCallStack` instead of overloading the `isAllowedByAll` that takes a KURL?

We'd end up with something like CSP::isAllowedByAll, CSP::isAllowedByAllWithCallStack, and CSP::isAllowedByAllWithKURL, which is ugly, but simpler to understand than digging through Windows' template specialization rules.

(It might, of course, just be something simple that I'm overlooking...)

[Adam Barth]

That's fine.  These functions are only used in this file, so it's ok if their names a bit wonky.

[Mike West]

Created attachment 143070 [details]
Patch

[WebKit Review Bot]

Comment on attachment 143070 [details]
Patch

Clearing flags on attachment: 143070

Committed r117826: <http://trac.webkit.org/changeset/117826>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.