Bug 50390 - Web Inspector: Console records for failed XHRs should contain call stack and request method
Summary: Web Inspector: Console records for failed XHRs should contain call stack and ...
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: Yury Semikhatsky
URL:
Keywords:
Depends on: 50551 50672
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-02 08:21 PST by Yury Semikhatsky
Modified: 2011-09-19 10:26 PDT (History)
10 users (show)

See Also:


Attachments
Patch (50.28 KB, patch)
2010-12-02 08:43 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (49.91 KB, patch)
2010-12-03 11:12 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (32.92 KB, patch)
2010-12-08 06:40 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (31.10 KB, patch)
2010-12-09 02:15 PST, Yury Semikhatsky
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2010-12-02 08:21:57 PST
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.
Comment 1 Yury Semikhatsky 2010-12-02 08:43:00 PST
Created attachment 75380 [details]
Patch
Comment 2 Joseph Pecoraro 2010-12-02 09:12:29 PST
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.
Comment 3 Patrick Mueller 2010-12-02 11:20:32 PST
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 ...
Comment 4 Pavel Feldman 2010-12-02 13:26:44 PST
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?
Comment 5 Yury Semikhatsky 2010-12-03 06:50:12 PST
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?
Comment 6 Yury Semikhatsky 2010-12-03 06:53:20 PST
(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.
Comment 7 Yury Semikhatsky 2010-12-03 11:12:22 PST
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.
Comment 8 Yury Semikhatsky 2010-12-03 11:12:51 PST
Created attachment 75515 [details]
Patch
Comment 9 Pavel Feldman 2010-12-07 02:11:39 PST
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.
Comment 10 Yury Semikhatsky 2010-12-08 06:40:32 PST
Created attachment 75897 [details]
Patch
Comment 11 Yury Semikhatsky 2010-12-08 06:44:02 PST
(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.
Comment 12 Yury Semikhatsky 2010-12-08 06:47:52 PST
(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)
Comment 13 Pavel Feldman 2010-12-08 07:03:49 PST
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.
Comment 14 Yury Semikhatsky 2010-12-09 02:15:09 PST
Created attachment 76027 [details]
Patch
Comment 15 Yury Semikhatsky 2010-12-09 02:17:25 PST
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.
Comment 16 Pavel Feldman 2010-12-09 05:18:33 PST
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.
Comment 17 Yury Semikhatsky 2010-12-09 05:36:28 PST
Committed r73607: <http://trac.webkit.org/changeset/73607>
Comment 18 Yury Semikhatsky 2010-12-09 05:37:13 PST
(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.
Comment 19 Pavel Feldman 2011-09-19 10:26:18 PDT
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.