Bug 73099 – Web Inspector: Unsafe cross origin access errors should show stack trace in console.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Vsevolod Vlasov]

Upstreaming https://code.google.com/p/chromium/issues/detail?id=88885

[Vsevolod Vlasov]

Created attachment 116551 [details]
Patch

[Vsevolod Vlasov]

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.

[Adam Barth]

Comment on attachment 116551 [details]
Patch

Oh, wow.  This is really exciting!

[Adam Barth]

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.

[Pavel Feldman]

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

[Vsevolod Vlasov]

Created attachment 116627 [details]
Patch

[Vsevolod Vlasov]

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.

[Sam Weinig]

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

[Sam Weinig]

Please also make sure this works of JSC.

[Yury Semikhatsky]

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.

[Vsevolod Vlasov]

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

[Vsevolod Vlasov]

Created attachment 117173 [details]
Patch

[Pavel Feldman]

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?

[Vsevolod Vlasov]

Created attachment 118371 [details]
Patch

[Vsevolod Vlasov]

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.

[Gustavo Noronha (kov)]

Comment on attachment 118371 [details]
Patch

Attachment 118371 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10809219

[Early Warning System Bot]

Comment on attachment 118371 [details]
Patch

Attachment 118371 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10809222

[Gyuyoung Kim]

Comment on attachment 118371 [details]
Patch

Attachment 118371 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10790224

[Vsevolod Vlasov]

Created attachment 118376 [details]
Patch

[Pavel Feldman]

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

[Vsevolod Vlasov]

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

[Vsevolod Vlasov]

Created attachment 118463 [details]
Patch

[Pavel Feldman]

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?

[Pavel Feldman]

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?

[Vsevolod Vlasov]

Created attachment 122905 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 122905 [details]
Patch

Clearing r?

[Vsevolod Vlasov]

Created attachment 122907 [details]
Patch

[Vsevolod Vlasov]

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

[Gyuyoung Kim]

Comment on attachment 122907 [details]
Patch

Attachment 122907 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11211600

[Early Warning System Bot]

Comment on attachment 122907 [details]
Patch

Attachment 122907 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11284168

[Pavel Feldman]

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.

[Vsevolod Vlasov]

Created attachment 122928 [details]
Patch

[Timothy Hatcher]

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.

[Vsevolod Vlasov]

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

[Collabora GTK+ EWS bot]

Comment on attachment 122907 [details]
Patch

Attachment 122907 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11194203

[Vsevolod Vlasov]

Committed r105310: <http://trac.webkit.org/changeset/105310>