Bug 55764 – Web Inspector: resource load cancellation is reported to console as an error

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

[Andrey Kosyakov]

1. go to https://chrome.google.com/extensions
2. open js console
3. click "install"

See the "failure to load resource" message

Original bug report:
http://code.google.com/p/chromium/issues/detail?id=62888

[Andrey Kosyakov]

Created attachment 84735 [details]
patch

[Pavel Feldman]

Comment on attachment 84735 [details]
patch

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

> Source/WebCore/loader/MainResourceLoader.cpp:122
> +    error.setIsCancellation(true);

I am not sure what this would result in. You should call for more reviewers. Adding ap for now.

[Alexey Proskuryakov]

Good question. I spent a couple of minutes looking into this, and couldn't easily answer if this this is OK - Safari uses PolicyIgnore in many cases. Will look again later.

How does this affect bug 43614 and bug 38346?

Can this be tested? See fast/loader/onload-policy-ignore-for-frame.html for an example of policy delegate test.

[Alexey Proskuryakov]

Comment on attachment 84735 [details]
patch

I think that this change is acceptable. It's a mess how WebCore decides what a cancellation is, but this is not making it worse.

However, I have one more question in addition to ones in comment 3. Why does ResourceLoader::didCancel() even call didFailToLoad()? Besides logging an error to Inspector, that also calls FrameLoaderClient::dispatchDidFailLoading(), and that also looks wrong at a first glance. If there is some historic reason, there should probably be a comment.

Since this needs more work (at least a test), I'm marking r- for now.

[Alexey Proskuryakov]

+        - fixed error formatting in case resource error code not present

I've noticed that error is completely broken in Safari nightlies when installing a Safari extension from a local file:

GET file:///Users/ap/Downloads/AdBlockForSafari.safariextz undefined (undefined)

In Safari 5.0.3, it's:

Failed to load resource: Загрузка фрейма прервана

Should I file a separate bug about that? Clearly, we shouldn't report any error, but could this be a general problem with console logging?

[Andrey Kosyakov]

(In reply to comment #5)
> +        - fixed error formatting in case resource error code not present
> 
> I've noticed that error is completely broken in Safari nightlies when installing a Safari extension from a local file:
> 
> GET file:///Users/ap/Downloads/AdBlockForSafari.safariextz undefined (undefined)
> 
> In Safari 5.0.3, it's:
> 
> Failed to load resource: Загрузка фрейма прервана
> 
> Should I file a separate bug about that? Clearly, we shouldn't report any error, but could this be a general problem with console logging?

I don't think this is worth an additional bug -- this change fixes both the error message format in case there's no error code (see the diff for ConsoleView.js) and reporting cancellation as an error. 

Thanks for mentioning bug 43614 and bug 38346 -- unfortunately I missed them. The latter is fixed by this change, and as for the former -- well, the poor "canceled" message is gone with this change, and someone else apparently added (brought back?) proper diagnostics -- so this does not reproduce anymore (I added a test for that).

> However, I have one more question in addition to ones in comment 3. 
> Why does ResourceLoader::didCancel() even call didFailToLoad()? 
> Besides logging an error to Inspector, that also calls FrameLoaderClient::dispatchDidFailLoading(), and that also looks wrong at a first glance. 
> If there is some historic reason, there should probably be a comment.

Well, my guess would be that this is indeed historical, probably done for the lack of separate notification method for cancel in FrameLoaderClient. While this looks like a misnomer to me, having one method for both cancel and fail is probably appropriate, since for most of the clients handling of an aborted request is perhaps similar no matter whether the reason is cancellation or early failure.

[Andrey Kosyakov]

Created attachment 85214 [details]
patch

- added a test for lack of console errors in case of policy change
- added a test for cross-origin XHR error
- rebaselined

[Alexey Proskuryakov]

OK, thanks for the investigation! Please do resolve the other bugs as appropriate.

[Andrey Kosyakov]

Created attachment 85318 [details]
patch (fixed binary diff)

Well, if everyone is happy, does anyone mind r+'ing this? ;-)

[Andrey Kosyakov]

*** Bug 43614 has been marked as a duplicate of this bug. ***

[Pavel Feldman]

Comment on attachment 85318 [details]
patch (fixed binary diff)

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

Please address my comment prior to landing.

> Source/WebCore/inspector/front-end/ConsoleView.js:726
> +                    else if (resource.statusCode)

You should make sure InspectorInstrumentation marks resources as failed prior to reporting error to the console. Then you can revert this change.

[Andrey Kosyakov]

Created attachment 85447 [details]
landed patch

[Andrey Kosyakov]

Manually committed r80832: http://trac.webkit.org/changeset/80832