Bug 149719 – Report error when main resource is blocked by content blocker

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Report error when main resource is blocked by content blocker

[Alex Christensen]

Created attachment 262282 [details]
Patch

[Alex Christensen]

rdar://problem/21970595

[Alex Christensen]

Created attachment 262298 [details]
Patch

[Brady Eidson]

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]

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]

Created attachment 262308 [details]
Patch

[Alex Christensen]

Created attachment 262315 [details]
Patch

[Darin Adler]

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]

(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]

Created attachment 262337 [details]
Patch

[Brady Eidson]

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]

(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]

Created attachment 262507 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 262507 [details]
Patch

Clearing flags on attachment: 262507

Committed r190611: <http://trac.webkit.org/changeset/190611>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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]

Fix landed in https://trac.webkit.org/changeset/190612

[Csaba Osztrogonác]

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]

Sorry about that.  Fixed Windows build in http://trac.webkit.org/changeset/190614