1517014 - AntiTrackingCommon::IsOnContentBlockingAllowList is slow

Status: RESOLVED FIXED

(Firefox :: Protections UI, enhancement, P1)

Description

[(no longer active)]

See this profile, for example:

https://perfht.ml/2R05y0J

There is string parsing, URL parsing, and a hashtable lookup going on there.  Note that we call this function from nsContentUtils::LoadImage as well as nsContentUtils::IsImageInCache.  So much sadness...

Comment 1

[(no longer active)]

Attachment: Bug 1517014 - Part 1: Add nsIPermissionManager.testPermissionOriginNoSuffix(), an API for testing permissions using an origin string without the overhead of parsing it into a URI;

For consumers which have an origin string, currently they need to parse it into
a URI before they can call testPermission().  Internally, in the common case this
nsIURI* argument will be immediately converted back into the same origin string
in PermissionKey::CreateFromURI().  This means that the cost of parsing the
original origin string will effectively end up being wasted in the common case.

This patch adds an API that allows the consumer to test a permission using the
origin string directly, and only parse it into an nsIURI when necessary, thereby
avoiding this overhead.

Comment 2

[(no longer active)]

Attachment: Bug 1517014 - Part 2: Avoid the overhead of parsing our origin string into a URI in AntiTrackingCommon::IsOnContentBlockingAllowList();

Depends on D15543

Comment 3

[(no longer active)]

Attachment: Bug 1517014 - Part 3: Avoid a hashtable lookup in AntiTrackingCommon::IsOnContentBlockingAllowList();

Depends on D15544

Comment 4

[(no longer active)]

Attachment: Bug 1517014 - Part 4: Remove a useless QueryInterface from AntiTrackingCommon::IsOnContentBlockingAllowList();

The only implementation of nsIURI which has a GetHostPort() method which can
succeed is nsStandardURL, which implements nsIURL.  Other implementations
either do not implement nsIURL (so in the old version, they would bail out
early) or they do, but their GetHostPort() method returns an error code
unconditionally (so in the old version, they would bail out later).  Hence,
this patch doesn't change the semantics of the code.

Depends on D15545

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d316c3634857
Part 1: Add nsIPermissionManager.testPermissionOriginNoSuffix(), an API for testing permissions using an origin string without the overhead of parsing it into a URI; r=nika
https://hg.mozilla.org/integration/autoland/rev/da74b409b3da
Part 2: Avoid the overhead of parsing our origin string into a URI in AntiTrackingCommon::IsOnContentBlockingAllowList(); r=baku
https://hg.mozilla.org/integration/autoland/rev/28a3d1a99256
Part 3: Avoid a hashtable lookup in AntiTrackingCommon::IsOnContentBlockingAllowList(); r=baku
https://hg.mozilla.org/integration/autoland/rev/14d8f03b496c
Part 4: Remove a useless QueryInterface from AntiTrackingCommon::IsOnContentBlockingAllowList(); r=baku

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/d316c3634857
https://hg.mozilla.org/mozilla-central/rev/da74b409b3da
https://hg.mozilla.org/mozilla-central/rev/28a3d1a99256
https://hg.mozilla.org/mozilla-central/rev/14d8f03b496c