Bug 146089 – [Content Extensions] Log blocked loads to the WebInspector console

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alex Christensen]

People are asking "Why isn't this showing up?" and there's nothing in the WebInspector.  Let's tell them.

[Alex Christensen]

Created attachment 255052 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 255052 [details]
Patch

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

r=me, I'd be surprised if tests don't include logs now. Do we have contentblocker tests?

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:219
> +            StringBuilder message;
> +            message.append("Content blocker prevented loading of ");
> +            message.append(request.url());
> +            message.append(" on ");
> +            message.append(mainDocumentURL);

Could this just be a makeString(...) list? If not, some of these appends could be appendLiteral().

[Joseph Pecoraro]

(In reply to comment #2)
> Comment on attachment 255052 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255052&action=review
> 
> r=me, I'd be surprised if tests don't include logs now. Do we have
> contentblocker tests?

Seems this did cause some text failures on the bots, so seems tests may just need to be updated.

Regressions: Unexpected text-only failures (8)
  http/tests/contentextensions/basic-filter.html [ Failure ]
  http/tests/contentextensions/character-set-basic-support.html [ Failure ]
  http/tests/contentextensions/domain-rules.html [ Failure ]
  http/tests/contentextensions/filters-with-quantifiers-combined.html [ Failure ]
  http/tests/contentextensions/main-resource-redirect-blocked.php [ Failure ]
  http/tests/contentextensions/media-filtered.html [ Failure ]
  http/tests/contentextensions/subresource-redirect-blocked.html [ Failure ]
  http/tests/contentextensions/text-track-blocked.html [ Failure ]

[Alex Christensen]

(In reply to comment #2)
> Could this just be a makeString(...) list? If not, some of these appends could be appendLiteral().
makeString doesn't like URLs.  I'll use appendLiteral.  And I'll check the tests...

[Build Bot]

Comment on attachment 255052 [details]
Patch

Attachment 255052 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4541667524739072

New failing tests:
http/tests/contentextensions/domain-rules.html
http/tests/contentextensions/main-resource-redirect-blocked.php
http/tests/contentextensions/text-track-blocked.html
http/tests/contentextensions/subresource-redirect-blocked.html
http/tests/contentextensions/media-filtered.html
http/tests/contentextensions/character-set-basic-support.html
http/tests/contentextensions/basic-filter.html
http/tests/contentextensions/filters-with-quantifiers-combined.html

[Build Bot]

Created attachment 255057 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

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

[Alex Christensen]

Created attachment 255060 [details]
Patch

[Alex Christensen]

The changes to main-resource-redirect-blocked-expected.txt look a little suspicious.  I think it's a bug in WebKitTestRunner, but we still want to see those previous logs.  Brady, could you take a look and see what is going on?

[Brady Eidson]

(In reply to comment #4)
> (In reply to comment #2)
> > Could this just be a makeString(...) list? If not, some of these appends could be appendLiteral().
> makeString doesn't like URLs.  I'll use appendLiteral.  And I'll check the
> tests...

URL myURL;
makeString("Are you sure that this ", myURL.string(), " doesn't work...?");

[Brady Eidson]

Comment on attachment 255060 [details]
Patch

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

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:223
> +    if (willBlockLoad) {
> +        if (currentDocument) {
> +            StringBuilder message;
> +            message.appendLiteral("Content blocker prevented loading of ");
> +            message.append(request.url());
> +            message.appendLiteral(" on ");
> +            message.append(mainDocumentURL);
> +            currentDocument->addConsoleMessage(MessageSource::ContentBlocker, MessageLevel::Info, message.toString());
> +        }
>          request = ResourceRequest();
> +    }

I think Joe's suggestion of makeString() should be perfectly fine here.

No, makeString doesn't handle URLs. But URLs are just fancy strings:
makeString("Content blocker prevented loading of ", request.url().string(), " on ", mainDocumentURL.string());

---

*THAT SAID*, I don't like this message. "Content blocker prevented loading of http://foo.com on http://bar.com" <--- This doesn't make sense. You don't load a URL onto another URL.

The blocked resource could be a document, or it could be a sub resource, but it's not "a url".

Additionally, you don't load a <resource located at a url> *on* to a <url>

The grammar just reads incorrectly.

My suggestion - that is off the cuff and might be improved:
"Content blocker prevented frame displaying http://bar.com from loading the resource at http://foo.com/someResource"

> LayoutTests/http/tests/contentextensions/main-resource-redirect-blocked-expected.txt:1
> -main frame - didStartProvisionalLoadForFrame
> -main frame - didFailProvisionalLoadWithError
> -main frame - (kWKErrorCodeCannotShowURL)
> +CONSOLE MESSAGE: Content blocker prevented loading of http://127.0.0.1:8000/contentextensions/resources/main-resource-redirect-blocked-target.html on about:blank

Thanks for alerting me to this one. This is wrong. I don't see how your patch causes it, but it's definitely wrong.

We'll take a look tomorrow on your machine that (presumably) has this built in a debug build already.

[Brady Eidson]

(In reply to comment #10)
> My suggestion - that is off the cuff and might be improved:
> "Content blocker prevented frame displaying http://bar.com from loading the
> resource at http://foo.com/someResource"

Even better:
"Content blocker prevented frame displaying http://bar.com from loading a resource from http://foo.com/someResource"

[Brady Eidson]

(In reply to comment #11)
> (In reply to comment #10)
> > My suggestion - that is off the cuff and might be improved:
> > "Content blocker prevented frame displaying http://bar.com from loading the
> > resource at http://foo.com/someResource"
> 
> Even better:
> "Content blocker prevented frame displaying http://bar.com from loading a
> resource from http://foo.com/someResource"

Bonus points for saying what type of resource.

"...from loading an image resource from..."
"...from loading a stylesheet resource from..."
"...from loading the document found at..."

[Alex Christensen]

used makeString, updated test results.
http://trac.webkit.org/changeset/185714