WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
149719
Report error when main resource is blocked by content blocker
https://bugs.webkit.org/show_bug.cgi?id=149719
Summary
Report error when main resource is blocked by content blocker
Alex Christensen
Reported
2015-10-01 12:38:21 PDT
Report error when main resource is blocked by content blocker
Attachments
Patch
(26.54 KB, patch)
2015-10-01 14:08 PDT
,
Alex Christensen
no flags
Details
Formatted Diff
Diff
Patch
(35.17 KB, patch)
2015-10-01 16:42 PDT
,
Alex Christensen
no flags
Details
Formatted Diff
Diff
Patch
(36.81 KB, patch)
2015-10-01 18:18 PDT
,
Alex Christensen
no flags
Details
Formatted Diff
Diff
Patch
(37.39 KB, patch)
2015-10-01 22:41 PDT
,
Alex Christensen
no flags
Details
Formatted Diff
Diff
Patch
(37.92 KB, patch)
2015-10-02 09:23 PDT
,
Alex Christensen
no flags
Details
Formatted Diff
Diff
Patch
(38.68 KB, patch)
2015-10-06 00:31 PDT
,
Alex Christensen
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Alex Christensen
Comment 1
2015-10-01 14:08:46 PDT
Created
attachment 262282
[details]
Patch
Alex Christensen
Comment 2
2015-10-01 14:21:17 PDT
rdar://problem/21970595
Alex Christensen
Comment 3
2015-10-01 16:42:08 PDT
Created
attachment 262298
[details]
Patch
Brady Eidson
Comment 4
2015-10-01 17:15:14 PDT
Comment on
attachment 262298
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=262298&action=review
The structure of this patch seems great. There's enough comments on the details for me to r-
> Source/WebCore/ChangeLog:16 > + Instead of nulling out the ResourceRequest, processContentExtensionRulesForLoad > + now returns a bool indicating whether the request should be blocked. > + This is needed because the DocumentLoader needs the request to make the ResourceError.
This doesn't accurately describe what the change is - That DocumentLoader needs a CachedResource with an error representing the blocking.
> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:148 > -void ContentExtensionsBackend::processContentExtensionRulesForLoad(ResourceRequest& request, ResourceType resourceType, DocumentLoader& initiatingDocumentLoader) > +bool ContentExtensionsBackend::processContentExtensionRulesForLoad(ResourceRequest& request, ResourceType resourceType, DocumentLoader& initiatingDocumentLoader)
Now that we're returning a bool instead of modifying request, request is no longer an out parameter and can be const.
> Source/WebCore/html/HTMLMediaElement.cpp:1229 > + if (page->userContentController() && documentLoader > + && page->userContentController()->processContentExtensionRulesForLoad(*page, request, ResourceType::Media, *documentLoader)) {
These should all be on one line.
> Source/WebCore/loader/DocumentLoader.cpp:1447 > + if (m_mainResource && m_mainResource->errorOccurred() && m_mainResource->resourceError().domain() == "WebKitContentBlockerDomain") {
You have the "WebKitContentBlockerDomain" string literal in multiple places. It needs to exist in one place as a constant String that can be referenced from these multiple places.
> Source/WebCore/loader/FrameLoader.cpp:2720 > + Page* page = nullptr; > + UserContentController* controller = nullptr; > + if (error.isNull() > + && m_documentLoader > + && (page = m_frame.page()) > + && (controller = page->userContentController()) > + && controller->processContentExtensionRulesForLoad(*page, newRequest, ResourceType::Raw, *m_documentLoader)) {
This is messy-ish to look at, especially the non-null-assignment checks in parens. I think the old nested style was fine.
> Source/WebCore/loader/FrameLoader.cpp:3334 > + error.setIsCancellation(true);
Is this right? Logically this is not a cancelled load. Is there actually a reason the cancelled load flag needs to be set?
> Source/WebCore/loader/ResourceLoader.cpp:345 > + if (userContentController > + && userContentController->processContentExtensionRulesForLoad(*page, request, m_resourceType, *m_documentLoader))
On one line please.
> Source/WebCore/loader/cache/CachedResourceLoader.cpp:523 > + resource->setResourceError(ResourceError("WebKitContentBlockerDomain", 0, request.mutableResourceRequest().url().string(), "The URL was blocked by a content blocker"));
Another place where the "WebKitContentBlockerDomain" should come from some shared location. Same with the "The URL was blocked by a content blocker" message. Also the "request.mutableResourceRequest().url().string()" dance seems bizarre - why do you have to get the mutable request?
> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebErrorsMac.mm:72 > + return ResourceError(API::Error::webKitErrorDomain(), kWKErrorCodeFrameLoadBlockedByContentBlocker, request.url(), WEB_UI_STRING("The URL was blocked by a content blocker", "WebKitErrorBlockedByContentBlocker description"));
"The URL was blocked by a content blocker" is now in multiple places, and probably shouldn't be.
Alex Christensen
Comment 5
2015-10-01 18:00:23 PDT
Addressed comments except as noted. I was thinking about using the errorCode instead of the domain of the ResourceError to determine if it was blocked by content blockers, but that isn't done elsewhere and I wasn't sure enough that 104 is a unique number with ResourceErrors. (In reply to
comment #4
) ContentExtensionsBackend::processContentExtensionRulesForLoad(ResourceRequest& request, ResourceType resourceType, DocumentLoader& initiatingDocumentLoader)
> > Now that we're returning a bool instead of modifying request, request is no > longer an out parameter and can be const.
It still can't be const because block-cookies strips the cookies from the request.
> > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebErrorsMac.mm:72 > > + return ResourceError(API::Error::webKitErrorDomain(), kWKErrorCodeFrameLoadBlockedByContentBlocker, request.url(), WEB_UI_STRING("The URL was blocked by a content blocker", "WebKitErrorBlockedByContentBlocker description")); > > "The URL was blocked by a content blocker" is now in multiple places, and > probably shouldn't be.
It needs to be in multiple places, but I made a WEB_UI_STRING in both places.
Alex Christensen
Comment 6
2015-10-01 18:18:42 PDT
Created
attachment 262308
[details]
Patch
Alex Christensen
Comment 7
2015-10-01 22:41:09 PDT
Created
attachment 262315
[details]
Patch
Darin Adler
Comment 8
2015-10-02 08:05:01 PDT
Comment on
attachment 262315
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=262315&action=review
Only had time for a partial review.
> Source/WebCore/loader/DocumentLoader.cpp:1449 > + m_identifierForLoadWithoutResourceLoader = m_frame->page()->progress().createUniqueIdentifier();
What guarantees m_frame is non-null? What guarantees m_frame->page() is non-null?
> Source/WebCore/loader/DocumentLoader.cpp:1451 > + frameLoader()->notifier().assignIdentifierToInitialRequest(m_identifierForLoadWithoutResourceLoader, this, request); > + frameLoader()->notifier().dispatchDidFailLoading(this, m_identifierForLoadWithoutResourceLoader, frameLoader()->blockedByContentBlockerError(m_request));
What guarantees frameLoader() is non-null?
> Source/WebCore/loader/FrameLoader.cpp:3328 > + ResourceError error = m_client.blockedByContentBlockerError(request); > + return error;
No need for a local variable here.
> Source/WebCore/loader/ResourceLoader.cpp:345 > + request = ResourceRequest();
I like to use { } in places like this rather than writing out ResourceRequest().
Alex Christensen
Comment 9
2015-10-02 09:21:39 PDT
(In reply to
comment #8
)
> Comment on
attachment 262315
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=262315&action=review
> > Only had time for a partial review. > > > Source/WebCore/loader/DocumentLoader.cpp:1449 > > + m_identifierForLoadWithoutResourceLoader = m_frame->page()->progress().createUniqueIdentifier(); > > What guarantees m_frame is non-null?
There's an early return on line 1424.
> What guarantees m_frame->page() is > non-null?
I need to add a check.
> > > Source/WebCore/loader/DocumentLoader.cpp:1451 > > + frameLoader()->notifier().assignIdentifierToInitialRequest(m_identifierForLoadWithoutResourceLoader, this, request); > > + frameLoader()->notifier().dispatchDidFailLoading(this, m_identifierForLoadWithoutResourceLoader, frameLoader()->blockedByContentBlockerError(m_request)); > > What guarantees frameLoader() is non-null?
We did an early return if !m_frame, and if m_frame is non-null then frameLoader() always returns non-null. It's not straightforward, but it is guaranteed not to be null.
Alex Christensen
Comment 10
2015-10-02 09:23:10 PDT
Created
attachment 262337
[details]
Patch
Brady Eidson
Comment 11
2015-10-05 11:03:43 PDT
Comment on
attachment 262337
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=262337&action=review
r+ after my comments are handled.
> Source/WebCore/platform/efl/ErrorsEfl.h:60 > + PolicyErrorBlockedByContentBlocker = 104
Please add a comma at the end of the new error type, in prep for a future new one.
> Source/WebCore/platform/gtk/ErrorsGtk.h:53 > + PolicyErrorBlockedByContentBlocker = 104
Please add a comma at the end of the new error type, in prep for a future new one.
> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebErrorsMac.mm:72 > + return ResourceError(API::Error::webKitErrorDomain(), kWKErrorCodeFrameLoadBlockedByContentBlocker, request.url(), WEB_UI_STRING("The URL was blocked by a content blocker", "WebKitErrorBlockedByContentBlocker description"));
You have this same code: WEB_UI_STRING("The URL was blocked by a content blocker", "WebKitErrorBlockedByContentBlocker description") ...twice. No way to share it and make it exist only once?
> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1153 > + RELEASE_ASSERT_NOT_REACHED(); // Content blockers are not enabled in WebKit1. > + return [NSError _webKitErrorWithDomain:WebKitErrorDomain code:WebKitErrorCannotShowURL URL:request.url()];
The compiler should allow you to not have a return value when you use RELEASE_ASSERT_NOT_REACHED, as I believe it's sprinkled with `noreturn` or some similar keyword.
> Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:864 > + RELEASE_ASSERT_NOT_REACHED(); // Content Blockers are not enabled for WK1. > + return ResourceError(String(WebKitErrorDomain), WebKitErrorCannotUseRestrictedPort, request.url().string(), WEB_UI_STRING("Not allowed to use restricted network port", "WebKitErrorCannotUseRestrictedPort description"));
Same comment here about noreturn, but I'm less confident about Visual Studio's compiler.
Brady Eidson
Comment 12
2015-10-05 11:08:04 PDT
(In reply to
comment #11
)
> Comment on
attachment 262337
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=262337&action=review
> > r+ after my comments are handled. >
And one more comment: bool processContentExtensionRulesForLoad(ResourceRequest&, ResourceType, DocumentLoader& initiatingDocumentLoader); This makes no sense. Returning true/false here doesn't match up with the naming or purpose of this function. Additionally, in general using a bool for "did" or "did not" do a thing has fallen out of favor. Please use an enum class to represent "load blocked" and "load not blocked" so code has to explicitly check for one of those values, therefore making it more readable.
Alex Christensen
Comment 13
2015-10-06 00:31:10 PDT
Created
attachment 262507
[details]
Patch
WebKit Commit Bot
Comment 14
2015-10-06 01:18:39 PDT
Comment on
attachment 262507
[details]
Patch Clearing flags on attachment: 262507 Committed
r190611
: <
http://trac.webkit.org/changeset/190611
>
WebKit Commit Bot
Comment 15
2015-10-06 01:18:43 PDT
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 16
2015-10-06 02:40:06 PDT
(In reply to
comment #14
)
> Comment on
attachment 262507
[details]
> Patch > > Clearing flags on attachment: 262507 > > Committed
r190611
: <
http://trac.webkit.org/changeset/190611
>
It broke the EFL build as the red EWS bubble noticed it ... In file included from DerivedSources/WebKit2/include/WebCore/UserContentController.h:1:0, from ../../Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:75: ../../Source/WebCore/page/UserContentController.h:29:37: fatal error: ContentExtensionActions.h: No such file or directory #include "ContentExtensionActions.h"
Csaba Osztrogonác
Comment 17
2015-10-06 02:42:13 PDT
Fix landed in
https://trac.webkit.org/changeset/190612
Csaba Osztrogonác
Comment 18
2015-10-06 02:49:35 PDT
Additionally it broke the Apple Windows, WinCairo and GTK builds too. My patch fixed EFL and GTK build, but Windows builds are still broken. C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\WebCore/UserContentController.h(29): fatal error C1083: Cannot open include file: 'ContentExtensionActions.h': No such file or directory (compiling source file C:\cygwin\home\buildbot\slave\win-release\build\Source\WebKit\win\WebView.cpp) [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebKit\WebKit.vcxproj] C:\Users\Alex\Documents\WinCairoBot\win-cairo-release\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\WebCore/UserContentController.h(29): fatal error C1083: Cannot open include file: 'ContentExtensionActions.h': No such file or directory
Alex Christensen
Comment 19
2015-10-06 07:28:51 PDT
Sorry about that. Fixed Windows build in
http://trac.webkit.org/changeset/190614
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug