Bug 50390 – Web Inspector: Console records for failed XHRs should contain call stack and request method

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

[Yury Semikhatsky]

More detailes should be provided for failed XHRs including call stack of the code initiated the request and HTTP request method. The error message should be linked with the request entry in the Network panel and the script source in the Scripts panel.

[Yury Semikhatsky]

Created attachment 75380 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 75380 [details]
Patch

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

Just a quick once over this looks good to me. Although I think removing the "groupLevel" could have been in a separate patch to make this cleaner. For that this could use a second pair of eyes.

> WebCore/bindings/js/ScriptCallStackFactory.cpp:57
> +PassRefPtr<ScriptCallStack> createScriptCallStack(size_t maxStackSize)
> +{
> +/*
> +    return 0;
> +    */
> +    ExecState* exec = JSMainThreadExecState::currentState();

I think you can remove the comment. The implementation looks good to me.

> WebCore/inspector/InspectorResourceAgent.cpp:292
> +    RequestInfo info = {createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture), String()};

I don't know if this is WebKit style to create a struct like this.

[Patrick Mueller]

Concur on separating the grouplevel and other unrelated console-y stuff to a separate patch.

Looks like "failed XHR" == (http status code >= 400).  Good starting place I guess.  XHR can also fail catastrophically as well, right (throw an exception?).  I guess that case is covered by "stop on exceptions".

But wondering also about other resource-y APIs.  Cases for WebSocket, Web DB, et al APIs which might also be able to make use of this?  Things that go into "error" states, but don't actually throw exceptions.  

Just something to think about for the future ...

[Pavel Feldman]

Comment on attachment 75380 [details]
Patch

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

> WebCore/bindings/js/ScriptCallStackFactory.cpp:55
> +    return 0;

Please remove.

> WebCore/inspector/InspectorResourceAgent.cpp:293
> +    m_requestIdToRequestInfo.set(identifier, info);

I think there is the same code in Timeline. Should it re-use this code?

> WebCore/inspector/InspectorResourceAgent.cpp:302
> +    it->second.httpMethod = request.httpMethod();

FYI: redirect will result in several calls to willSendRequest with same id. With potentially different http methods. You end up with the resulting one which is not necessarily what you want.

> WebCore/inspector/InspectorResourceAgent.cpp:344
> +    m_requestIdToRequestInfo.remove(identifier);

Don't forget to make this call in didFailLoading too.

> WebCore/inspector/front-end/inspector.js:-1424
> -        payload.groupLevel,

Should you remove groupLevel from the native code as well?

[Yury Semikhatsky]

Comment on attachment 75380 [details]
Patch

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

>> WebCore/inspector/InspectorResourceAgent.cpp:292
>> +    RequestInfo info = {createScriptCallStack(ScriptCallStack::maxCallStackSizeToCapture), String()};
> 
> I don't know if this is WebKit style to create a struct like this.

It doesn't prohibited by the style guide and I can see several places in WebCore where such initializers are used so I believe it's OK to use.

>> WebCore/inspector/InspectorResourceAgent.cpp:293
>> +    m_requestIdToRequestInfo.set(identifier, info);
> 
> I think there is the same code in Timeline. Should it re-use this code?

I can't find such mapping in Timeline on the backend side, could you explain which code you suggest reuse?

With our current separation into independent components for timeline, resource tracking etc. which are supposed to work even when other components are disabled, we already send some data several times if different component need them. If we'd like to avoid that we should design a generic solution.

>> WebCore/inspector/InspectorResourceAgent.cpp:302
>> +    it->second.httpMethod = request.httpMethod();
> 
> FYI: redirect will result in several calls to willSendRequest with same id. With potentially different http methods. You end up with the resulting one which is not necessarily what you want.

I think we should capture HTTP method and URL during the first invocation of willSendRequest for given identifier.

>> WebCore/inspector/front-end/inspector.js:-1424
>> -        payload.groupLevel,
> 
> Should you remove groupLevel from the native code as well?

This patch removed it from InspectorController and ConsoleMessage, did I miss anything else?

[Yury Semikhatsky]

(In reply to comment #3)
> Looks like "failed XHR" == (http status code >= 400).  Good starting place I guess.  XHR can also fail catastrophically as well, right (throw an exception?).  I guess that case is covered by "stop on exceptions".
> 
> But wondering also about other resource-y APIs.  Cases for WebSocket, Web DB, et al APIs which might also be able to make use of this?  Things that go into "error" states, but don't actually throw exceptions.  
> 
> Just something to think about for the future ...

Yeah, there is some room for improvement in error reporting. I was also thinking about highlighting broken links in html.

[Yury Semikhatsky]

Comment on attachment 75380 [details]
Patch

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

>> WebCore/bindings/js/ScriptCallStackFactory.cpp:57
>> +    ExecState* exec = JSMainThreadExecState::currentState();
> 
> I think you can remove the comment. The implementation looks good to me.

I'd like to remove the commented piece but this code doesn't capture current stack trace, exec will always point at the bottom call frame of the current call stack and I don't know another way to get current JS call stack in JSC. I'm removing this code and leaving this method always returning 0.

[Yury Semikhatsky]

Created attachment 75515 [details]
Patch

[Pavel Feldman]

Comment on attachment 75515 [details]
Patch

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

A bunch of nits, no real suggestions. Switch in Switch worries me a lot.

> WebCore/inspector/InspectorController.cpp:866
> +        addConsoleMessage(new ConsoleMessage(OtherMessageSource, LogMessageType, ErrorMessageLevel, info.httpMethod, response.url().string(), response.httpStatusCode(), response.httpStatusText(), info.callStack));

Nit: this should probably be new message type.

> WebCore/inspector/front-end/ConsoleView.js:260
> +            var msgCopy = new WebInspector.ConsoleMessage(msg.source, msg.type, msg.level, msg.line, msg.url, count - prevRepeatCount, msg._messageText, msg._parameters, msg._stackTrace, msg._httpMethod, msg._httpStatusCode);

Should this be a ConsoleMessage ancestor?

> WebCore/inspector/front-end/ConsoleView.js:692
> +                    case WebInspector.ConsoleMessage.MessageType.Assert:

switch by type in switch by type clearly tells us that there is something wrong with the abstraction. We should either create hierarchy of messages (like ConsoleMessageWithStackInfo), or rely upon message types more extensively and introduce new ones.

> WebCore/inspector/front-end/ConsoleView.js:704
> +                        hideStackTrace = true;

I think there should be an explicit way of telling which messages have stack traces.

> WebCore/inspector/front-end/inspector.js:1428
> +        payload.httpMethod,

This will look quite strange in the protocol. It sounds like we should distinguish console messages from error messages. It sounds more like an error message bound to resource load rather than generic console message.

[Yury Semikhatsky]

Created attachment 75897 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #9)
> (From update of attachment 75515 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75515&action=review
> 
> A bunch of nits, no real suggestions. Switch in Switch worries me a lot.
> 
> > WebCore/inspector/InspectorController.cpp:866
> > +        addConsoleMessage(new ConsoleMessage(OtherMessageSource, LogMessageType, ErrorMessageLevel, info.httpMethod, response.url().string(), response.httpStatusCode(), response.httpStatusText(), info.callStack));
> 
> Nit: this should probably be new message type.
> 
Done. Added new message type for network errors.

> > WebCore/inspector/front-end/ConsoleView.js:260
> > +            var msgCopy = new WebInspector.ConsoleMessage(msg.source, msg.type, msg.level, msg.line, msg.url, count - prevRepeatCount, msg._messageText, msg._parameters, msg._stackTrace, msg._httpMethod, msg._httpStatusCode);
> 
> Should this be a ConsoleMessage ancestor?
>
Why? The clone just differs in repeat count.


> > WebCore/inspector/front-end/ConsoleView.js:692
> > +                    case WebInspector.ConsoleMessage.MessageType.Assert:
> 
> switch by type in switch by type clearly tells us that there is something wrong with the abstraction. We should either create hierarchy of messages (like ConsoleMessageWithStackInfo), or rely upon message types more extensively and introduce new ones.
> 
> > WebCore/inspector/front-end/ConsoleView.js:704
> > +                        hideStackTrace = true;
> 
> I think there should be an explicit way of telling which messages have stack traces.
> 
Done.


> > WebCore/inspector/front-end/inspector.js:1428
> > +        payload.httpMethod,
> 
> This will look quite strange in the protocol. It sounds like we should distinguish console messages from error messages. It sounds more like an error message bound to resource load rather than generic console message.

Only network error messages will have this data in the protocol.

[Yury Semikhatsky]

(In reply to comment #3)
> Concur on separating the grouplevel and other unrelated console-y stuff to a separate patch.
> 
Fixed this separately(https://bugs.webkit.org/show_bug.cgi?id=50672)

[Pavel Feldman]

Comment on attachment 75897 [details]
Patch

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

> WebCore/inspector/front-end/inspector.js:1390
> +        payload.httpMethod,

I still don't understand how you document these from the protocol standpoint.

[Yury Semikhatsky]

Created attachment 76027 [details]
Patch

[Yury Semikhatsky]

Changed implementation as it was agreed offline: ConsoleMessage now has request identifier which is used to lookup additional request details including call stack in the network panel. If there are no data for the request in the network panel we fall back to the default message formatting.

[Pavel Feldman]

Comment on attachment 76027 [details]
Patch

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

> WebCore/inspector/front-end/ConsoleView.js:668
> +                    stackTrace = resource.stackTrace;

In case of redirects, stackTrace will only be on the first item of the redirect chain. Get it from there or copy stack trace upon redirect resource clone.

[Yury Semikhatsky]

Committed r73607: <http://trac.webkit.org/changeset/73607>

[Yury Semikhatsky]

(In reply to comment #16)
> (From update of attachment 76027 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76027&action=review
> 
> > WebCore/inspector/front-end/ConsoleView.js:668
> > +                    stackTrace = resource.stackTrace;
> 
> In case of redirects, stackTrace will only be on the first item of the redirect chain. Get it from there or copy stack trace upon redirect resource clone.

Done.

[Pavel Feldman]

Comment on attachment 76027 [details]
Patch

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

> WebCore/inspector/front-end/ConsoleView.js:716
> +        if (this._stackTrace) {

You use "this._stackTrace", but you should use "stackTrace" here.