Remove application ID, OS and version checks from remote settings add-on (but not plugin/gfx!) blocklist client implementation
Categories
(Toolkit :: Blocklist Implementation, task, P3)
Tracking
()
People
(Reporter: Gijs, Assigned: robwu)
References
Details
So selecting the longest jank period in https://perfht.ml/2xdSBYf, filtering for JS, and inverting the call stacks, the second-most time is spent filtering entries.
Seems this is just going to be run ~1000 times and do more or less nothing - on my mac, I get 909 entries when the list has 914.
From rudimentary checks, it seems the currently published list on the remote settings endpoint ( https://firefox.settings.services.mozilla.com/v1/buckets/blocklists/collections/addons/records ) where targetApplication
is present and not empty has 18 entries. 15 are for Firefox and have maxVersion: "*"
so they apply to current release. 2 are for Thunderbird, and so will be no-ops when applied in Firefox. 1 is for Firefox specific to version 9 (block ID i46
), from bug 712369, which is no longer available on AMO and so the block is also a no-op.
There are also 2 entries with an os
filter, both for (different guids of) the "Real Player Record" extension, which also seem to no longer exist on AMO. Blocking this everywhere seems acceptable, too.
This explains the -5 difference between the total list and the list I see locally.
From this, I believe it would be possible to just remove the filtering for the add-on blocklist outright. This looks, from https://perfht.ml/3cyxpfD (checking time spent in _filterItem
and targetAppFilter
), like it'd save us somewhere between a quarter and half the time spent in JS land for the update. It's a bit hard to tell what the costs here are, exactly, given how it's spread out and because of the async/await usage which loses stack trace information, and it's windows so the profile is lossy, so YMMV.
Anyway, given that it's effectively code removal + updating some tests, this seems worth doing. AFAICT, this wouldn't require any action on the AMO side given the current state of the list and the fact that we don't (per comments in bug 1508332) add new entries with target application/version or OS filters. Jorge, can you confirm? (To be clear: this would continue to support blocking specific add-on versions, just not blocking an add-on only on Windows / Firefox 26-42, etc.).
This wouldn't address the cost of iterating over all the entries to figure out if each of the installed add-ons (including e.g. builtin search extensions) are blocked. If we're comfortable doing so, it may be worth adding a carve-out that doesn't check the blocklist for builtin add-ons, which isn't a real solution but helps in the empty profile case. Needinfo :TheOne for that part...
Comment 1•5 years ago
|
||
Yes, I can confirm the only filters we care about now and moving forward are add-on version filters. I also agree that the old blocks can have their filters removed.
Assignee | ||
Updated•5 years ago
|
Reporter | ||
Comment 2•5 years ago
|
||
Are we likely to enable the new backend in release for 76 already, Rob? If so, it might not be worth me spending time on this...
Assignee | ||
Comment 3•5 years ago
|
||
We are targeting 78 (ESR), but I'm aiming at 77 so we have another release cycle for testing and improvements if needed.
So in about one month, this bug should be fixed (I've marked it as a dependency of the blocklist-v3 bug so I can go through all bugs and close them if possible).
Updated•5 years ago
|
Reporter | ||
Comment 4•5 years ago
|
||
it may be worth adding a carve-out that doesn't check the blocklist for builtin add-ons, which isn't a real solution but helps in the empty profile case. Needinfo :TheOne for that part...
Still curious about this, Andreas. Would we be comfortable not checking builtin add-ons against the blocklist?
Comment 5•5 years ago
|
||
Sorry, I missed that.
How much would that gain us for empty profiles?
Is there a scenario where our only path of remediation for a bad built-in add-on would be a block of that? Or do we always have other means to remediate? If so, I believe we could skip that check.
Reporter | ||
Comment 6•5 years ago
|
||
(In reply to Andreas Wagner [:TheOne] [use NI] from comment #5)
Sorry, I missed that.
How much would that gain us for empty profiles?
Depends on the machine, but likely up to a few hundred ms.
Is there a scenario where our only path of remediation for a bad built-in add-on would be a block of that? Or do we always have other means to remediate? If so, I believe we could skip that check.
Well, if app updates are disabled and blocklist updates are not and they end up with a bad build, I guess that'd leave us without a way to reach those users - but that seems unlikely. I think if a built-in add-on was "bad" we'd probably fix with an app update anyway, plus they're mostly search engines so the risk is low.
Anyway, the performance of these lookups is going to be fixed in 78 so it might not be worth messing with. I'm still trying to make up my mind about whether to write a short-ish patch to fix the filtering.
Reporter | ||
Updated•5 years ago
|
Comment 7•5 years ago
|
||
I agree it's probably low-risk for search engines. Are there any extension-type add-ons? Maybe we can loop in stakeholders for them?
Reporter | ||
Comment 8•5 years ago
|
||
I haven't had time to work on this.
Assignee | ||
Comment 9•5 years ago
|
||
I'll take it. When working on blocklist v3 I found some remains and unused code/test data that I would like to remove at some point, and this cleanup also fits in that plan.
Updated•2 years ago
|
Description
•