Opened 7 months ago

Closed 5 months ago

Last modified 5 months ago

#4330 closed defect (fixed)

Block element tool slow for images with data URI

Reported by: mapx Assignee: kzar
Priority: P3 Milestone: Adblock-Plus-1.12.4-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: kzar, sebastian, greiner Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29356630/

Description (last modified by kzar)

Environment

chrome 53
ABP 1.12.1.1640

How to reproduce

  1. Browse to http://dopiaza.org/tools/datauri/examples/index.php
  2. Right click on the cat picture, select "Block Element".

Observed behaviour

The "Block element" popup window opens but is extremely slow and unresponsive.

Expected behaviour

The window should not be slow or unresponsive.

Notes

The problem is that the suggested filter is extremely long, containing the entire data URI.

What to change

Prevent very long URLs from being suggested for filters. (Very long being defined as more than 1000 characters.)

Change History (12)

comment:1 Changed 7 months ago by mapx

  • Description modified (diff)

comment:2 Changed 7 months ago by greiner

  • Cc greiner added
  • Description modified (diff)

This issue is a bit rough to trace back since Chrome's DevTools cannot cope with this load. Anyway, I was able to narrow it down to the part where we set document.getElementById("filters").value in "composer.js" so it very much appears to be caused by Chrome not being able to handle this amount of text in an input field.

So to tackle that I'd suggest improving our filter creation algorithm to create shorter filters for elements with data URIs. One such improvement could be to only match certain parts of the data URI instead of the entire string. For example instead of

img[src="data:image/png;base64,abcdefghijkl"]

we could create

img[src^="data:image/png;base64,abc][src*="efgh"][src$="jkl"]

instead.

The issue with such an approach would be that false positives become much more likely so we'd have to be careful.

Note that I also observed the extension's CPU usage going up and staying at 100% for some minutes even when after closing the dialog. That means that Adblock Plus cannot handle requests in the meantime and therefore you cannot load any webpage until the CPU usage drops down to normal levels again.

comment:3 follow-up: Changed 7 months ago by mapx

see the same issue in ubo (fixed now):
https://github.com/gorhill/uBlock/issues/1725

comment:4 in reply to: ↑ 3 Changed 7 months ago by greiner

Replying to mapx:

see the same issue in ubo (fixed now):
https://github.com/gorhill/uBlock/issues/1725

Thanks for the link. From what I see it's basically the same idea: stripping away most parts of the data URI and shrinking it down to a predefined size. The only difference seems to be that the filter I'm suggesting is not only matching the beginning but also the end and a part in the middle of the URI to, hopefully, decrease the probability for false positives since the first few pixels can be identical across different images.

comment:5 Changed 5 months ago by kzar

  • Description modified (diff)
  • Platform changed from Chrome to Unknown / Cross platform
  • Priority changed from Unknown to P3
  • Summary changed from slow "block element" on (data: uri ) elements to Block element tool slow for images with data URI

I suggest we simply don't use data URIs for these filter suggestions at all. What do you think Sebastian?

comment:6 Changed 5 months ago by sebastian

Note that we specifically fall back to generate an element hiding filter, matching the src attribute, when we cannot generate a request blocking filter instead (i.e. for data: and javascript: URLs):

// If there is a "src" attribute, specifiying a URL that we can't block,
// generate a CSS selector matching the "src" attribute
if (isValidString(details.src))
  selectors.push(escapeCSS(details.tagName) + "[src=" + quoteCSS(details.src) + "]");

However, the problem is not that it is a data: URL, but that it is a very long URL. So instead of ignoring data: URLs, we should rather ignore very long URLs here.

comment:7 Changed 5 months ago by kzar

  • Description modified (diff)
  • Owner set to kzar
  • Ready set

Sounds good to me. I've arbitrarily decided on 500 characters for the limit, but feel free to change that if you disagree. (On my laptop the 45k URL is really slow, but the 1k URL seemed OK.)

comment:8 Changed 5 months ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:9 Changed 5 months ago by kzar

  • Description modified (diff)

comment:10 Changed 5 months ago by abpbot

A commit referencing this issue has landed:
Issue 4330 - Avoid suggesting huge URL filters

comment:11 Changed 5 months ago by kzar

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Safari-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:12 Changed 5 months ago by rraceanu

  • Verified working set

Issue no longer occurs, block element pop-up window appears immediately for long URL images; Verified on ABP version 1.12.2.1670, Chrome version 41, 45, 51, 54, Opera 35, 40 and Safari 7, 9.

Last edited 5 months ago by rraceanu (previous) (diff)
Note: See TracTickets for help on using tickets.