1337672 - Reduce the priority of network requests for <link rel="prefetch">

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P3)

Description

[Chris Peterson [:cpeterson]]

These patches will reduce the priority of network requests for <link rel="prefetch"> and Flash plugin content.

Honza, if you think this is a good idea, should I use the nsISupportsPriority interface (as used for Tracking Protection requests in bug 1141814) and/or nsIClassOfService's Throttleable flag (added in bug 1312745)? Would nsIClassOfService's Background or Speculative flags also be appropriate?

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: Bug 1337672 - Part 1: Reduce the priority of prefetch network requests.

Reduce the priority of <link rel="prefetch"> network requests from PRIORITY_NORMAL to PRIORITY_LOWEST, the same as favicons. These prefetch requests are already deferred until the browser is idle using nsIWebProgressListener, but we can also adjust the network request priority to reflect their lower importance.

https://developer.mozilla.org/docs/Web/HTTP/Link_prefetching_FAQ#How_is_browser_idle_time_determined.3F

Review commit: https://reviewboard.mozilla.org/r/110618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110618/

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Bug 1337672 - Part 2: Reduce the priority of Flash network requests.

Reduce the priority of network requests to load Flash <object> elements from PRIORITY_NORMAL to PRIORITY_LOW + 2, below images (PRIORITY_LOW and PRIORITY_LOW + 1) but above favicons (PRIORITY_LOWEST). Reduce the priority of NPAPI network requests issued by Flash content from PRIORITY_NORMAL to PRIORITY_LOW + 3, below images and <object> elements but above favicons. We don't want to outgoing requests from early Flash content to delay the initial loading and rendering of other Flash <object> elements on the page.

Review commit: https://reviewboard.mozilla.org/r/110620/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110620/

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Chris: you want Jan Bambas [:mayhemer] to do the review I guess.

Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8834776 [details]
Bug 1337672 - Part 1: Reduce the priority of prefetch network requests.

https://reviewboard.mozilla.org/r/110618/#review111898

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8834777 [details]
Bug 1337672 - Part 2: Reduce the priority of Flash network requests.

https://reviewboard.mozilla.org/r/110620/#review111900

Comment 6

[Chris Peterson [:cpeterson]]

(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> @Chris: you want Jan Bambas [:mayhemer] to do the review I guess.

oops! Yes, thank you.

Comment 7

[Chris Peterson [:cpeterson]]

These patches will reduce the priority of network requests for <link rel="prefetch"> and Flash plugin content.

Honza, if you think this is a good idea, should I use the nsISupportsPriority interface (as used for Tracking Protection requests in bug 1141814) and/or nsIClassOfService's Throttleable flag (added in bug 1312745)? Would nsIClassOfService's Background or Speculative flags also be appropriate?

In my testing of my news sites linked from news.google.com, I only saw the following resources loaded using <link rel="prefetch">. None of them seem urgent.

http://darkroom.baltimoresun.com/2017/01/preparing-for-the-trump-inauguration/
http://tpc.googlesyndication.com/safeframe/1-0-5/html/container.html
https://a.disquscdn.com/next/embed/lounge.bundle.bc7675b3e7e1b4df8aad51f94abd7265.js
https://assets.guim.co.uk/javascripts/e7ed4d0fceb8658f3ba7c1eb518ed2e1/app.js
https://disqus.com/next/config.js
https://ims-na1.adobelogin.com/favicon.ico?cache_bust=ddca24049ad2b8
https://securepubads.g.doubleclick.net/static/glade.js

In contrast, I see Flash ad loaders on these news sites issuing nonstop requests to load new Flash ad images and XML.

Comment 8

[Chris Peterson [:cpeterson]]

Comment on attachment 8834776 [details]
Bug 1337672 - Part 1: Reduce the priority of prefetch network requests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110618/diff/1-2/

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 8834777 [details]
Bug 1337672 - Part 2: Reduce the priority of Flash network requests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110620/diff/1-2/

Comment 10

[Benjamin Smedberg]

I don't know enough about the effects of network prioritization to have much opinion about this. For example, are requests for the current tab always prioritized higher than background tabs? I wouldn't want requests from the current page's Flash (perhaps including video) to be prioritized lower than any load from other tabs.

From an engineering/code-readability perspective, the values `nsISupportsPriority::PRIORITY_LOW + 3` are terrible and should really be new values on nsISupportsPriority, no?

But overall I'm going to defer review to mayhemer, since I lack enough context.

Comment 11

[Benjamin Smedberg]

Comment on attachment 8834777 [details]
Bug 1337672 - Part 2: Reduce the priority of Flash network requests.

https://reviewboard.mozilla.org/r/110620/#review112052

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Chris Peterson [:cpeterson] from comment #0)
> These patches will reduce the priority of network requests for <link
> rel="prefetch"> and Flash plugin content.
> 
> Honza, if you think this is a good idea, should I use the
> nsISupportsPriority interface (as used for Tracking Protection requests in
> bug 1141814) 

Yes

> and/or nsIClassOfService's Throttleable flag (added in bug
> 1312745)? 

I more tend to say no, but this has no affect on prioritization.  It only says to simply throttle download speed (by suspending the channel periodically) for those requests.  At the moment it's not clear how well it's going to work, if at all, so I don't suggest using it.

> Would nsIClassOfService's Background or Speculative flags also be
> appropriate?

You have to ask Patrick about these two.

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #10)
> I don't know enough about the effects of network prioritization to have much
> opinion about this. For example, are requests for the current tab always
> prioritized higher than background tabs? 

No, but we are working on it!

> I wouldn't want requests from the
> current page's Flash (perhaps including video) to be prioritized lower than
> any load from other tabs.

Good argument.

> 
> From an engineering/code-readability perspective, the values
> `nsISupportsPriority::PRIORITY_LOW + 3` are terrible and should really be
> new values on nsISupportsPriority, no?

We have LOWEST too, I think?

> 
> But overall I'm going to defer review to mayhemer, since I lack enough
> context.

Comment 14

[Chris Peterson [:cpeterson]]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> (In reply to Benjamin Smedberg [:bsmedberg] from comment #10)
> > I don't know enough about the effects of network prioritization to have much
> > opinion about this. For example, are requests for the current tab always
> > prioritized higher than background tabs? 
> 
> No, but we are working on it!

The code that will favor the foreground tab's requests might need a special case to identify which Flash plugin requests come from Flash content on the foreground tab. Something to keep in mind.

> > From an engineering/code-readability perspective, the values
> > `nsISupportsPriority::PRIORITY_LOW + 3` are terrible and should really be
> > new values on nsISupportsPriority, no?
> 
> We have LOWEST too, I think?

Yes, though there are nine priority levels available between PRIORITY_LOW (10) and PRIORITY_LOWEST (20).

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 8834776 [details]
Bug 1337672 - Part 1: Reduce the priority of prefetch network requests.

https://reviewboard.mozilla.org/r/110618/#review112494

::: uriloader/prefetch/nsPrefetchService.cpp:164
(Diff revision 2)
>  
> +    // Reduce the priority of prefetch network requests.
> +    nsCOMPtr<nsISupportsPriority> priorityChannel = do_QueryInterface(mChannel);
> +    if (priorityChannel) {
> +      priorityChannel->AdjustPriority(nsISupportsPriority::PRIORITY_LOWEST);
> +    }

so this class is actually used... :)

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8834777 [details]
Bug 1337672 - Part 2: Reduce the priority of Flash network requests.

https://reviewboard.mozilla.org/r/110620/#review112496

So, with this patch... when I go to a porn site and want to see the video I found, you will deliberately delay it for me among other advertising and what ever stuff I'm definitely not interested in?  The video (on any video providing site) with autoplay is the main content of interest.  This patch IMO goes exactly against delivering that with added priority.

I will r- this for now.  I have to think first about how to tail this in the overall CDP picture.  I think for some content we want to lower the priority (ads etc) but when the content is the one of most interest, we should add priority.

Question is how to recognize this soon enough.  Maybe the position and size of the player element given to some simple heuristic is enough?

Or... is it worth to add a priority to Flash et al in times of HTLM5 players?

::: dom/base/nsObjectLoadingContent.cpp:2624
(Diff revision 2)
>    }
>  
> +  // Reduce the priority of network requests to load Flash <object> elements.
> +  nsCOMPtr<nsISupportsPriority> priorityChannel = do_QueryInterface(chan);
> +  if (priorityChannel) {
> +    priorityChannel->AdjustPriority(nsISupportsPriority::PRIORITY_LOW + 2);

the LOW + 2 needs some comment to rationalize it.

::: dom/plugins/base/nsPluginHost.cpp:3272
(Diff revision 2)
>    }
> +
> +  // Reduce the priority of network requests issued by Flash content.
> +  nsCOMPtr<nsISupportsPriority> priorityChannel = do_QueryInterface(channel);
> +  if (priorityChannel) {
> +    priorityChannel->AdjustPriority(nsISupportsPriority::PRIORITY_LOW + 3);

as well here (probably in relation to the above)

Comment 17

[Chris Peterson [:cpeterson]]

(In reply to Honza Bambas (:mayhemer) from comment #16)
> I will r- this for now.  I have to think first about how to tail this in the
> overall CDP picture.  I think for some content we want to lower the priority
> (ads etc) but when the content is the one of most interest, we should add
> priority.
> 
> Question is how to recognize this soon enough.  Maybe the position and size
> of the player element given to some simple heuristic is enough?

I see what you mean. I will land the prefetch patch you r+'d and then close this bug for now.

We can make a pretty good guess whether a Flash element is a video based on its size and aspect ratio. Chrome and Edge use similar heuristics to distinguish important from "non-essential" Flash (which they then make click-to-play). Some details about their heuristics:

https://docs.google.com/document/d/1r4xFSsR4gtjBf1gOP4zHGWIFBV7WWZMgCiAHeepoHVw/

That said, we (and Chrome and Edge) plan to make most Flash content click-to-play over the next 6-12 months, so any Flash content that the user *does* load is likely to be something they want to see. So we might not need to bother adding any special logic to distinguish important from non-essential Flash. Flash video is still preferred over HTML5 video by some sites that use Adobe's DRM or analytics, such as HBO GO, Hulu, and CNN.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8da87f54011
Part 1: Reduce the priority of prefetch network requests. r=mayhemer

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c8da87f54011