Bug 151649 – Asynchronously call onerror when a content blocker blocks a script element's load

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

[Andrey Meshkov]

There is an issue with the way content blockers present blocked requests to the requesters.

Usually (e.g. for all other browsers) when request is blocked by add-on or extension it looks like an error to the page code. For instance, "onerror" is called for XMLHttpRequest and "element.onerror" callback is called for the DOM element if it's load is blocked by the ad blocker.

This is a common behavior and it's handled by web developers.

In case of Safari content blocker "onerror" event is not raised and it may seem that request is successful while it is not.

Here is an example.

This is a rule blocking access to visualwebsiteoptimizer.com domain.
This domain is a known tracker so you can see this rule in all "privacy" related filter lists like EasyPrivacy and such.
[
	{
		"trigger": {
			"url-filter": "^https?://[^.]+\\.?visualwebsiteoptimizer\\.com[/:&?]?",
			"load-type": [
				"third-party"
			]
		},
		"action": {
			"type": "block"
		}
	}
]

But it can't be used in Safari because the "silent" blocking breaks entire website which use visualwebsiteoptimizer.com. Examples of such websites: 
http://info.singtel.com
http://www.harveynorman.com.au

Here is a code used by them:
http://pastebin.com/awYa9s95

As you can see they handle "onerror" callback, but it's not fired in Safari so the "body" element remains hidden.

Btw, this issue also causes significant delays when ads are blocked on Youtube. They also are waiting for "onerror" callback to fire, but as it is not fired, video does not start until "ontimeout" is fired.

[Alex Christensen]

Created attachment 266459 [details]
Patch

[Alex Christensen]

(In reply to comment #0)
Thank you for such a great, reduced, repeatable bug report!
> As you can see they handle "onerror" callback, but it's not fired in Safari
> so the "body" element remains hidden.
The onerror callback was being called immediately, in this case before the style element had been added to the head.  This was causing finish to be called, which tried to remove the style element before the style element existed, then it would create and add the style element.  This caused the entire page to have 0 opacity.  This patch fixes that.
We've also seen something like this when blocking fonts, but I haven't been able to figure out what is happening yet.
> 
> Btw, this issue also causes significant delays when ads are blocked on
> Youtube. They also are waiting for "onerror" callback to fire, but as it is
> not fired, video does not start until "ontimeout" is fired.
This was fixed in http://trac.webkit.org/changeset/191077

[Andrey Meshkov]

Great, I am glad I was been able to help! Thank you for the prompt reaction!

[Alex Christensen]

Please let me know if you have any reduced test cases causing a similar 0-opacity-body behavior when blocking all fonts.  I still need to look into that case.

[Andrey Meshkov]

I can provide more examples, but they all are about the same script (i mean visualwebsiteoptimizer.com). Is it ok for you?

[Andrey Meshkov]

(In reply to comment #4)
> Please let me know if you have any reduced test cases causing a similar
> 0-opacity-body behavior when blocking all fonts.  I still need to look into
> that case.

Here is one more example:
http://www.onlime.ru/

[Build Bot]

Comment on attachment 266459 [details]
Patch

Attachment 266459 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/508051

New failing tests:
http/tests/security/local-JavaScript-from-remote.html
http/tests/misc/unloadable-script.html

[Build Bot]

Created attachment 266462 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266459 [details]
Patch

Attachment 266459 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/508053

New failing tests:
http/tests/security/local-JavaScript-from-remote.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 266459 [details]
Patch

Attachment 266459 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/508057

New failing tests:
http/tests/security/local-JavaScript-from-remote.html
http/tests/misc/unloadable-script.html

[Build Bot]

Created attachment 266465 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 266459 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 266459 [details]:

transitions/default-timing-function.html bug 138901 (author: [e-mail])
The commit-queue is continuing to process your patch.

[Andrey Meshkov]

(In reply to comment #1)
> Created attachment 266459 [details]
> Patch

As I understand you handle only "script" blocking in this asynchronous way.
Shouldn't it be called for other elements in the same way?

I've done a quick check and it seems it is called properly for "img".
But it does not called at all for "link" element.

Here is an example:
http://jsfiddle.net/82ez45js/11/

Block webkit.org to see it.

[Alex Christensen]

Created attachment 266472 [details]
Patch

[Alex Christensen]

Created attachment 266479 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 266479 [details]
Patch

Clearing flags on attachment: 266479

Committed r192983: <http://trac.webkit.org/changeset/192983>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 266479 [details]
Patch

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

> Source/WebCore/dom/ScriptElement.h:113
> +    Timer m_errorEventTimer;

A timer is huge. Do we really want to add one to every script element?

[Alex Christensen]

sizeof(Timer) is 112 bytes
sizeof(ScriptElement) without Timer size: 56 bytes
So this exactly tripled the size of a ScriptElement :(
I'm planning to use a std::unique_ptr<Timer> instead.

[Alex Christensen]

I get all the memory back in https://bugs.webkit.org/show_bug.cgi?id=151786

[Andrey Meshkov]

Alex, what about comment #15 (the same issue with "link" elements)?

Regarding the memory, just a question - will it help to initialize this timer only when you need it (e.g. before startOneShot call)?

[Alex Christensen]

(In reply to comment #23)
> Alex, what about comment #15 (the same issue with "link" elements)?
That'll need to be a separate bug.
> 
> Regarding the memory, just a question - will it help to initialize this
> timer only when you need it (e.g. before startOneShot call)?
We could make a pointer to a Timer that is only used if necessary, and that would add less memory.  That was my first approach in https://bugs.webkit.org/show_bug.cgi?id=151786 but it turns out I don't need a timer at all.

[Andrey Meshkov]

(In reply to comment #24)
> That'll need to be a separate bug.

Done:
https://bugs.webkit.org/show_bug.cgi?id=151815

[Radar WebKit Bug Importer]

<rdar://problem/24281137>