[webkit-reviews] review denied: [Bug 73099] Web Inspector: Unsafe cross origin access errors should show stack trace in console. : [Attachment 116627] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Vsevolod Vlasov <vsevik at chromium.org>'s
request for review:
Bug 73099: Web Inspector: Unsafe cross origin access errors should show stack
trace in console.
https://bugs.webkit.org/show_bug.cgi?id=73099

Attachment 116627: Patch
https://bugs.webkit.org/attachment.cgi?id=116627&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
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()