Bug 156828 – Blocked redirected main resource requests need descriptive errors

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Blocked redirected main resource requests need descriptive errors

[Alex Christensen]

Created attachment 276876 [details]
Patch

[Alex Christensen]

Created attachment 276878 [details]
Patch

[Alex Christensen]

This is close, but the use of cancelMainResourceLoad leaves something in an inconsistent state, and it crashes some of the tests later.

[Build Bot]

Comment on attachment 276878 [details]
Patch

Attachment 276878 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1193654

New failing tests:
http/tests/contentextensions/make-https.html

[Build Bot]

Created attachment 276883 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Daniel Bates]

Comment on attachment 276878 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        Call cancelMainResourceLoad so that didFailProvisionalLoadWithError is called 

Nit: cancelMainResourceLoad => cancelMainResourceLoad()
Nit: didFailProvisionalLoadWithError => InjectedBundlePage::didFailProvisionalLoadWithErrorForFrame()

> Source/WebCore/ChangeLog:21
> +        Use a blockedByContentBlockerError instead of a cannotShowURLError when we blocked the load.

Nit: blockedByContentBlockerError => blockedByContentBlockerError()
Nit: cannotShowURLError => cannotShowURLError()

> Source/WebCore/loader/DocumentLoader.cpp:1508
>          m_mainResource = nullptr;

It is unnecessary to explicitly nullify m_mainResource here as DocumentLoader::clearMainResource() (called via cancelMainResourceLoad()) will ultimately do this for us.

> Source/WebCore/loader/ResourceLoader.cpp:350
>                  request = { };

This line does not do anything meaningful now that we now early return because request is a local variable. We need to update m_request.

> LayoutTests/http/tests/contentextensions/block-everything-if-domain-expected.txt:1
>  CONSOLE MESSAGE: Content blocker prevented frame displaying http://127.0.0.1:8000/contentextensions/block-everything-if-domain.html from loading a resource from http://127.0.0.1:8000/contentextensions/block-everything-if-domain.html

I know that this is not part of the patch. This error message does not make sense. I mean, the message states that the page <http://127.0.0.1:8000/resources/should-not-load.html> is loading itself. But <http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/contentextensions/block-everything-if-domain.html?rev=186807> only contains text (*). That is, it does not contain any markup or scripts that initiate a load. The content blocker is blocking the display of the frame.

Obviously <http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/contentextensions/block-everything-if-domain.html?rev=186807> is not represent a well-formed HTML document. It is sufficient in the context of this test, which tests that the content blocker blocks the display of the page.

> LayoutTests/http/tests/contentextensions/main-resource-redirect-error-expected.txt:7
> +CONSOLE MESSAGE: Content blocker prevented frame displaying http://127.0.0.1:8000/resources/should-not-load.html from loading a resource from http://127.0.0.1:8000/resources/should-not-load.html

This error message does not make sense. I mean, the message states that the non-existent page <http://127.0.0.1:8000/resources/should-not-load.html> is loading itself.

> LayoutTests/http/tests/contentextensions/main-resource-redirect-error.html:1
> +<script>

Although this file renders empty visually if it were shown and our HTML parser is smart enough to interpret this document correctly, this document makes use of HTML5's <script> semantics (so as to omit the attributes type) and is not well-formed. I suggest that we add <!DOCTYPE html> to the top of this file to make it a well-formed HTML5 document.

[Alex Christensen]

Created attachment 278141 [details]
Patch

[Alex Christensen]

Addressed review feedback with two exceptions:
(In reply to comment #6)
> > Source/WebCore/loader/ResourceLoader.cpp:350
> >                  request = { };
> 
> This line does not do anything meaningful now that we now early return
> because request is a local variable. We need to update m_request.
request is a ResourceRequest& passed in as a parameter.  This line nulls out the request in the calling function.  This is needed or else plugin-doesnt-crash.html crashes.

> > LayoutTests/http/tests/contentextensions/block-everything-if-domain-expected.txt:1
> >  CONSOLE MESSAGE: Content blocker prevented frame displaying http://127.0.0.1:8000/contentextensions/block-everything-if-domain.html from loading a resource from http://127.0.0.1:8000/contentextensions/block-everything-if-domain.html
> 
> I know that this is not part of the patch. This error message does not make
> sense. I mean, the message states that the page
> <http://127.0.0.1:8000/resources/should-not-load.html> is loading itself.

That is the main document url and the current request's url, the two urls used to determine if a request should be blocked.  They are both useful to developers, but they are the same if we are loading the main resource.  We could change the text of this debugging message, but if so we should do that in a different patch.

[Daniel Bates]

Comment on attachment 278141 [details]
Patch

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

> LayoutTests/http/tests/contentextensions/block-everything-unless-domain.html:13
> +            console.log("blocking load succeeded");

Very minor. This message does not read well. For your consideration, I suggest using the words PASS and FAIL to convey test success and failure, respectively, as opposed to the words "succeed" and "unexpectedly". If this test was using js-tests-pre/post.js such language is emitted when using its assertion methods, including testPassed()/testFailed(). Maybe something like "PASS did block load" or "PASS load blocked".

> LayoutTests/http/tests/contentextensions/block-everything-unless-domain.html:15
> +            console.log("unexpectedly loaded a blocked url");

Nit: url => URL

Very minor. You may want to consider using the word FAIL instead of "unexpectedly" to convey that the test failed. Maybe something like "FAIL did not block load".

[Alex Christensen]

http://trac.webkit.org/changeset/200461