780087 - When an app exceed allowed traffic, block its network traffic

Status: RESOLVED INVALID

(Core :: Networking, defect)

Description

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

In bug 746074, a policy manager is built for setting network policy for each app. A network usage meter is also built in bug 746073. Base on the policy and network traffic data of each app, we can block application's network usage when it exceed its allowed traffic.

Comment 1

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Patrick Wang [:kk1fff] from comment #0)
> In bug 746074, a policy manager is built for setting network policy for each
> app. A network usage meter is also built in bug 746073. Base on the policy
> and network traffic data of each app, we can block application's network
> usage when it exceed its allowed traffic.

Patrick, not only limit & block the connection. We also need to redirect traffic by different interfaces, ie, the facebook app will always go through 3g while system upgrades will only work through wifi.

So Necko SHALL do both things, select the correct network interface (if available) and limit traffic consumption per app

Comment 2

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

It seems to be a nice goal :) . However, after some discussion, this could be a complex task. As the first step, I think I will start with description of bug 746074, check policy when an app is opening a channel, and block the traffic (and/or notify the app) if the app exceed the quota on current default interface.

Comment 3

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP

This is a very early WIP. Just to make sure if I am on the right path.

I add a function NS_DidAppRanOutOfQuota(nsIChannel aChannel) in nsNetUtil.h. This function retrieves the appid from the channel, check if the app exceeds its traffic limit on current default interface. If the app has exceeded it traffic limit on current interface, this function returns true.

Every time when nsIChannel::AsyncOpen is called, it calls NS_DidAppRanOutOfQuota to check if the channel should be opened. If the channel should not open (say, the app has exceeded its limit), AsyncOpen exits and returns an error.

I modified AsyncOpen of nsHttpChannel in this WIP, it can block http. If it looks sensible, I will also add the logic to AsyncOpen() of another protocols.

Hoping for some feedback :)
Thank you.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 649573 [details] [diff] [review]
WIP

Review of attachment 649573 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/public/nsNetUtil.h
@@ +2149,5 @@
> +        rv = ni->GetType(&type);
> +        __android_log_print(ANDROID_LOG_INFO, "nsNetUtil",
> +                            "Network service: %p, network interface: %p, "
> +                            "type = %d, GetType = %d",
> +                            ns.get(), ni.get(), type, rv);

__android_log_print is definitely not cross-platform ;)  If you need, you may use LOG().

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4351,5 @@
>  {
>      LOG(("nsHttpChannel::AsyncOpen [this=%p]\n", this));
>  
> +    if (NS_DidAppRanOutOfQuota(this)) {
> +        return NS_ERROR_NOT_CONNECTED;

Definitely not the right error and definitely not the proper way from the system point of view.

I would personally more tend to have a CSP policy for this that would check the quota, if doable.  That is a generic code that may block any kind of traffic.

Other option is to register http-on-modify-request global observer that can inspect the request and cancel it when out of quota.  But that is just for http.

Comment 5

[Jason Duell]

Comment on attachment 649573 [details] [diff] [review]
WIP

Review of attachment 649573 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/public/nsNetUtil.h
@@ +2125,5 @@
> +// Check if the app of the channel ran out of its quota on current default
> +//  network interface.
> +//
> +inline bool
> +NS_DidAppRanOutOfQuota(nsIChannel* aChannel) {

So will this API map to a model where we want to use a different interface (if one is available) when we hit quota on the default interface?  It doesn't look like it.  But perhaps for the B2G case at least we don't anticipate this being an issue (i.e. we won't have quota on wifi, just cellular interfaces, and if we hit a quote on cellular we won't have wifi to fall back on, else we would have used it in the first place).

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4351,5 @@
>  {
>      LOG(("nsHttpChannel::AsyncOpen [this=%p]\n", this));
>  
> +    if (NS_DidAppRanOutOfQuota(this)) {
> +        return NS_ERROR_NOT_CONNECTED;

Seems like this deserves its own, new error code, so we can show an appropriate boilerplate message to the user. Here are some notes I took a while ago on adding new error pages:

"nsDocShell::EndPageLoad looks at the status of the channel, and if it's an error status it knows about it loads the appropriate error page.  So you want to add a new error code, return it from your OnDataAvailable or OnStopRequest to cancel the channel, then add code to EndPageLoad, similarly to the way other error codes are handled there."

Things will work differently when you're failing synchronously (i.e. asyncOpen fails right away) as in your patch. (you could always change this to be an asyncError by calling AsyncAbort instead of returning an error).  Also, I'm not sure that a failure page is the right UI (probably not a bad first start: but check with a B2G UI person).

Comment 6

[Jason Duell]

And Honza's idea for a CSP or observer are both good.  They can Cancel() a load if quote is hit with the new error code, and the error page logic I mentioned will still work.

Comment 7

[Honza Bambas (:mayhemer)]

Having an own error is a good idea.  Since B2G can work with it directly and present user as well as the web app with info about being over the quota in a specific way.  That can be definitely useful.

And yes, I forgot to mention that failing synchronously may not be the best way to 
prevent the load.  Right.

Comment 8

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Thank you for the feedbacks. They are very helpful :) . CSP seems to be a good place for us to implement the logic. Another question is, if we implement a new policy, do we need a new nsIContentPolicy::REJECT_* code when the request is rejected because its app reached the limit, or any of the reject codes is already suitable for this case?

Thank you.

Comment 9

[Jason Duell]

Patrick,

Are you planning to do the CSP check on the child or the parent?    I just talked to cjones and he's under the impression that CSP checks for apps must be done on the parent.

In that case you need to have the failure mode be async on the child (i.e. return NS_OK for asyncOpen but then Cancel()ed channel on parent sends back error code in OnStopRequest.  That should just happen correctly if you fail the channel in the CSP on the parent.

I don't know if this makes the network policy stuff easier?  I'll comment in that bug too.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

CSP checks definitely does not need to happen in the parent, though it's fine if they do happen there. The checks in this bug would ideally happen in the parent, but it's not a big deal if they don't in the initial release.

Comment 11

[Jason Duell]

Turns out 1) CSPs can be done on child, but 2) CSPs may not be a good place for us to add this monitoring (sicking at least thinks its "a scary hack given that it's not CSP code").

I've opened a topic here about possibly adding a new nsIObserverService notification for this instead:

  https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.tech.network/5lsJ90U5VEQ[1-25]

Comment 12

[Jason Duell]

per discussion in dev-tech-network, we'll be hooking http-on-modify-request instead (and adding an FTP equivalent).

Comment 13

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Observe http-on-modify-request and block traffic from specified app.

Observe http-on-modify-event. When the event is fired, retrieve appid from the subject and get the interface.
Getting quota of the app and getting the traffic stat of the app haven't been implemented in this patch.

Comment 14

[Jason Duell]

Comment on attachment 651281 [details] [diff] [review]
Observe http-on-modify-request and block traffic from specified app.

Review of attachment 651281 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like a good start.

Are you planning on having this exist in the parent, child, or both?   If you put it in the parent, you'll get all channels in the system.   If you run it on a child, you'll get just the channels in that child process.   If you do both, you'll get double-notified for child channels (e10s necko HTTP channels fire these notification both once in the child, for the HttpChannelChild, and once in the parent, for the "real" channel that does the work (nsHttpChannel).  I'm guessing you just to only listen in the parent.

::: dom/network/src/nsTrafficBlocker.cpp
@@ +45,5 @@
> +NS_IMETHODIMP
> +nsTrafficBlocker::Observe(nsISupports *aSubject,
> +                        const char *aTopic,
> +                        const PRUnichar *aData) {
> +  LOG("Observed http-on-modify-request");

Never hurts to actually strcmp(aTopic, 'http-on-modify-request'), to make sure we catch it if somehow we register for some other events (or we somehow get the wrong events posted to us).

@@ +50,5 @@
> +  nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(aSubject));
> +
> +  if (!httpChannel)
> +    // Fail to query interface from subject.
> +    return NS_ERROR_OUT_OF_MEMORY;

I'd use NS_ENSURE_TRUE(httpChannel, NS_ERROR_UNEXPECTED) here, but the actual error you use is mostly ornamental--it gets ignored by the observerService.  Maybe throw in moz_assert(httpChannel, "didn't get HTTP channel from http-on-modify-request") too, so we actually get a real error if this happen in debug mode.

@@ +62,5 @@
> +  rv = NS_GetAppInfo(httpChannel, &appid, &isInBrowserElement);
> +
> +  if (NS_FAILED(rv)) {
> +    // Cannot get appid from the channel, our job ends here.
> +    LOG("Cannot get appid");

This isn't really an error case--there will be channels in the chrome process (and possibly some in the child) that don't have an appId.   Probably ok to return NS_OK (especially again since the code is ignored).

Your LOG messages should start with "nsTrafficBlocker::Observe:  .....") to make it easer to find the code from a log.

Comment 15

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Jason Duell (:jduell) from comment #14)
> Comment on attachment 651281 [details] [diff] [review]
> Observe http-on-modify-request and block traffic from specified app.
> 
> Review of attachment 651281 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like a good start.
> 
> Are you planning on having this exist in the parent, child, or both?   If
> you put it in the parent, you'll get all channels in the system.   If you
> run it on a child, you'll get just the channels in that child process.   If
> you do both, you'll get double-notified for child channels (e10s necko HTTP
> channels fire these notification both once in the child, for the
> HttpChannelChild, and once in the parent, for the "real" channel that does
> the work (nsHttpChannel).  I'm guessing you just to only listen in the
> parent.
> 
Yeah. I'd like nsTrafficBlocker run in parent to observe the channel
parent. Did I implement correctly?

> ::: dom/network/src/nsTrafficBlocker.cpp
> @@ +45,5 @@
> > +NS_IMETHODIMP
> > +nsTrafficBlocker::Observe(nsISupports *aSubject,
> > +                        const char *aTopic,
> > +                        const PRUnichar *aData) {
> > +  LOG("Observed http-on-modify-request");
> 
> Never hurts to actually strcmp(aTopic, 'http-on-modify-request'), to make
> sure we catch it if somehow we register for some other events (or we somehow
> get the wrong events posted to us).
> 
> @@ +50,5 @@
> > +  nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(aSubject));
> > +
> > +  if (!httpChannel)
> > +    // Fail to query interface from subject.
> > +    return NS_ERROR_OUT_OF_MEMORY;, hopefully, in days

> 
> I'd use NS_ENSURE_TRUE(httpChannel, NS_ERROR_UNEXPECTED) here, but the
> actual error you use is mostly ornamental--it gets ignored by the
> observerService.  Maybe throw in moz_assert(httpChannel, "didn't get HTTP
> channel from http-on-modify-request") too, so we actually get a real error
> if this happen in debug mode.
> 
> @@ +62,5 @@
> > +  rv = NS_GetAppInfo(httpChannel, &appid, &isInBrowserElement);
> > +
> > +  if (NS_FAILED(rv)) {
> > +    // Cannot get appid from the channel, our job ends here.
> > +    LOG("Cannot get appid");
> 
> This isn't really an error case--there will be channels in the chrome
> process (and possibly some in the child) that don't have an appId.  
> Probably ok to return NS_OK (especially again since the code is ignored).
> 
> Your LOG messages should start with "nsTrafficBlocker::Observe:  .....") to
> make it easer to find the code from a log.

Thank Jason for giving feedback :). I'll update patch with fix and code for
retrieving data form Network Stats and Network Policies.

Comment 16

[Albert [:albert]]

After having an e-mail discussion, we would like to share with everybody the approach we think that can be a good option to face this bug.

The current proposal to determine if an application's connection should be blocked is based on a request from the nsTrafficBlocker to the NetworkStats to retrieve the data usage of an application, then another request to the NetworkPolicies is needed in order to check if the allowed quota has been reached. Therefore, it means that there will be a lot of requests and messages that don't seem a good solution.

To simplify it, the idea is that NetworkStats will act as a proxy of NetworkPolicies, NetworkStats will have some kind of policies cache updated by notifications from the NetworkPolicies service. Then, as NetworkStats service is receiving updates of the data usage done by the applications, it can notify to the nsTrafficBlocker when an application has reached its quota and should be blocked when requesting a connection for 3g or wifi. So the nsTrafficBlocker can hold a list with applications to block.

When the NetworkStatsService starts it will have to check each application to evaluate which have reached the quota and, if so, send a notification to the blocker. Otherwise we can add a method to check if one application should be blocked in case that nsTrafficBlocker doesn't have info about what to do.

Do you think that this approach makes the implementation easier? Some issues or comments?

Comment 17

[Jason Duell]

> Then, as NetworkStats service is receiving updates of the data usage done 
> by the applications, it can notify to the nsTrafficBlocker when an 
> application has reached its quota 

Sounds good to me

Comment 18

[Jason Duell]

Comment on attachment 651281 [details] [diff] [review]
Observe http-on-modify-request and block traffic from specified app.

Patrick,

:sicking tells me the way he'd like to block network traffic is to tell the child process that the network is down.  He expects that apps will work better in that scenario (as if they were offline), rather than if we cancel all their channels via on-modify-request.

Comment 19

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Jason Duell (:jduell) from comment #18)
> Comment on attachment 651281 [details] [diff] [review]
> Observe http-on-modify-request and block traffic from specified app.
> 
> Patrick,
> 
> :sicking tells me the way he'd like to block network traffic is to tell the
> child process that the network is down.  He expects that apps will work
> better in that scenario (as if they were offline), rather than if we cancel
> all their channels via on-modify-request.

Sure. May I ask you to give some hint about how to notify child process that network
is down?

Comment 20

[Jason Duell]

> how to notify child process that network is down?

See bug 786419

Comment 21

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Albert from comment #16)
> After having an e-mail discussion, we would like to share with everybody the
> approach we think that can be a good option to face this bug.
> 
> The current proposal to determine if an application's connection should be
> blocked is based on a request from the nsTrafficBlocker to the NetworkStats
> to retrieve the data usage of an application, then another request to the
> NetworkPolicies is needed in order to check if the allowed quota has been
> reached. Therefore, it means that there will be a lot of requests and
> messages that don't seem a good solution.
> 
> To simplify it, the idea is that NetworkStats will act as a proxy of
> NetworkPolicies, NetworkStats will have some kind of policies cache updated
> by notifications from the NetworkPolicies service. Then, as NetworkStats
> service is receiving updates of the data usage done by the applications, it
> can notify to the nsTrafficBlocker when an application has reached its quota
> and should be blocked when requesting a connection for 3g or wifi. So the
> nsTrafficBlocker can hold a list with applications to block.
> 
> When the NetworkStatsService starts it will have to check each application
> to evaluate which have reached the quota and, if so, send a notification to
> the blocker. Otherwise we can add a method to check if one application
> should be blocked in case that nsTrafficBlocker doesn't have info about what
> to do.
> 
> Do you think that this approach makes the implementation easier? Some issues
> or comments?

Hi Albert,

Is the event name defined?

Thanks

Comment 22

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

> Is the event name defined?

Or should we discuss the event name on this bug? :)

Comment 23

[Albert [:albert]]

(In reply to Patrick Wang [:kk1fff] from comment #22)
> > Is the event name defined?
> 
> Or should we discuss the event name on this bug? :)

The event name is not defined, we can call it 'application-quota-reached'.

And the method to check if an application has reached its quota can be:
boolean isApplicationBlocked(in long appID)

What do you think?

Comment 24

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Albert from comment #23)
> The event name is not defined, we can call it 'application-quota-reached'.
> 
> And the method to check if an application has reached its quota can be:
> boolean isApplicationBlocked(in long appID)
> 
> What do you think?

Sounds good to me. I think we might also need another event like 'application-quota-reset' to inform that the application is not need to be blocked anymore.

Comment 25

[Albert [:albert]]

We have 2 options:

We can call the event 'application-quota-status' and send a boolean variable 'isBlocked'.

We can send 'application-quota-reached' and 'application-quota-reset' events.

What do you prefer?

Comment 26

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Albert from comment #25)
> We have 2 options:
> 
> We can call the event 'application-quota-status' and send a boolean variable
> 'isBlocked'.
> 
> We can send 'application-quota-reached' and 'application-quota-reset' events.
> 
> What do you prefer?

I'd prefer the first one. And I think the event name could be changed to 'application-quota-status-changed', and the variable with the event would be named 'isReached'. I suggest changing the name because the net meter monitors the quota but doesn't actually block the traffic. How do you think?

So there will be two properties that is sent with the event: 1. Manifest URL or App ID. 2. A boolean value that indicates if the app should be blocked. Is that right?

Comment 27

[Albert [:albert]]

It is ok for me

Comment 28

[Valentin Gosu [:valentin] (he/him)]

Closing bug as b2g code is getting removed (bug 1306391).