Blocked redirected main resource requests need descriptive errors
Created attachment 276876 [details] Patch
Created attachment 276878 [details] Patch
This is close, but the use of cancelMainResourceLoad leaves something in an inconsistent state, and it crashes some of the tests later.
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
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
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.
Created attachment 278141 [details] Patch
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.
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".
http://trac.webkit.org/changeset/200461