Proposal: Remove URLRequestThrottlerManager

[Xunjie]

Hi everyone,

net::URLRequestThrottlerManager is being used to throttle extension-issued requests when the same URL is requested too aggressively. It's currently on the network team to move URLRequestThrottlerManager out of net/. Since it is only used for extensions, we are thinking of turning it into a content::ResourceThrottle and move it into extensions/browser/. However, ideally if there isn't much usage, we could get rid of URLRequestThrottlerManager completely. But I am not sure if the usage is significant from the histogram data ("Throttling.RequestThrottled").

What do people think about this proposal of getting rid of URLRequestThrottlerManager completely? Tracking bug is at crbug.com/484241.

[Benjamin Kalman]

Do you know what the history of the throttle is? It does sound odd that it's only used by extensions.

--
You received this message because you are subscribed to the Google Groups "extensions-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to extensions-de...@chromium.org.
For more options, visit https://groups.google.com/a/chromium.org/d/optout.

[Helen Li]

I think it was once enabled for all requests, but the throttling mechanism created problems so it was only enabled for extensions. The old tracking bug is at https://code.google.com/p/chromium/issues/detail?id=83775.

[Benjamin Kalman]

I don't see the point in throttling extensions but not anybody else... but I'm not an expert on networking. Can you thinking of anything? From my perspective, I have no objections to removing it. It may be wise to track network traffic to attribute something to extensions I suppose - is there something equivalent for webpages?

[Matt Menke]

Also worth noting that this only throttles repeated requests to the exact same URL (Ignoring query and reference), and doesn't distinguish which extension made the request, so it's only really useful in throttling out of control extensions requesting the same URL.  Unless this was something specifically wanted for the extensions team for some reason, I think we're safe removing it.

[Benjamin Kalman]

If it was it was before my time, though we could always try to trawl back through commit history and find the bug.

[Helen Li]

I believe this throttling mechanism was intended to be a feature. I don't think there was a specific extension related bug. This feature also hampered extension development, see crbug.com/66062, and later a command line flag is introduced to disable this (http://www.chromium.org/throttling). The code that enables throttling for extensions is in ChromeNetworkDelegate::OnCanThrottleRequest. I think we should probably remove this, unless the extensions team would like to keep it. Any thought?

[Matt Menke]

I don't seen any context for why it was switched to extensions only...  https://codereview.chromium.org/9819001 landed, with no discussion on the bug, and no reasoning on the codereview.  Unless someone from the extensions team has any objections (Or more context), think we should go ahead and remove it.

If anyone's not comfortable with the decision, we could send out an intent to remove email to chromium-dev first, make sure we aren't keeping anyone out of the loop who should be.

[Antony Sargent]

My recollection is that Joi originally implemented this throttling based on requests from both Google and 3rd party server-side folks who complained about various incidents of buggy misbehaving extensions code hammering their services. One proposal put forward was to mark all requests from extensions with some special http header so that servers could filter them in such cases as being "not human initiated", but a number of us opposed this since we were worried about servers having blanket bans of such traffic for arbitrary reasons like "we don't like extensions." It turned into a bit of an ideological debate between people with client-centric and server-centric views, and the compromise worked out was to build the denial-of-service protection into chrome itself.

From our client-centric view, it can be easy to say "What's the big deal? Client programs misbehave all the time and servers always need to be ready to protect against that", but from the server-centric (eg SRE) view, in the middle of a fire when faced with a big spike in requests to some important service, you want as many tools as possible to block the "bad" traffic while still letting through the "good" traffic through, and one experience with any misbehaving client (whether a chrome extension or anything else) will make you loudly request a way to stop that particular thing in the future. Anyhow, I'd recommend we keep this protection in place unless maintaining it is a large burden, in which case it would be worth brainstorming if there are ways to reimplement it in an easier-to-maintain form.  

[Matt Menke]

Having a throttle manager just for extensions requests that lives in net/ is a layering violation.  We have ResourceThrottles for that sort of stuff.

Also, we're planning on implementing server-controller throttling.  Having two throttle managers in net/, each using different NetworkDelegate methods really doesn't work.

[Antony Sargent]

To clarify, I'm not opposed to making changes to how we achieve the protection against hammering servers, just that I don't think we want to remove it completely. If switching to ResourceThrottles can achieve a roughly similar or even better protection, and is planned to be well-supported going forward in the net/ infrastructure, that sounds great to me. 

[Benjamin Kalman]

Antony's point is what I was thinking as well. The combination of extensions not being written by website authors, their traffic being able to masquerade as coming from a real client, and because there is no way to close them - it makes sense to try to put client side protections as a sanity check. If that's valuable then server side throttling will only help Google anyway. It sounds like we/somebody should reimplement this as a ResourceThrottle.

[Helen Li]

Alright. Looks like turning URLRequestThrottlerManager into a ResourceThrottle, and place it in extensions/browser/ is the way to go. I will continue looking into it. Thanks for the feedback, everyone!