Bug 151649 - Asynchronously call onerror when a content blocker blocks a script element's load
Summary: Asynchronously call onerror when a content blocker blocks a script element's ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-11-29 15:54 PST by Andrey Meshkov
Modified: 2016-01-21 10:41 PST (History)
9 users (show)

See Also:


Attachments
Patch (5.18 KB, patch)
2015-12-02 11:51 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (732.19 KB, application/zip)
2015-12-02 12:37 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (764.14 KB, application/zip)
2015-12-02 12:41 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (781.21 KB, application/zip)
2015-12-02 12:47 PST, Build Bot
no flags Details
Patch (10.88 KB, patch)
2015-12-02 14:47 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (8.99 KB, patch)
2015-12-02 15:08 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Meshkov 2015-11-29 15:54:30 PST
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.
Comment 1 Alex Christensen 2015-12-02 11:51:08 PST
Created attachment 266459 [details]
Patch
Comment 2 Alex Christensen 2015-12-02 11:55:33 PST
(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
Comment 3 Andrey Meshkov 2015-12-02 12:23:23 PST
Great, I am glad I was been able to help! Thank you for the prompt reaction!
Comment 4 Alex Christensen 2015-12-02 12:26:45 PST
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.
Comment 5 Andrey Meshkov 2015-12-02 12:28:29 PST
I can provide more examples, but they all are about the same script (i mean visualwebsiteoptimizer.com). Is it ok for you?
Comment 6 Andrey Meshkov 2015-12-02 12:34:50 PST
(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/
Comment 7 Build Bot 2015-12-02 12:37:07 PST
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
Comment 8 Build Bot 2015-12-02 12:37:10 PST
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
Comment 9 Build Bot 2015-12-02 12:41:13 PST
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
Comment 10 Build Bot 2015-12-02 12:41:15 PST
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
Comment 11 Build Bot 2015-12-02 12:47:16 PST
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
Comment 12 Build Bot 2015-12-02 12:47:19 PST
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
Comment 13 WebKit Commit Bot 2015-12-02 13:03:25 PST
The commit-queue encountered the following flaky tests while processing attachment 266459 [details]:

The commit-queue is continuing to process your patch.
Comment 14 WebKit Commit Bot 2015-12-02 13:03:58 PST
The commit-queue encountered the following flaky tests while processing attachment 266459 [details]:

transitions/default-timing-function.html bug 138901 (author: simon.fraser@apple.com)
The commit-queue is continuing to process your patch.
Comment 15 Andrey Meshkov 2015-12-02 13:17:10 PST
(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.
Comment 16 Alex Christensen 2015-12-02 14:47:19 PST
Created attachment 266472 [details]
Patch
Comment 17 Alex Christensen 2015-12-02 15:08:34 PST
Created attachment 266479 [details]
Patch
Comment 18 WebKit Commit Bot 2015-12-02 15:56:05 PST
Comment on attachment 266479 [details]
Patch

Clearing flags on attachment: 266479

Committed r192983: <http://trac.webkit.org/changeset/192983>
Comment 19 WebKit Commit Bot 2015-12-02 15:56:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Darin Adler 2015-12-02 17:03:05 PST
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?
Comment 21 Alex Christensen 2015-12-02 17:36:36 PST
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.
Comment 22 Alex Christensen 2015-12-02 18:32:36 PST
I get all the memory back in https://bugs.webkit.org/show_bug.cgi?id=151786
Comment 23 Andrey Meshkov 2015-12-02 23:46:18 PST
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)?
Comment 24 Alex Christensen 2015-12-03 10:18:41 PST
(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.
Comment 25 Andrey Meshkov 2015-12-03 12:02:35 PST
(In reply to comment #24)
> That'll need to be a separate bug.

Done:
https://bugs.webkit.org/show_bug.cgi?id=151815
Comment 26 Radar WebKit Bug Importer 2016-01-21 10:41:54 PST
<rdar://problem/24281137>