1915186 - UrlClassifierCommon::AnnotateChannel lowers priority of channels requested at high priority

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Andrew Creskey [:acreskey]]

As discussed here and onwards, the lowering of network priority done in UrlClassifierCommon::AnnotateChannel, here resets priorities that were previously set with intention (e.g. fetchPriority)

I think this is because of the nightly-only behaviour of lowering priority based on privacy.trackingprotection.lower_network_priority
https://searchfox.org/mozilla-central/rev/490a1df802d8872f996f8ef4093d54e3c854c8f9/modules/libpref/init/StaticPrefList.yaml#14498-14505

Comment 1

[Andrew Creskey [:acreskey]]

privacy.trackingprotection.lower_network_priority has been nightly only for at least 5 years.

It might make sense to disable this on nightly as well and see if it moves any performance metrics.

This lowering of priority for third party resources might be beneficial overall (outside of colliding with priority set elsewhere in the runtime, like fetchPriority).
Or it might not be.

Valentin - thoughts?

Comment 2

[Andrew Creskey [:acreskey]]

From browsing some top wordpress sites, I collected a list of resources which had their priorities lowered due to privacy.trackingprotection.lower_network_priority being enabled:

https://storage.googleapis.com/pr-newsroom-wp/1/2024/08/GT0sh5UbIAAFDZw.jpeg
https://storage.googleapis.com/pr-newsroom-wp/1/2022/05/community.svg
https://fonts.gstatic.com/s/robotocondensed/v27/ieVl2ZhZI2eCN5jzbjEETS9weq8-19K7DQk6YvM.woff2
https://fonts.googleapis.com/css?family=Cabin%3A400%2C600%2C700%2C400italic%2C500%7CDroid+Serif%3A400%2C700%2C400italic%2C700italic%7CLibre+Baskerville%3A400%2C400italic&subset=latin%2Clatin-ext 
https://cdnjs.cloudflare.com/ajax/libs/slick-carousel/1.7.1/slick.min.css 
https://www.youtube.com/iframe_api
https://www.youtube.com/s/player/bcd1f224/player_ias.vflset/en_US/embed.js 
https://www.youtube.com/s/player/bcd1f224/www-embed-player.vflset/www-embed-player.js 
https://cdnjs.cloudflare.com/ajax/libs/slick-carousel/1.7.1/fonts/slick.woff
https://www.youtube.com/s/player/bcd1f224/www-player.css
https://d3e54v103j8qbb.cloudfront.net/js/jquery-3.5.1.min.dc5e7f18c8.js?site=65f36a007b9322599125cdb7 
https://i3.wp.com/rafaelnadal.com/wp-content/uploads/2017/04/rafa_twit.jpg

Comment 3

[Valentin Gosu [:valentin] (he/him) {{ PTO until Nov 5 }}]

Thank you for tracking this down.
I think there are some things here we should definitely not be lowering the priority for, CSS and fonts in particular.
Let's disable it for now.

PS. It seems like that pref was supposed to be lowering the priority of tracking resources, not all third party resources. Which means our behaviour is broken.

Comment 4

[Andrew Creskey [:acreskey]]

Thanks Valentin, patch incoming.

The impact of the pref is lowered priority on many third party resources, but not all.

I'll collect more details and log that as a new bug against our tracker detection.

Comment 5

[Andrew Creskey [:acreskey]]

Attachment: Bug 1915186 - UrlClassifierCommon::AnnotateChannel lowers priority of channels requested at high priority r=#necko-reviewers

privacy.trackingprotection.lower_network_priority has been lowering the priority for identified third party tracker resources (nightly-only) for > 5 years.

We recently discovered that it is incorrectly lowering the priority of resources such as fonts from fonts.gstatic.com and other key web resources.

So disabling the pref to better assess the performance impact of this feature as it does not appear to be working as intended.

Comment 6

[Andrew Creskey [:acreskey]]

This pref is also used in fetchDriver and XMLHttpRequest and so their nightly-only behaviour will be affected.

Comment 7

[Andrew Creskey [:acreskey]]

Bug 1915490 deals with isThirdPartyTrackingResource tagging key resources (images, fonts, css) as trackers

Comment 8

[Tim Huang[:timhuang] [PTO 10/19 ~ 11/3]]

Another feature called tailing lowers the network priority for tracking resources. We could consider disabling it as well. It doesn't actually bring any privacy benefits.

Valentin, what do you think?

Comment 9

[Andrew Creskey [:acreskey]]

Personally, I think that both of these features, lower_network_priority and tailing of tracking resources seem to have a lot of potential performance benefit.
Particularly if we could exclude the content resources from them (e.g. fonts.gstatic.com), etc.
But it would be useful to have some understanding of their current performance impact, if any.

Comment 10

[Andrew Creskey [:acreskey]]

Attempting to land the disablement of this nightly-only feature to see if it moves any performance metrics.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16ee50b5707c
UrlClassifierCommon::AnnotateChannel lowers priority of channels requested at high priority r=necko-reviewers,jesup

Comment 12

[Andrew Creskey [:acreskey]]

I've disabled lower_network_priority in this try push -- it looks like the change will trigger a perf alert. Some improvements, but mostly regressions.

We can also see if this moves the numbers on the Necko Performance Pulse dashboards.

Comment 13

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/16ee50b5707c

Comment 14

[Valentin Gosu [:valentin] (he/him) {{ PTO until Nov 5 }}]

(In reply to Tim Huang[:timhuang] from comment #8)

Another feature called tailing lowers the network priority for tracking resources. We could consider disabling it as well. It doesn't actually bring any privacy benefits.

Valentin, what do you think?

Tailing has some perf benefits, and has shipped to release. I don't think we should pref it off, but the default values are probably suboptimal.
I think we should discuss the matter more broadly in bug 1915866.