Bug 37215

Summary: Web Inspector: be more explicit about resource loading errors
Product: WebKit Reporter: Andrey Kosyakov <caseq>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Minor CC: abarth, bweinstein, commit-queue, eric, joepeck, keishi, ossy, pfeldman, pmuellr, rik, timothy, webkit.review.bot
Priority: P4    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 37614, 38214    
Bug Blocks:    
Attachments:
Description Flags
Log error message to inspector console if a resource fails to load. Disable checking of mime-type consistency for failed resource.
timothy: review-
Log error message to inspector console if a resource fails to load. Disable checking of mime-type consistency for failed resource (fixed messages)
timothy: review-
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)
none
Log error message to inspector console if a resource fails to load. Disable checking of mime-type consistency for failed resource (added tests)
none
patch
none
patch; added test expectation overrides for mac-tiger none

Description Andrey Kosyakov 2010-04-07 09:06:17 PDT
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.
Comment 1 Patrick Mueller 2010-04-07 09:18:52 PDT
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?
Comment 2 Andrey Kosyakov 2010-04-07 10:38:02 PDT
(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).
Comment 3 Andrey Kosyakov 2010-04-07 10:43:48 PDT
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.
Comment 4 Timothy Hatcher 2010-04-07 10:47:51 PDT
(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.
Comment 5 Timothy Hatcher 2010-04-07 11:07:56 PDT
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?
Comment 6 Andrey Kosyakov 2010-04-07 12:12:40 PDT
(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.
Comment 7 Andrey Kosyakov 2010-04-07 12:23:57 PDT
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)
Comment 8 Timothy Hatcher 2010-04-07 12:57:57 PDT
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+.
Comment 9 Patrick Mueller 2010-04-07 21:22:15 PDT
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.
Comment 10 Andrey Kosyakov 2010-04-08 08:31:34 PDT
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!)
Comment 11 Andrey Kosyakov 2010-04-08 08:41:40 PDT
(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.
Comment 12 Pavel Feldman 2010-04-09 05:03:44 PDT
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());
Comment 13 Andrey Kosyakov 2010-04-09 06:07:18 PDT
(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.
Comment 14 Andrey Kosyakov 2010-04-09 09:55:25 PDT
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)
Comment 15 WebKit Commit Bot 2010-04-14 14:49:12 PDT
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>
Comment 16 WebKit Commit Bot 2010-04-14 14:49:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 WebKit Review Bot 2010-04-14 15:07:55 PDT
http://trac.webkit.org/changeset/57609 might have broken Qt Linux Release
Comment 18 Eric Seidel (no email) 2010-04-14 15:12:40 PDT
Is Qt's inspector implementation wrong?
http://build.webkit.org/results/Qt%20Linux%20Release/r57609%20(10105)/inspector/console-resource-errors-pretty-diff.html
Comment 19 Eric Seidel (no email) 2010-04-14 15:13:45 PDT
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?
Comment 20 Csaba Osztrogonác 2010-04-14 15:23:07 PDT
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/...
Comment 21 Eric Seidel (no email) 2010-04-14 16:16:28 PDT
Was rolled out.
Comment 22 WebKit Review Bot 2010-04-14 16:23:36 PDT
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
Comment 23 Andrey Kosyakov 2010-04-16 11:48:02 PDT
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.
Comment 24 WebKit Commit Bot 2010-04-27 11:47:54 PDT
Comment on attachment 53546 [details]
patch

Clearing flags on attachment: 53546

Committed r58318: <http://trac.webkit.org/changeset/58318>
Comment 25 WebKit Commit Bot 2010-04-27 11:48:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 WebKit Review Bot 2010-04-27 13:02:58 PDT
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
Comment 27 Eric Seidel (no email) 2010-04-27 13:28:00 PDT
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
Comment 28 Eric Seidel (no email) 2010-04-27 13:30:46 PDT
Created rollout bug.
Comment 29 Eric Seidel (no email) 2010-04-27 15:05:31 PDT
Reverted r58318 for reason:

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

Committed r58342: <http://trac.webkit.org/changeset/58342>
Comment 30 Andrey Kosyakov 2010-04-28 02:58:37 PDT
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.
Comment 31 WebKit Commit Bot 2010-04-28 04:49:15 PDT
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>
Comment 32 WebKit Commit Bot 2010-04-28 04:49:24 PDT
All reviewed patches have been landed.  Closing bug.