Issue 4330 - Avoid suggesting huge URL filters

Issue 4330 - Avoid suggesting huge URL filters

[kzar, 2016-10-12 19:48:22]

Patch Set 1

[Sebastian Noack, 2016-10-13 07:41:13]

1. I've seen HTTP URLs (usually with a large querystring) with more than 500
characters in the wild. 1000 seems to be a more appropriate limit.
2. We might want to ignore/truncate the URLs in the content script, before
messaging them to the background page, since messaging large data might already
cause performance issues.

[kzar, 2016-10-13 11:05:38]

Patch Set 2 : Upped character limit to 1000

I considered filtering these URLs before sending them to the background too, but
found it worked just fine to filter them afterwards. (On my laptop with a ~45k
image data URI.) Before I added the fix Adblock Plus would hang / crash reliably
when doing that.

I also had a look at the code and noticed the element collapsing logic does the
same thing, sending potentially huge URLs to the background.

[Sebastian Noack, 2016-10-14 09:54:47]

On 2016/10/13 11:05:38, kzar wrote:
> Patch Set 2 : Upped character limit to 1000
> 
> I considered filtering these URLs before sending them to the background too,
but
> found it worked just fine to filter them afterwards. (On my laptop with a ~45k
> image data URI.) Before I added the fix Adblock Plus would hang / crash
reliably
> when doing that.
> 
> I also had a look at the code and noticed the element collapsing logic does
the
> same thing, sending potentially huge URLs to the background.

I remember that Safari at least, gets very slow (and usually crashes), when
passing large data (like a 1MB image encoded as data: URL) to the background
page. But I guess Safari isn't important anymore. So assuming there are no
implications for Chrome, LGTM.

[kzar, 2016-10-14 17:00:20]

Patch Set 3 : Check src length in content script

I made a test page with a > 3 meg data URI image
https://static.kzar.co.uk/large-data-uri.html . You were right,  attempting to
block that reliably crashed Adblock Plus for Chrome. Now that we're checking in
the content script it works OK.

[Sebastian Noack, 2016-10-14 17:32:32]

https://codereview.adblockplus.org/29356630/diff/29357565/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29356630/diff/29357565/composer.postload.j...
composer.postload.js:51: src: src && src.length < 1000 && src,
This expression is quite weird. Besides that it is hard to read, note that in
case src has more than 1000 characters you pass false which is not great. How
about this:

  src && src.length <= 1000 ? src : null

[kzar, 2016-10-14 17:48:59]

Patch Set 4 : Changed crazy logic

https://codereview.adblockplus.org/29356630/diff/29357565/composer.postload.js
File composer.postload.js (right):

https://codereview.adblockplus.org/29356630/diff/29357565/composer.postload.j...
composer.postload.js:51: src: src && src.length < 1000 && src,
On 2016/10/14 17:32:32, Sebastian Noack wrote:
> This expression is quite weird. Besides that it is hard to read, note that in
> case src has more than 1000 characters you pass false which is not great. How
> about this:
> 
>   src && src.length <= 1000 ? src : null

Yea you're right, it was pretty weird to be fair :p
Done.

[Sebastian Noack, 2016-10-14 18:08:24]

LGTM