Bug 73099

Created attachment 116551 [details] Patch

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 on attachment 116551 [details] Patch Oh, wow. This is really exciting!

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

Created attachment 116627 [details] Patch

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

Please also make sure this works of JSC.

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.

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

Created attachment 117173 [details] Patch

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?

Created attachment 118371 [details] Patch

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 on attachment 118371 [details] Patch Attachment 118371 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10809219

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

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

Created attachment 118376 [details] Patch

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

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

Created attachment 118463 [details] Patch

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

Created attachment 122905 [details] Patch

Comment on attachment 122905 [details] Patch Clearing r?

Created attachment 122907 [details] Patch

(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 on attachment 122907 [details] Patch Attachment 122907 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11211600

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

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.

Created attachment 122928 [details] Patch

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.

(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 on attachment 122907 [details] Patch Attachment 122907 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11194203

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