Bug 37215 – Web Inspector: be more explicit about resource loading errors

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

[Andrey Kosyakov]

Currently, if a resource fails to load, we only show this in resource's headers tab. Since a failure to load a resource is likely to lead to a noticeable change in page behavior, I suggest we be more explicit about this, e.g. log an error message to console. This will lead to an error bubble being displayed against resource's icon in the resource list, and provide a hint to developers even in case resource tracking is not enabled.

[Patrick Mueller]

I agree we should provide some additional diagnostic information regarding resources that don't load - I just experienced this myself regarding a resource not be loaded from an app-cache-enabled page, for instance.

However, I dislike spamming the console.  Error bubble in the resource list is fine.  Perhaps a line in the Timeline (category "Loading") with diagnostic info.  

Where would be put the diagnostic info in the Resources panel?  Create a new "tab" (peer of Headers/Content) with the information?

[Andrey Kosyakov]

(In reply to comment #1)
> I agree we should provide some additional diagnostic information regarding
> resources that don't load - I just experienced this myself regarding a resource
> not be loaded from an app-cache-enabled page, for instance.
> 
> However, I dislike spamming the console.  Error bubble in the resource list is
> fine.  Perhaps a line in the Timeline (category "Loading") with diagnostic
> info.

My point is that a failure to load resource is supposed to be rare, but rather significant event, so in my view reporting it shouldn't be considered as spamming by most. Besides, currently we're very likely to produce "content type mismatch" warning in console when receiving error response from server (typically text/html) in response to many resource types (images, scripts, stylesheets) -- so my patch would just replace it with more comprehensible message. Also, I think it makes sense to produce it even in case resource tracking is disabled, to give users opening inspector a quick hint on why something on a page might have failed. Do you have a use case in mind that where a resource loading failure is "normal" and would cause lots of messages to be logged?

> 
> Where would be put the diagnostic info in the Resources panel?  Create a new
> "tab" (peer of Headers/Content) with the information?

In the current version of my patch, the errors cause a red bubble to appear next to a resource icon -- I think this is pretty sufficient to give a quick overview of failed resources. If the consensus is that we don't want this logged to console, I propose we still leave these bubbles (just implement it slightly differently). I wouldn't like to introduce any additional tabs for that, as it would be counter-intuitive -- one would need to know to navigate there (we already display failed status on a resource's headers tab).

[Andrey Kosyakov]

Created attachment 52752 [details]
Log error message to inspector console if a resource fails to load. Disable checking of mime-type consistency for failed resource.

[Timothy Hatcher]

(In reply to comment #2)
> (In reply to comment #1)
> > I agree we should provide some additional diagnostic information regarding
> > resources that don't load - I just experienced this myself regarding a resource
> > not be loaded from an app-cache-enabled page, for instance.
> > 
> > However, I dislike spamming the console.  Error bubble in the resource list is
> > fine.  Perhaps a line in the Timeline (category "Loading") with diagnostic
> > info.
> 
> My point is that a failure to load resource is supposed to be rare, but rather
> significant event, so in my view reporting it shouldn't be considered as
> spamming by most. Besides, currently we're very likely to produce "content type
> mismatch" warning in console when receiving error response from server
> (typically text/html) in response to many resource types (images, scripts,
> stylesheets) -- so my patch would just replace it with more comprehensible
> message. Also, I think it makes sense to produce it even in case resource
> tracking is disabled, to give users opening inspector a quick hint on why
> something on a page might have failed. Do you have a use case in mind that
> where a resource loading failure is "normal" and would cause lots of messages
> to be logged?

I think the console message is fine and should definatly replace the existing incorrect warning.

> > Where would be put the diagnostic info in the Resources panel?  Create a new
> > "tab" (peer of Headers/Content) with the information?
> 
> In the current version of my patch, the errors cause a red bubble to appear
> next to a resource icon -- I think this is pretty sufficient to give a quick
> overview of failed resources. If the consensus is that we don't want this
> logged to console, I propose we still leave these bubbles (just implement it
> slightly differently). I wouldn't like to introduce any additional tabs for
> that, as it would be counter-intuitive -- one would need to know to navigate
> there (we already display failed status on a resource's headers tab).

Are you using the existing error count on the right of the resource in the sidebar?

I agree and additional tab is overkill here. There is already the HTTP status code for showing failure in the Headers tab.

[Timothy Hatcher]

Comment on attachment 52752 [details]
Log error message to inspector console if a resource fails to load. Disable checking of mime-type consistency for failed resource. 


> +    String message = "Failed to load resource " + url;
> +    if (status)
> +        message += String::format(", status: %d", status);
> +
> +    addMessageToConsole(OtherMessageSource, LogMessageType, ErrorMessageLevel, message, 0, url);

I think it might be best to do this in the front-end, then the message can be localized. But that isn't as important.

The message could be clearer, something like:

     Failed to load resource. The server responded with a status of %u.

Consider using format for both variations of the message instead of appending strings. I don't think you need to include the URL in the message since the URL is automatically included as meta data and shown as a link with the message.

> +    void reportResourceError(const String& url, int status = 0);

Status should be unsigned.

> +        // If status is an error, content is likely to be of an inconsistent type,
> +        // as it's going to be an error message. We do not want to emit a warning
> +        // for this, though, as this will already be reported as resource loading
> +        // failure.

It is unfornanate to have a widow in comments like this. It is less annoying to have longer lines than to have widows, IMHO.

> -        resource.warnings = 0;
> -        resource.errors = 0;

Why is this needed?

[Andrey Kosyakov]

(In reply to comment #5)
> (From update of attachment 52752 [details])
> 
> > +    String message = "Failed to load resource " + url;
> > +    if (status)
> > +        message += String::format(", status: %d", status);
> > +
> > +    addMessageToConsole(OtherMessageSource, LogMessageType, ErrorMessageLevel, message, 0, url);
> 
> I think it might be best to do this in the front-end, then the message can be
> localized. But that isn't as important.
> 

I did consider making it in front-end to enable localization, but it would be an effort of a different scale -- we need either:
- introduce another call to front-end, along with some buffering for these events in InspectorController in case front-end is not yet attached;
- introduce special console message type to piggy-back existing console message transmission & buffering (a bit of a hack, perhaps).

I wasn't sure whether the added mess would be justified by increased functionality, so I'd rather leave it to reviewers discretion ;)

> The message could be clearer, something like:
> 
>      Failed to load resource. The server responded with a status of %u.
>
> Consider using format for both variations of the message instead of appending
> strings. I don't think you need to include the URL in the message since the URL
> is automatically included as meta data and shown as a link with the message.

Agree, fixed.

> > +    void reportResourceError(const String& url, int status = 0);
> 
> Status should be unsigned.

I made it int as it happened to be int in ResourceResposeBase. Just being paranoid about the unlikely case someone decides to (ab)use negative statuses (e.g. for network-level errors?)

> > +        // failure.
> 
> It is unfornanate to have a widow in comments like this. It is less annoying to
> have longer lines than to have widows, IMHO.

Fixed.

> > -        resource.warnings = 0;
> > -        resource.errors = 0;
> 
> Why is this needed?

For XHRs, we happen to change resource type quite late (after didReceiveResponse), and this used to reset errors/warnings counters. We only call recreateViewForResourceIfNeeded in the only place, in Resource's setter for category, so I thought resetting errors/warnings counters there is not appropriate there and removing it wouldn't harm.

[Andrey Kosyakov]

Created attachment 52769 [details]
Log error message to inspector console if a resource fails to load. Disable checking of mime-type consistency for failed resource (fixed messages)

[Timothy Hatcher]

Comment on attachment 52769 [details]
Log error message to inspector console if a resource fails to load. Disable checking of mime-type consistency for failed resource (fixed messages)


> +    // Safari doesn't report resource URL, so do our best by retrieving it from our
> +    // resource record (obviously, works only with resource tracking enabled).

It isn't Safari at this level. It would be CFNetwork. I am surprised the URL isn't there, since we do show errors in Safari's activity monitor just fine and it uses a similar code path as the Inspector's didFailLoading.

Otherwise r+.

[Patrick Mueller]

re: comment 2: "Do you have a use case in mind that where a resource loading failure is "normal" and would cause lots of messages to be logged?"

I didn't mean to imply I thought resource loading was "normal", more that I've heard other complaints about excess messages being printed to the console; for example XHR completion messages.  Used to be, that was the only way to provide information to the user; today there are a variety of other panels.  I tend to think of the console now as just a REPL, but it's serving dual purpose as both a logging device and a REPL.

re: comment 4: "There is already the HTTP status code for showing failure in the Headers tab."

Which isn't appropriate if the resource never got an HTTP response.  Examples: XHR requests that result in xhr.status == 0 (when you can't connect to the server, and probably other cases); resource loads in an app-cache environment where the resource isn't listed in the manifest (no request is ever made of the resource, but it shows up in the resource list - as it should).

In both of these cases, there is additional information that we can provide, in theory, about why the resource wasn't loaded.  Let's say we can actually determine the reason why an xhr.status == 0 occurred, say ECONNREFUSED when trying to access the resource - where would we provide that information?  The app-cache example would require a sentence or two explaining the problem ("a resource was requested which should have been listed in the manifest, but wasn't, so the resource not loaded"). 

I think this sort of information could go in the headers tab, although I suppose if there's no content, it could go in the content tab as well.  Or both.

[Andrey Kosyakov]

Created attachment 52870 [details]
Log error message to inspector console if a resource fails to load. Disable checking of mime-type consistency for failed resource (added localized error details)

Thanks for the hint, I guess I gave up a little bit to early yesterday -- it transpired to be a fairly trivial bug in WebCore platform code for mac (see http://webkit.org/b/37274). Fixed this, along with localized HTTP statuses as a bonus. Unfortunately, I can't use String::format() now, as it happened not to support UTF-8 (sic!)

[Andrey Kosyakov]

(In reply to comment #9)
> re: comment 2: "Do you have a use case in mind that where a resource loading
> failure is "normal" and would cause lots of messages to be logged?"
> 
> I didn't mean to imply I thought resource loading was "normal", more that I've
> heard other complaints about excess messages being printed to the console; for
> example XHR completion messages.  

I disable logging XHR messages to console a few days ago, so there's going to be way less noise now. The key difference to logging resource failures is that these are of much greater importance and usually mean something went wrong -- much as JS errors that we also log there.

> re: comment 4: "There is already the HTTP status code for showing failure in
> the Headers tab."
> 
> Which isn't appropriate if the resource never got an HTTP response.  Examples:
> XHR requests that result in xhr.status == 0 (when you can't connect to the
> server, and probably other cases); resource loads in an app-cache environment
> where the resource isn't listed in the manifest (no request is ever made of the
> resource, but it shows up in the resource list - as it should).

That's not the case where we used to log those messages -- they were logged only when we got an erroneous HTTP response.

> In both of these cases, there is additional information that we can provide, in
> theory, about why the resource wasn't loaded.  Let's say we can actually
> determine the reason why an xhr.status == 0 occurred, say ECONNREFUSED when
> trying to access the resource - where would we provide that information?  The

I like the idea to provide details on network-level failures on resource tab -- I'll have a look.

[Pavel Feldman]

Comment on attachment 52870 [details]
Log error message to inspector console if a resource fails to load. Disable checking of mime-type consistency for failed resource (added localized error details)

Following code workds all over the place in WebCore, why is your case so special?

String::format("Failed to load resource: the server responded with a status of %u (%s)", response.httpStatusCode(), response.httpStatusText().utf8().data());

[Andrey Kosyakov]

(In reply to comment #12)
> (From update of attachment 52870 [details])
> Following code workds all over the place in WebCore, why is your case so
> special?
> 
> String::format("Failed to load resource: the server responded with a status of
> %u (%s)", response.httpStatusCode(), response.httpStatusText().utf8().data());

I can't help finding "code works all over the place in WebCore" to be a bit of overstatement. The way I implemented initially was exactly as you suggested, and here's what happens: String::format() uses StringImpl::create() to create a String, which treats input as ASCII characters and just performs byte-to-word expansion over it, mangling UTF-8. I filed http://webkit.org/b/37327 on this.

[Andrey Kosyakov]

Created attachment 52962 [details]
Log error message to inspector console if a resource fails to load. Disable checking of mime-type consistency for failed resource (added tests)

[WebKit Commit Bot]

Comment on attachment 52962 [details]
Log error message to inspector console if a resource fails to load. Disable checking of mime-type consistency for failed resource (added tests)

Clearing flags on attachment: 52962

Committed r57609: <http://trac.webkit.org/changeset/57609>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57609 might have broken Qt Linux Release

[Eric Seidel (no email)]

Is Qt's inspector implementation wrong?
http://build.webkit.org/results/Qt%20Linux%20Release/r57609%20(10105)/inspector/console-resource-errors-pretty-diff.html

[Eric Seidel (no email)]

Looks like this broke on SL too:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r57609%20(8298)/inspector/console-resource-errors-pretty-diff.html

Do these all need platform specific results, or should we roll this out?

[Csaba Osztrogonác]

I don't think it is a good idea that inspector/console-resource-errors.html
test make XMLHttpRequest, because here http server is stopped. I think it should be moved under http/tests/...

[Eric Seidel (no email)]

Was rolled out.

[WebKit Review Bot]

http://trac.webkit.org/changeset/57610 might have broken Leopard Intel Debug (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/57610
http://trac.webkit.org/changeset/57611

[Andrey Kosyakov]

Created attachment 53546 [details]
patch

Sorry for broken tests -- this is due to error messages for file:// resources being very platform-specific. I moved the test to http now, this makes error messages consistent for all platforms.

[WebKit Commit Bot]

Comment on attachment 53546 [details]
patch

Clearing flags on attachment: 53546

Committed r58318: <http://trac.webkit.org/changeset/58318>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58318 might have broken Tiger Intel Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/58318
http://trac.webkit.org/changeset/58319

[Eric Seidel (no email)]

This broke Tiger:
http://build.webkit.org/results/Tiger%20Intel%20Release/r58319%20(11315)/http/tests/inspector/console-resource-errors-diffs.txt
--- /Volumes/Data/WebKit-BuildSlave/tiger-intel-release/build/layout-test-results/http/tests/inspector/console-resource-errors-expected.txt	2010-04-27 12:32:32.000000000 -0700
+++ /Volumes/Data/WebKit-BuildSlave/tiger-intel-release/build/layout-test-results/http/tests/inspector/console-resource-errors-actual.txt	2010-04-27 12:32:32.000000000 -0700
@@ -1,7 +1,7 @@
  Tests that errors to load a resource cause error messages to be logged to console.
 
-missing.cssFailed to load resource: the server responded with a status of 404 (Not Found) console-message console-other-source console-error-level
-non-existent-iframe.htmlFailed to load resource: the server responded with a status of 404 (Not Found) console-message console-other-source console-error-level
-non-existent-script.jsFailed to load resource: the server responded with a status of 404 (Not Found) console-message console-other-source console-error-level
-non-existent-xhrFailed to load resource: the server responded with a status of 404 (Not Found) console-message console-other-source console-error-level
+missing.cssFailed to load resource: the server responded with a status of 404 (OK) console-message console-other-source console-error-level
+non-existent-iframe.htmlFailed to load resource: the server responded with a status of 404 (OK) console-message console-other-source console-error-level
+non-existent-script.jsFailed to load resource: the server responded with a status of 404 (OK) console-message console-other-source console-error-level
+non-existent-xhrFailed to load resource: the server responded with a status of 404 (OK) console-message console-other-source console-error-level

[Eric Seidel (no email)]

Created rollout bug.

[Eric Seidel (no email)]

Reverted r58318 for reason:

Broke test on Tiger.  Might just need updated results, unclear.

Committed r58342: <http://trac.webkit.org/changeset/58342>

[Andrey Kosyakov]

Created attachment 54543 [details]
patch; added test expectation overrides for mac-tiger

See https://bugs.webkit.org/show_bug.cgi?id=37274 and https://bugs.webkit.org/show_bug.cgi?id=24572 on why we need expectations override for Tiger.

[WebKit Commit Bot]

Comment on attachment 54543 [details]
patch; added test expectation overrides for mac-tiger

Clearing flags on attachment: 54543

Committed r58403: <http://trac.webkit.org/changeset/58403>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.