Bug 156828

Summary:

Blocked redirected main resource requests need descriptive errors

Product:

WebKit

Reporter:

Alex Christensen <achristensen>

Component:

New Bugs

Assignee:

Alex Christensen <achristensen>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, beidson, buildbot, cdumez, commit-queue, dbates, japhet, rniwa

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Archive of layout-test-results from ews105 for mac-yosemite-wk2	none
Patch	dbates: review+

Alex Christensen

Reported 2016-04-20 17:42:22 PDT

Blocked redirected main resource requests need descriptive errors

Attachments
Patch (7.83 KB, patch) 2016-04-20 17:45 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Patch (12.05 KB, patch) 2016-04-20 18:12 PDT, Alex Christensen	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.46 MB, application/zip) 2016-04-20 18:58 PDT, Build Bot	no flags	Details
Patch (14.12 KB, patch) 2016-05-04 16:32 PDT, Alex Christensen	dbates: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Alex Christensen

Comment 1 2016-04-20 17:45:34 PDT

Created attachment 276876 [details] Patch

Alex Christensen

Comment 2 2016-04-20 18:12:24 PDT

Created attachment 276878 [details] Patch

Alex Christensen

Comment 3 2016-04-20 18:28:27 PDT

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 4 2016-04-20 18:58:29 PDT

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

Comment 5 2016-04-20 18:58:33 PDT

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 6 2016-04-21 09:57:26 PDT

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

Comment 7 2016-05-04 16:32:17 PDT

Created attachment 278141 [details] Patch

Alex Christensen

Comment 8 2016-05-04 16:37:31 PDT

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 9 2016-05-04 18:41:51 PDT

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

Comment 10 2016-05-05 10:43:07 PDT

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

Note You need to log in before you can comment on or make changes to this bug.