Bug 73099 - Web Inspector: Unsafe cross origin access errors should show stack trace in console.
Summary: Web Inspector: Unsafe cross origin access errors should show stack trace in c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Vsevolod Vlasov
URL:
Keywords:
Depends on: 76566
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-24 15:18 PST by Vsevolod Vlasov
Modified: 2012-01-18 13:55 PST (History)
18 users (show)

See Also:


Attachments
Patch (20.90 KB, patch)
2011-11-24 15:22 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (24.08 KB, patch)
2011-11-25 08:28 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (27.81 KB, patch)
2011-11-30 05:07 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (44.18 KB, patch)
2011-12-08 05:34 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (44.21 KB, patch)
2011-12-08 05:59 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (16.20 KB, patch)
2011-12-08 14:01 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (18.84 KB, patch)
2012-01-18 05:35 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (18.80 KB, patch)
2012-01-18 05:51 PST, Vsevolod Vlasov
pfeldman: review+
pfeldman: commit-queue-
Details | Formatted Diff | Diff
Patch (18.78 KB, patch)
2012-01-18 07:28 PST, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2011-11-24 15:18:59 PST
Upstreaming https://code.google.com/p/chromium/issues/detail?id=88885
Comment 1 Vsevolod Vlasov 2011-11-24 15:22:57 PST
Created attachment 116551 [details]
Patch
Comment 2 Vsevolod Vlasov 2011-11-24 15:24:46 PST
I am going to add some more tests to check each scenario that was touched by this patch, but I'd like to get some early feedback on the overall implementation.
Comment 3 Adam Barth 2011-11-24 21:16:54 PST
Comment on attachment 116551 [details]
Patch

Oh, wow.  This is really exciting!
Comment 4 Adam Barth 2011-11-24 21:35:18 PST
Comment on attachment 116551 [details]
Patch

My only complaint is that this idiom takes too many lines to write.  Aside from that, this is great.
Comment 5 Pavel Feldman 2011-11-24 22:41:49 PST
Comment on attachment 116551 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Proxy.cpp:166
> +    if (stackTrace && stackTrace->size() > 0) {

Sounds like console could figure it out by itsef. See ConsoleMessage.cpp line #50. Reusing it will address Adam's concern.

> Source/WebCore/page/DOMWindow.cpp:928
> +            if (stackTrace && stackTrace->size() > 0) {

ditto

> Source/WebCore/page/DOMWindow.cpp:1785
> +    if (stackTrace && stackTrace->size() > 0) {

ditto

> Source/WebCore/xml/XMLHttpRequest.cpp:840
> +    if (m_stackTrace && m_stackTrace->size() > 0) {

ditto
Comment 6 Vsevolod Vlasov 2011-11-25 08:28:19 PST
Created attachment 116627 [details]
Patch
Comment 7 Vsevolod Vlasov 2011-11-25 08:30:11 PST
Please note that fixing console output (stdout/stderr) is out of the scope of this change. That would require a lot of tests rebaseline and I am not sure how much this is needed. This patch only passes callStack through to inspector.
Comment 8 Sam Weinig 2011-11-28 20:27:54 PST
Comment on attachment 116627 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Proxy.cpp:172
> +    addMessageToConsole(page, str, kSourceID, kLineNumber, stackTrace);

You could release the stackTrace here, since addMessageToConsole takes a PassRefPtr.

> Source/WebCore/page/DOMWindow.cpp:143
> +    PassRefPtr<ScriptCallStack> stackTrace() const { return m_stackTrace; }

This should return a raw pointer, since you are not relinquishing the ref.

> Source/WebCore/page/DOMWindow.cpp:908
> +    // Capture stack trace only when inspector front-end is loaded as it may be time consuming.
> +    RefPtr<ScriptCallStack> stackTrace;
> +    if (InspectorInstrumentation::hasFrontends())
> +        stackTrace = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);

In other places, I believe we used settings()->developerExtrasEnabled() for this check.

> Source/WebCore/page/DOMWindow.cpp:911
> +    PostMessageTimer* timer = new PostMessageTimer(this, message, sourceOrigin, source, channels.release(), target.get(), stackTrace);

This could use stackTrace.release().

> Source/WebCore/page/DOMWindow.cpp:925
> +            RefPtr<ScriptCallStack> stackTrace = timer->stackTrace();

There doesn't seem to be a good reason to put this in a RefPtr, since you never use it.

> Source/WebCore/page/DOMWindow.cpp:1778
>      // FIXME: Add arguments so that we can provide a correct source URL and line number.
> -    console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, 1, String());
> +    RefPtr<ScriptCallStack> stackTrace = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
> +    console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, 1, String(), stackTrace);

If we have a stack track, can we also have the URL and line number?

> Source/WebCore/xml/XMLHttpRequest.cpp:483
> +    // Capture stack trace only when inspector front-end is loaded as it may be time consuming.
> +    if (InspectorInstrumentation::hasFrontends())
> +        m_stackTrace = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);

Same comment as about about settings()->developerExtrasEnabled()
Comment 9 Sam Weinig 2011-11-28 20:28:26 PST
Please also make sure this works of JSC.
Comment 10 Yury Semikhatsky 2011-11-28 23:06:39 PST
Comment on attachment 116627 [details]
Patch

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

> Source/WebCore/page/DOMWindow.cpp:907
> +    if (InspectorInstrumentation::hasFrontends())

There is Console::shouldCaptureFullStackTrace which is used to decide whether we should capture current stack trace for console messages. developerExtrasEnabled() may be added there if needed, but there should be a single place where we chose to capture stack trace.
Comment 11 Vsevolod Vlasov 2011-11-30 05:02:00 PST
(In reply to comment #8)
> (From update of attachment 116627 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116627&action=review
> 
> > Source/WebCore/bindings/v8/V8Proxy.cpp:172
> > +    addMessageToConsole(page, str, kSourceID, kLineNumber, stackTrace);
> 
> You could release the stackTrace here, since addMessageToConsole takes a PassRefPtr.
Done.

> > Source/WebCore/page/DOMWindow.cpp:143
> > +    PassRefPtr<ScriptCallStack> stackTrace() const { return m_stackTrace; }
> 
> This should return a raw pointer, since you are not relinquishing the ref.
Done.

> > Source/WebCore/page/DOMWindow.cpp:908
> > +    // Capture stack trace only when inspector front-end is loaded as it may be time consuming.
> > +    RefPtr<ScriptCallStack> stackTrace;
> > +    if (InspectorInstrumentation::hasFrontends())
> > +        stackTrace = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
> 
> In other places, I believe we used settings()->developerExtrasEnabled() for this check.
It is not enough to have developer extras enabled here. We don't want to capture stack trace unless inspector front-end is open.

> > Source/WebCore/page/DOMWindow.cpp:911
> > +    PostMessageTimer* timer = new PostMessageTimer(this, message, sourceOrigin, source, channels.release(), target.get(), stackTrace);
> 
> This could use stackTrace.release().
Done.

> > Source/WebCore/page/DOMWindow.cpp:925
> > +            RefPtr<ScriptCallStack> stackTrace = timer->stackTrace();
> 
> There doesn't seem to be a good reason to put this in a RefPtr, since you never use it.
Removed this line.

> Please also make sure this works of JSC.
Unfortunately it does not. Once https://bugs.webkit.org/show_bug.cgi?id=40118 is fixed it will work as will some other features that need stack trace. Looks like something is going on there so hopefully it will be resolved soon.

> > Source/WebCore/page/DOMWindow.cpp:1778
> >      // FIXME: Add arguments so that we can provide a correct source URL and line number.
> > -    console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, 1, String());
> > +    RefPtr<ScriptCallStack> stackTrace = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
> > +    console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, 1, String(), stackTrace);
> 
> If we have a stack track, can we also have the URL and line number?
Currently we have this logic for extracting url and line number from stack trace for inspector console only: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/inspector/ConsoleMessage.cpp&l=70
As I mentioned above, I am not sure how much this is needed for stderr.
We can put it Console.cpp:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/page/Console.cpp&exact_package=chromium&q=console.cpp&type=cs&l=156
but that would require a lot of tests rebaseline and what is worse, since we don't have stack traces in JSC yet, custom expectations for chromium for a huge amount of tests.
I suggest we do that in a separate patch once JSC have createScriptCallStack() implemented.
Comment 12 Vsevolod Vlasov 2011-11-30 05:07:27 PST
Created attachment 117173 [details]
Patch
Comment 13 Pavel Feldman 2011-12-01 01:37:28 PST
Comment on attachment 117173 [details]
Patch

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

Passing the line number along with the stack is a bit confusing. I'd suggest that you introduce Console::addErrorMessage() receiving the stack trace only (no explicit location). You could assign one to XMLHttpRequest from within bindings instead of setting the line number. Sounds like all other call sites are falling back to the default values in either case.

> Source/WebCore/page/DOMWindow.cpp:928
> +            console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, 0, String(), timer->stackTrace());

Should it be "0"?

> Source/WebCore/page/DOMWindow.cpp:1778
> +    console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, 1, String(), stackTrace.release());

Or "1" ?

> Source/WebCore/xml/XMLHttpRequest.cpp:163
> +    , m_lastSendLineNumber(1)

I.e. why did this change?
Comment 14 Vsevolod Vlasov 2011-12-08 05:34:09 PST
Created attachment 118371 [details]
Patch
Comment 15 Vsevolod Vlasov 2011-12-08 05:36:16 PST
This patch keeps zeros and ones in console messages the same as they were before.
I also filed https://bugs.webkit.org/show_bug.cgi?id=74075 and uploaded a patch that makes use of ones as the default line number consistent there.
Comment 16 Gustavo Noronha (kov) 2011-12-08 05:38:38 PST
Comment on attachment 118371 [details]
Patch

Attachment 118371 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10809219
Comment 17 Early Warning System Bot 2011-12-08 05:41:53 PST
Comment on attachment 118371 [details]
Patch

Attachment 118371 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10809222
Comment 18 Gyuyoung Kim 2011-12-08 05:41:57 PST
Comment on attachment 118371 [details]
Patch

Attachment 118371 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10790224
Comment 19 Vsevolod Vlasov 2011-12-08 05:59:41 PST
Created attachment 118376 [details]
Patch
Comment 20 Pavel Feldman 2011-12-08 06:53:38 PST
Comment on attachment 118376 [details]
Patch

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

> Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:41
> +#include "ScriptCallStack.h"

Please mind the alphabetic order

> Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:118
> +    impl()->setCallStack(createScriptCallStack(exec));

I think createScriptCallStack is a bad name for the method, sorry I missed the moment it got landed. You should explicitly state that the stack depth depends on the inspector front-end existence.

> Source/WebCore/bindings/v8/V8Proxy.cpp:166
> +    RefPtr<ScriptCallStack> stackTrace = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);

I don't see a corresponding change to JSC. Is this a part of a different change? You should probably limit the scope of this change to XHR and keep symmetry here.

> Source/WebCore/xml/XMLHttpRequest.h:128
> +    void setCallStack(PassRefPtr<ScriptCallStack>);

setLastSendCallStack ?

> Source/WebCore/xml/XMLHttpRequest.h:228
> +    RefPtr<ScriptCallStack> m_callStack;

m_lastSendCallStack
Comment 21 Vsevolod Vlasov 2011-12-08 13:57:45 PST
(In reply to comment #20)
> (From update of attachment 118376 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118376&action=review
> 
> > Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:118
> > +    impl()->setCallStack(createScriptCallStack(exec));
> 
> I think createScriptCallStack is a bad name for the method, sorry I missed the moment it got landed. You should explicitly state that the stack depth depends on the inspector front-end existence.
XHR changes are moved out to a separate patch.

> 
> > Source/WebCore/bindings/v8/V8Proxy.cpp:166
> > +    RefPtr<ScriptCallStack> stackTrace = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
> 
> I don't see a corresponding change to JSC. Is this a part of a different change? You should probably limit the scope of this change to XHR and keep symmetry here.
JSC uses DOMWindow->printErrorMessage() to report unsafe cross origin access from bindings.
Comment 22 Vsevolod Vlasov 2011-12-08 14:01:51 PST
Created attachment 118463 [details]
Patch
Comment 23 Pavel Feldman 2011-12-08 14:28:18 PST
Comment on attachment 118463 [details]
Patch

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

> Source/WebCore/page/DOMWindow.cpp:909
> +        stackTrace = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);

Should we use createScriptCallStackForInspector here?

> Source/WebCore/page/DOMWindow.cpp:928
> +            console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, 0, String(), timer->stackTrace());

Please file a bug requesting to introduce addMessage that would only accept message and stack (with no line number and url). I guess you are not doing it here in order not to update expectations, right?

> Source/WebCore/page/DOMWindow.cpp:1779
> +    RefPtr<ScriptCallStack> stackTrace = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);

Should we use createScriptCallStackForInspector here?
Comment 24 Pavel Feldman 2011-12-08 14:29:26 PST
Comment on attachment 118463 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:-1566
> -    Settings* settings = targetFrame->settings();

Why did this change?
Comment 25 Vsevolod Vlasov 2012-01-18 05:35:09 PST
Created attachment 122905 [details]
Patch
Comment 26 Vsevolod Vlasov 2012-01-18 05:44:28 PST
Comment on attachment 122905 [details]
Patch

Clearing r?
Comment 27 Vsevolod Vlasov 2012-01-18 05:51:03 PST
Created attachment 122907 [details]
Patch
Comment 28 Vsevolod Vlasov 2012-01-18 05:51:39 PST
(In reply to comment #23)
> (From update of attachment 118463 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118463&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:909
> > +        stackTrace = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
> 
> Should we use createScriptCallStackForInspector here?

I don't think so. This code is called even when there wasn't any error and saving even one frame from call stack when there inspector is not open could be bad for performance.

> > Source/WebCore/page/DOMWindow.cpp:928
> > +            console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, 0, String(), timer->stackTrace());
> 
> Please file a bug requesting to introduce addMessage that would only accept message and stack (with no line number and url). I guess you are not doing it here in order not to update expectations, right?

I've added methods taking only message and call stack to both Console and ScriptExecutionContext. Expectations will not be updated because line number reported in expectations is never based on information from call stack.

> > Source/WebCore/page/DOMWindow.cpp:1779
> > +    RefPtr<ScriptCallStack> stackTrace = createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture, true);
> 
> Should we use createScriptCallStackForInspector here?

Done.

(In reply to comment #24)
> (From update of attachment 118463 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118463&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:-1566
> > -    Settings* settings = targetFrame->settings();
> 
> Why did this change?

This is done in DOMWindow::printErrorMessage.
Comment 29 Gyuyoung Kim 2012-01-18 06:21:12 PST
Comment on attachment 122907 [details]
Patch

Attachment 122907 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11211600
Comment 30 Early Warning System Bot 2012-01-18 06:24:45 PST
Comment on attachment 122907 [details]
Patch

Attachment 122907 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11284168
Comment 31 Pavel Feldman 2012-01-18 07:17:54 PST
Comment on attachment 122907 [details]
Patch

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

r- since breaking the build badly.

> Source/WebCore/page/DOMWindow.cpp:1724
> +    console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, stackTrace.release());

Please capture the full stack here.
Comment 32 Vsevolod Vlasov 2012-01-18 07:28:18 PST
Created attachment 122928 [details]
Patch
Comment 33 Timothy Hatcher 2012-01-18 07:43:24 PST
Comment on attachment 122928 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Proxy.cpp:147
> +    sourceDocument->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, str, stackTrace.release());

Please file a bug for JSC.
Comment 34 Vsevolod Vlasov 2012-01-18 08:39:51 PST
(In reply to comment #33)
> (From update of attachment 122928 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122928&action=review
> 
> > Source/WebCore/bindings/v8/V8Proxy.cpp:147
> > +    sourceDocument->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, str, stackTrace.release());
> 
> Please file a bug for JSC.

I don't see any JSC specific handling of unsafe access error reporting.
Looks like JSC is using WebCore methods to report errors, like here: http://goo.gl/OAxcS.
Comment 35 Collabora GTK+ EWS bot 2012-01-18 09:07:54 PST
Comment on attachment 122907 [details]
Patch

Attachment 122907 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11194203
Comment 36 Vsevolod Vlasov 2012-01-18 12:59:33 PST
Committed r105310: <http://trac.webkit.org/changeset/105310>